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

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@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