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

Sean Anderson sean.anderson at seco.com
Tue Aug 1 18:27:08 CEST 2023



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))
> 
> This does not work either.
> 
> oldtopsize is needed in calloc() both w/ and w/o GD_FLG_FULL_MALLOC_INIT 
> and w/ and w/o SYS_MALLOC_RUNTIME_INIT to define the memory area to zero 
> out for calloc(). You must set it to the correct value irrespective of 
> these conditions.

No it's not.

   if (mem == NULL)
     return NULL;
   else
   {
#if CONFIG_VAL(SYS_MALLOC_F_LEN)
	if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
		memset(mem, 0, sz);
		return mem;
	}
#endif

There's no reference to oldtopsize here.

> Why don't you want to call malloc_init() here?

Why not just lazily call malloc_init and skip the whole malloc_simple
thing?

--Sean

> Best regards
> 
> Heinrich
> 
>>
>> 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
> 


More information about the U-Boot mailing list