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

[Hans de Goede]

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.

Regards,

Hans


p.s.

I've also seen your reply to 0/4, I will reply when I've some
more time.