[PATCH 2/3] malloc: Don't statically initialize av_ if using malloc_init

[Sean Anderson]

On 8/1/23 03:04, Heinrich Schuchardt wrote:
> 
> 
> On 8/1/23 00:33, Sean Anderson wrote:
>> When we enable malloc_init, there is no need to statically initialize
>> av_, since we are going to do it manually. This lets us move av_ to
>> .bss, saving around 1-2k of data (depending on the pointer size).
>>
>> cALLOc must be adjusted to not access top before malloc_init.
>>
>> While we're at it, rename/reword the Kconfig to better describe what
>> this option does.
>>
>> Signed-off-by: Sean Anderson <sean.anderson at seco.com>
>> ---
>>
>>   Kconfig           | 18 +++++++-----------
>>   common/dlmalloc.c |  9 +++++++--
>>   2 files changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/Kconfig b/Kconfig
>> index 70efb41cc6..4b32286b69 100644
>> --- a/Kconfig
>> +++ b/Kconfig
>> @@ -372,18 +372,14 @@ if EXPERT
>>         When disabling this, please check if malloc calls, maybe
>>         should be replaced by calloc - if one expects zeroed memory.
>> -config SYS_MALLOC_DEFAULT_TO_INIT
>> -    bool "Default malloc to init while reserving the memory for it"
>> +config SYS_MALLOC_RUNTIME_INIT
>> +    bool "Initialize malloc's internal data at runtime"
>>       help
>> -      It may happen that one needs to move the dynamic allocation
>> -      from one to another memory range, eg. when moving the malloc
>> -      from the limited static to a potentially large dynamic (DDR)
>> -      memory.
>> -
>> -      If so then on top of setting the updated memory aside one
>> -      needs to bring the malloc init.
>> -
>> -      If such a scenario is sought choose yes.
>> +         Initialize malloc's internal data structures at runtime, 
>> rather than
>> +         at compile-time. This is necessary if relocating the malloc 
>> arena
>> +         from a smaller static memory to a large DDR memory. It can also
>> +         reduce the size of U-Boot by letting malloc's data reside in 
>> .bss
>> +         instead of .data.
>>   config TOOLS_DEBUG
>>       bool "Enable debug information for tools"
>> diff --git a/common/dlmalloc.c b/common/dlmalloc.c
>> index 30c78ae976..8a1daae5ec 100644
>> --- a/common/dlmalloc.c
>> +++ b/common/dlmalloc.c
>> @@ -556,6 +556,7 @@ typedef struct malloc_chunk* mbinptr;
>>   #define IAV(i)  bin_at(i), bin_at(i)
>>   static mbinptr av_[NAV * 2 + 2] = {
>> +#if !CONFIG_IS_ENABLED(SYS_MALLOC_RUNTIME_INIT)
>>    NULL, NULL,
>>    IAV(0),   IAV(1),   IAV(2),   IAV(3),   IAV(4),   IAV(5),   
>> IAV(6),   IAV(7),
>>    IAV(8),   IAV(9),   IAV(10),  IAV(11),  IAV(12),  IAV(13),  
>> IAV(14),  IAV(15),
>> @@ -573,6 +574,7 @@ static mbinptr av_[NAV * 2 + 2] = {
>>    IAV(104), IAV(105), IAV(106), IAV(107), IAV(108), IAV(109), 
>> IAV(110), IAV(111),
>>    IAV(112), IAV(113), IAV(114), IAV(115), IAV(116), IAV(117), 
>> IAV(118), IAV(119),
>>    IAV(120), IAV(121), IAV(122), IAV(123), IAV(124), IAV(125), 
>> IAV(126), IAV(127)
>> +#endif
>>   };
> 
> With this patch booting qemu-riscv64_spl_defconfig with 
> SYS_MALLOC_RUNTIME_INIT=y fails without output from main U-Boot.
> 
> After removing the #if above, main U-Boot output provides some output 
> but driver binding fails.
> 
> Without this patch and with SYS_MALLOC_DEFAULT_TO_INIT=y booting succeeds.
> 
>>   #ifdef CONFIG_NEEDS_MANUAL_RELOC
>> @@ -623,7 +625,7 @@ void mem_malloc_init(ulong start, ulong size)
>>       mem_malloc_end = start + size;
>>       mem_malloc_brk = start;
>> -    if (CONFIG_IS_ENABLED(SYS_MALLOC_DEFAULT_TO_INIT))
>> +    if (CONFIG_IS_ENABLED(SYS_MALLOC_RUNTIME_INIT))
>>           malloc_init();
>>       debug("using memory %#lx-%#lx for malloc()\n", mem_malloc_start,
>> @@ -2151,7 +2153,10 @@ Void_t* cALLOc(n, elem_size) size_t n; size_t 
>> elem_size;
>>   #ifdef CONFIG_SYS_MALLOC_CLEAR_ON_INIT
>>   #if MORECORE_CLEARS
>>     mchunkptr oldtop = top;
>> -  INTERNAL_SIZE_T oldtopsize = chunksize(top);
>> +  INTERNAL_SIZE_T oldtopsize;
>> +  if (CONFIG_IS_ENABLED(SYS_MALLOC_RUNTIME_INIT) &&
>> +      !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>> +    oldtopsize = chunksize(top);
> 
> malloc_simple needs a value for oldtopsize and this is before 
> GD_FLG_FULL_MALLOC_INIT is set.
> 
> This change worked for me:
> 
> #ifdef CONFIG_SYS_MALLOC_CLEAR_ON_INIT
> #if MORECORE_CLEARS
>    mchunkptr oldtop = top;
>    if (CONFIG_IS_ENABLED(SYS_MALLOC_RUNTIME_INIT) &&
>        !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>          malloc_init();
>    INTERNAL_SIZE_T oldtopsize = chunksize(top);
> #endif
> #endif
> 
> You don't want to call malloc_init() twice. So some flag should be added 
> indicating if malloc_init() has been invoked.

Actually, I think the original condition was just backwards. It should
be

	if (!CONFIG_IS_ENABLED(SYS_MALLOC_RUNTIME_INIT) &&
		(gd->flags & GD_FLG_FULL_MALLOC_INIT))

although this still doesn't match the malloc_simple condition. So maybe
the condition to remove the initialization should be

#if !CONFIG_VAL(SYS_MALLOC_F_LEN) && \
	!CONFIG_IS_ENABLED(SYS_MALLOC_RUNTIME_INIT)

or perhaps RUNTIME_INIT should depend on F_LEN? I don't see anyone using
for other purposes, so I think adding this dependency should be fine.

--Sean