Hi,
On 05-02-15 12:24, Siarhei Siamashka wrote:
On Wed, 4 Feb 2015 13:05:50 +0100
Hans de Goede <[email protected]> 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 <[email protected]>
---
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.
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
