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

[Sean Anderson]

On 8/1/23 12:56, Heinrich Schuchardt wrote:
> 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.

Ah. So maybe we should set this flag in malloc_init. This is what SPL
does (see board_init_r).

--Sean