Hi,
On 05-02-15 12:24, Siarhei Siamashka wrote:
On Wed, 4 Feb 2015 13:05:50 +0100 Hans de Goede hdegoede@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@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;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.