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

[Heinrich Schuchardt]

On 01.08.23 18:27, Sean Anderson wrote:
> 
> 
> On 8/1/23 12:23, Heinrich Schuchardt wrote:
>> On 01.08.23 18:02, Sean Anderson wrote:
>>>
>>>
>>> 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))

GD_FLG_FULL_MALLOC_INIT is set in initr_reloc().

*Thereafter*

mem_malloc_init is called in initr_malloc().

See init_sequence_r[].

This explains why calling malloc_init() here helped me.

Best regards

Heinrich