[U-Boot] [PATCH 3/4] malloc_simple: Return NULL on malloc failure rather then calling panic()

[Simon Glass]

On 5 February 2015 at 11:00, Simon Glass <sjg at chromium.org> wrote:
> Hi,
>
> On 5 February 2015 at 10:53, Hans de Goede <hdegoede at redhat.com> wrote:
>> Hi,
>>
>>
>> On 05-02-15 12:24, Siarhei Siamashka wrote:
>>>
>>> On Wed,  4 Feb 2015 13:05:50 +0100
>>> Hans de Goede <hdegoede at redhat.com> wrote:
>>>
>>>> All callers of malloc should already do error checking, and may even be
>>>> able
>>>> to continue without the alloc succeeding.
>>>>
>>>> Moreover, common/malloc_simple.c is the only user of .rodata.str1.1 in
>>>> common/built-in.o when building the SPL, triggering this gcc bug:
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54303
>>>>
>>>> Causing .rodata to grow with e.g. 0xc21 bytes, nullifying all benefits of
>>>> using malloc_simple in the first place.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>> ---
>>>>   common/malloc_simple.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/common/malloc_simple.c b/common/malloc_simple.c
>>>> index afdacff..64ae036 100644
>>>> --- a/common/malloc_simple.c
>>>> +++ b/common/malloc_simple.c
>>>> @@ -19,7 +19,7 @@ void *malloc_simple(size_t bytes)
>>>>
>>>>         new_ptr = gd->malloc_ptr + bytes;
>>>>         if (new_ptr > gd->malloc_limit)
>>>> -               panic("Out of pre-reloc memory");
>>>> +               return NULL;
>>>>         ptr = map_sysmem(gd->malloc_base + gd->malloc_ptr, bytes);
>>>>         gd->malloc_ptr = ALIGN(new_ptr, sizeof(new_ptr));
>>>>         return ptr;
>>>
>>>
>>> The other patches look great, but I'm not convinced that requiring the
>>> malloc callers to do error checking is such a great idea. This means a
>>> lot of checks in a lot of places with extra code paths instead of just
>>> a single check in one place for the "impossible to happen" critical
>>> failure. I think that we should normally assume that malloc always
>>> succeeds in the production code and the panic may only happen while
>>> debugging.
>>>
>>> If the malloc pool is in the DRAM, then we usually have orders of
>>> magnitude more space than necessary. While the code might be still
>>> in the SRAM at the same time (the extra branching code logic for
>>> errors checking may be wasting the scarce SRAM space).
>>>
>>> If the malloc pool is in the SRAM and potentially may fail allocations,
>>> then this is a major reliability problem by itself. The malloc pool is
>>> always inefficient, has fragmentation problems, etc. If this is the
>>> case, then IMHO the only right solution is to replace such problematic
>>> dynamic allocations with static reservations in the ".data" section.
>>> Otherwise the reliability critical things (something like Mars rovers
>>> for example) will be sometimes failing. The Murphy law exists for
>>> a reason :-)
>>
>>
>> Actually we had a discussion about this at the u-boot miniconf at ELCE,
>> I was actually advocating for having malloc behave as gmalloc from
>> glib2 does, so basically what you're advocating for, but I lost the
>> discussion. All this patch does is bring the simple, light-weight malloc
>> from malloc_simple.c inline with the regular malloc implementation which
>> can already fail.
>>
>>> The workaround for the GCC compiler bug is orthogonal to this.
>>> Maybe there is some other solution?
>>
>>
>> To workaround this gcc bug we need to not use any const strings.
>> I guess we could do something like this and still panic ():
>>
>> char str[4];
>>
>> str[0] = 'O';
>> str[1] = 'O';
>> str[2] = 'M';
>> str[3] = 0;
>>
>> But I think that just removing the panic is better, as it makes
>> malloc_simple.c consistent with the regular malloc. In the end
>> we should really get the gcc bug fixed, this will likely
>> trim down .rodata significantly.
>
> Yes I'd like to apply this one. It works around a really annoying gcc bug.
>
> Hans I think you are right that we can deal with the errors at the top
> level, as we do with other things. SPL error handling is probably not
> great, but that's a separate issue.
>
> Regards,
> Simon

Applied to u-boot-dm, thanks!