[PATCH 0/2] make CONFIG_SPL_SYS_MALLOC_SIMPLE && CONFIG_SYS_SPL_MALLOC actually work
Currently, setting both CONFIG_SPL_SYS_MALLOC_SIMPLE and CONFIG_SYS_SPL_MALLOC (but not CONFIG_SPL_STACK_R) doesn't work as expected: The SIMPLE option means that all malloc etc. calls are directed at build-time to the implementation in malloc_simple.c, but the mem_alloc_init() call which is done in board_init_f() currently calls into dlmalloc.c, and updates the variables describing the dlmalloc arena - which is of course completely unused. And the malloc() implementation continues to hand out allocations from the initial SPL_SYS_MALLOC_F_LEN bytes.
These two patches are an attempt at making the combination in $subject actually work as one would expect: define an area of sdram to be used as a malloc arena, but still manage that using the malloc_simple() implementation.
Since this now changes the mem_alloc_init() from being a harmless no-op call to actually doing something, there's a risk of some boards breaking. The solution for those boards will probably be to just drop CONFIG_SYS_SPL_MALLOC, since that hasn't actually done anything.
Rasmus Villemoes (2): spl: make SYS_SPL_MALLOC depend on !(SPL_STACK_R && SPL_SYS_MALLOC_SIMPLE) malloc_simple: add mem_malloc_init_simple()
common/dlmalloc.c | 2 +- common/malloc_simple.c | 7 +++++++ common/spl/Kconfig | 1 + include/malloc.h | 7 +++++-- 4 files changed, 14 insertions(+), 3 deletions(-)
Currently, one can have both SYS_SPL_MALLOC=y and SPL_SYS_MALLOC_SIMPLE=y.
However, while the former does make board_init_r() in spl.c call mem_malloc_init(), that has no effect at all, because that just updates a few bookkeeping variables, but as the linker map shows, the latter setting has (as expected) caused most of dlmalloc.o to be garbage-collected. That is, those bookkeeping variables are not used for anything.
IOWs, with SYS_SPL_MALLOC=y and SPL_SYS_MALLOC_SIMPLE=y, the value of CONFIG_SYS_SPL_MALLOC_SIZE is irrelevant, and one still only has the small, SRAM-backed, malloc arena available.
Now I want to change that so that the mem_malloc_init() instead updates the gd->malloc* variables to point at the SDRAM area.
However, there's a small complication, namely when SPL_STACK_R=y is also in the mix. In that case, the "simple" malloc arena is indeed updated to point at the SDRAM area carved out of the new stack (see spl_relocate_stack_gd()). So that case works in the sense that one does get a "large" "simple" malloc arena (of size SPL_STACK_R_MALLOC_SIMPLE_LEN) - but CONFIG_SYS_SPL_MALLOC_SIZE is still irrelevant. Once I change the mem_malloc_init() logic, this would then break, because the gd->malloc* variables would be updated again. Also, it doesn't really make sense to allow the .config to essentially specify two different SDRAM-backed malloc arenas.
So since CONFIG_SYS_SPL_MALLOC and its dependent options are no-ops currently when SPL_STACK_R && SPL_SYS_MALLOC_SIMPLE, simply forbid that combination.
Simple grepping suggests that these boards/configs are affected, in that they become a tiny bit smaller, and the defconfig will need refreshing:
am62ax_evm_r5_defconfig am62x_evm_r5_defconfig am64x_evm_a53_defconfig am64x_evm_r5_defconfig am65x_evm_a53_defconfig am65x_hs_evm_a53_defconfig iot2050_defconfig j7200_evm_a72_defconfig j721e_evm_a72_defconfig j721s2_evm_a72_defconfig j721s2_evm_r5_defconfig verdin-am62_r5_defconfig
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- common/spl/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/common/spl/Kconfig b/common/spl/Kconfig index c5dd476db5..fca9ada337 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -405,6 +405,7 @@ config SPL_SEPARATE_BSS config SYS_SPL_MALLOC bool "Enable malloc pool in SPL" depends on SPL_FRAMEWORK + depends on !(SPL_STACK_R && SPL_SYS_MALLOC_SIMPLE)
config HAS_CUSTOM_SPL_MALLOC_START bool "For the SPL malloc pool, define a custom starting address"
On Fri, Sep 15, 2023 at 07:50:49PM +0200, Rasmus Villemoes wrote:
Currently, one can have both SYS_SPL_MALLOC=y and SPL_SYS_MALLOC_SIMPLE=y.
However, while the former does make board_init_r() in spl.c call mem_malloc_init(), that has no effect at all, because that just updates a few bookkeeping variables, but as the linker map shows, the latter setting has (as expected) caused most of dlmalloc.o to be garbage-collected. That is, those bookkeeping variables are not used for anything.
IOWs, with SYS_SPL_MALLOC=y and SPL_SYS_MALLOC_SIMPLE=y, the value of CONFIG_SYS_SPL_MALLOC_SIZE is irrelevant, and one still only has the small, SRAM-backed, malloc arena available.
Now I want to change that so that the mem_malloc_init() instead updates the gd->malloc* variables to point at the SDRAM area.
However, there's a small complication, namely when SPL_STACK_R=y is also in the mix. In that case, the "simple" malloc arena is indeed updated to point at the SDRAM area carved out of the new stack (see spl_relocate_stack_gd()). So that case works in the sense that one does get a "large" "simple" malloc arena (of size SPL_STACK_R_MALLOC_SIMPLE_LEN) - but CONFIG_SYS_SPL_MALLOC_SIZE is still irrelevant. Once I change the mem_malloc_init() logic, this would then break, because the gd->malloc* variables would be updated again. Also, it doesn't really make sense to allow the .config to essentially specify two different SDRAM-backed malloc arenas.
So since CONFIG_SYS_SPL_MALLOC and its dependent options are no-ops currently when SPL_STACK_R && SPL_SYS_MALLOC_SIMPLE, simply forbid that combination.
Simple grepping suggests that these boards/configs are affected, in that they become a tiny bit smaller, and the defconfig will need refreshing:
am62ax_evm_r5_defconfig am62x_evm_r5_defconfig am64x_evm_a53_defconfig am64x_evm_r5_defconfig am65x_evm_a53_defconfig am65x_hs_evm_a53_defconfig iot2050_defconfig j7200_evm_a72_defconfig j721e_evm_a72_defconfig j721s2_evm_a72_defconfig j721s2_evm_r5_defconfig verdin-am62_r5_defconfig
I've boot tested this on j721e and am65x evms, so it's likely true that all of these platforms are fine.
On Fri, 15 Sept 2023 at 11:51, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
Currently, one can have both SYS_SPL_MALLOC=y and SPL_SYS_MALLOC_SIMPLE=y.
However, while the former does make board_init_r() in spl.c call mem_malloc_init(), that has no effect at all, because that just updates a few bookkeeping variables, but as the linker map shows, the latter setting has (as expected) caused most of dlmalloc.o to be garbage-collected. That is, those bookkeeping variables are not used for anything.
IOWs, with SYS_SPL_MALLOC=y and SPL_SYS_MALLOC_SIMPLE=y, the value of CONFIG_SYS_SPL_MALLOC_SIZE is irrelevant, and one still only has the small, SRAM-backed, malloc arena available.
Now I want to change that so that the mem_malloc_init() instead updates the gd->malloc* variables to point at the SDRAM area.
However, there's a small complication, namely when SPL_STACK_R=y is also in the mix. In that case, the "simple" malloc arena is indeed updated to point at the SDRAM area carved out of the new stack (see spl_relocate_stack_gd()). So that case works in the sense that one does get a "large" "simple" malloc arena (of size SPL_STACK_R_MALLOC_SIMPLE_LEN) - but CONFIG_SYS_SPL_MALLOC_SIZE is still irrelevant. Once I change the mem_malloc_init() logic, this would then break, because the gd->malloc* variables would be updated again. Also, it doesn't really make sense to allow the .config to essentially specify two different SDRAM-backed malloc arenas.
So since CONFIG_SYS_SPL_MALLOC and its dependent options are no-ops currently when SPL_STACK_R && SPL_SYS_MALLOC_SIMPLE, simply forbid that combination.
Simple grepping suggests that these boards/configs are affected, in that they become a tiny bit smaller, and the defconfig will need refreshing:
am62ax_evm_r5_defconfig am62x_evm_r5_defconfig am64x_evm_a53_defconfig am64x_evm_r5_defconfig am65x_evm_a53_defconfig am65x_hs_evm_a53_defconfig iot2050_defconfig j7200_evm_a72_defconfig j721e_evm_a72_defconfig j721s2_evm_a72_defconfig j721s2_evm_r5_defconfig verdin-am62_r5_defconfig
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
common/spl/Kconfig | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Simon Glass sjg@chromium.org
I was running out of malloc() in SPL, and the message told me to look at CONFIG_SYS_SPL_MALLOC_SIZE. So I did, and bumped it quite a bit, but that had no effect whatsoever.
The reason for that was that I also have CONFIG_SPL_SYS_MALLOC_SIMPLE=y. So while board_init_r() in spl.c duly calls
mem_malloc_init(SYS_SPL_MALLOC_START, CONFIG_SYS_SPL_MALLOC_SIZE);
that doesn't actually do anything, because that function just sets some variables in dlmalloc.c, and (as the linker map shows), the rest of dlmalloc.o has been garbage-collected.
I don't want to have the full dlmalloc implementation in SPL - code size is still precious. However, once SDRAM is initialized, the heap is practically infinite, if only CONFIG_SYS_SPL_MALLOC_SIZE actually had an effect.
So just as CONFIG_SPL_SYS_MALLOC_SIMPLE redirects malloc() and friends at build-time to the _simple variants, add a _simple variant of mem_malloc_init() which will actually update the bookkeeping variables relevant to the actual and active malloc() implementation.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- common/dlmalloc.c | 2 +- common/malloc_simple.c | 7 +++++++ include/malloc.h | 7 +++++-- 3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/common/dlmalloc.c b/common/dlmalloc.c index dcecdb8623..d42b26410f 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -619,7 +619,7 @@ void *sbrk(ptrdiff_t increment) return (void *)old; }
-void mem_malloc_init(ulong start, ulong size) +void mem_dlmalloc_init(ulong start, ulong size) { mem_malloc_start = start; mem_malloc_end = start + size; diff --git a/common/malloc_simple.c b/common/malloc_simple.c index 0a004d40e1..9ecf05cf2e 100644 --- a/common/malloc_simple.c +++ b/common/malloc_simple.c @@ -17,6 +17,13 @@
DECLARE_GLOBAL_DATA_PTR;
+void mem_malloc_init_simple(ulong start, ulong size) +{ + gd->malloc_base = start; + gd->malloc_ptr = 0; + gd->malloc_limit = size; +} + static void *alloc_simple(size_t bytes, int align) { ulong addr, new_ptr; diff --git a/include/malloc.h b/include/malloc.h index 161ccbd129..f59942115b 100644 --- a/include/malloc.h +++ b/include/malloc.h @@ -899,6 +899,7 @@ void malloc_disable_testing(void); #define malloc malloc_simple #define realloc realloc_simple #define memalign memalign_simple +#define mem_malloc_init mem_malloc_init_simple #if IS_ENABLED(CONFIG_VALGRIND) #define free free_simple #else @@ -908,6 +909,9 @@ void *calloc(size_t nmemb, size_t size); void *realloc_simple(void *ptr, size_t size); #else
+#define mem_malloc_init mem_dlmalloc_init +void mem_dlmalloc_init(ulong start, ulong size); + # ifdef USE_DL_PREFIX # define cALLOc dlcalloc # define fREe dlfree @@ -955,6 +959,7 @@ int initf_malloc(void); /* Simple versions which can be used when space is tight */ void *malloc_simple(size_t size); void *memalign_simple(size_t alignment, size_t bytes); +void mem_malloc_init_simple(ulong start, ulong size);
#pragma GCC visibility push(hidden) # if __STD_C @@ -997,8 +1002,6 @@ extern ulong mem_malloc_start; extern ulong mem_malloc_end; extern ulong mem_malloc_brk;
-void mem_malloc_init(ulong start, ulong size); - #ifdef __cplusplus }; /* end of extern "C" */ #endif
Hi Rasmus,
On Fri, 15 Sept 2023 at 11:51, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
I was running out of malloc() in SPL, and the message told me to look at CONFIG_SYS_SPL_MALLOC_SIZE. So I did, and bumped it quite a bit, but that had no effect whatsoever.
The reason for that was that I also have CONFIG_SPL_SYS_MALLOC_SIMPLE=y. So while board_init_r() in spl.c duly calls
mem_malloc_init(SYS_SPL_MALLOC_START, CONFIG_SYS_SPL_MALLOC_SIZE);
that doesn't actually do anything, because that function just sets some variables in dlmalloc.c, and (as the linker map shows), the rest of dlmalloc.o has been garbage-collected.
I don't want to have the full dlmalloc implementation in SPL - code size is still precious. However, once SDRAM is initialized, the heap is practically infinite, if only CONFIG_SYS_SPL_MALLOC_SIZE actually had an effect.
So just as CONFIG_SPL_SYS_MALLOC_SIMPLE redirects malloc() and friends at build-time to the _simple variants, add a _simple variant of mem_malloc_init() which will actually update the bookkeeping variables relevant to the actual and active malloc() implementation.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
common/dlmalloc.c | 2 +- common/malloc_simple.c | 7 +++++++ include/malloc.h | 7 +++++-- 3 files changed, 13 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
width change below
diff --git a/common/dlmalloc.c b/common/dlmalloc.c index dcecdb8623..d42b26410f 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -619,7 +619,7 @@ void *sbrk(ptrdiff_t increment) return (void *)old; }
-void mem_malloc_init(ulong start, ulong size) +void mem_dlmalloc_init(ulong start, ulong size) { mem_malloc_start = start; mem_malloc_end = start + size; diff --git a/common/malloc_simple.c b/common/malloc_simple.c index 0a004d40e1..9ecf05cf2e 100644 --- a/common/malloc_simple.c +++ b/common/malloc_simple.c @@ -17,6 +17,13 @@
DECLARE_GLOBAL_DATA_PTR;
+void mem_malloc_init_simple(ulong start, ulong size) +{
gd->malloc_base = start;gd->malloc_ptr = 0;gd->malloc_limit = size;+}
static void *alloc_simple(size_t bytes, int align) { ulong addr, new_ptr; diff --git a/include/malloc.h b/include/malloc.h index 161ccbd129..f59942115b 100644 --- a/include/malloc.h +++ b/include/malloc.h @@ -899,6 +899,7 @@ void malloc_disable_testing(void); #define malloc malloc_simple #define realloc realloc_simple #define memalign memalign_simple +#define mem_malloc_init mem_malloc_init_simple #if IS_ENABLED(CONFIG_VALGRIND) #define free free_simple #else @@ -908,6 +909,9 @@ void *calloc(size_t nmemb, size_t size); void *realloc_simple(void *ptr, size_t size); #else
+#define mem_malloc_init mem_dlmalloc_init +void mem_dlmalloc_init(ulong start, ulong size);
Please add a full comment here
# ifdef USE_DL_PREFIX # define cALLOc dlcalloc # define fREe dlfree @@ -955,6 +959,7 @@ int initf_malloc(void); /* Simple versions which can be used when space is tight */ void *malloc_simple(size_t size); void *memalign_simple(size_t alignment, size_t bytes); +void mem_malloc_init_simple(ulong start, ulong size);
Please add a full comment here
#pragma GCC visibility push(hidden) # if __STD_C @@ -997,8 +1002,6 @@ extern ulong mem_malloc_start; extern ulong mem_malloc_end; extern ulong mem_malloc_brk;
-void mem_malloc_init(ulong start, ulong size);
#ifdef __cplusplus }; /* end of extern "C" */
#endif
2.37.2
Regards, Simon
Hi Rasmus,
On Fri, 15 Sept 2023 at 11:51, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
Currently, setting both CONFIG_SPL_SYS_MALLOC_SIMPLE and CONFIG_SYS_SPL_MALLOC (but not CONFIG_SPL_STACK_R) doesn't work as expected: The SIMPLE option means that all malloc etc. calls are directed at build-time to the implementation in malloc_simple.c, but
Sort-of. It is control by both a CONFIG and GD_FLG_FULL_MALLOC_INIT.
the mem_alloc_init() call which is done in board_init_f() currently calls into dlmalloc.c, and updates the variables describing the dlmalloc arena - which is of course completely unused. And the malloc() implementation continues to hand out allocations from the initial SPL_SYS_MALLOC_F_LEN bytes.
These two patches are an attempt at making the combination in $subject actually work as one would expect: define an area of sdram to be used as a malloc arena, but still manage that using the malloc_simple() implementation.
Since this now changes the mem_alloc_init() from being a harmless no-op call to actually doing something, there's a risk of some boards breaking. The solution for those boards will probably be to just drop CONFIG_SYS_SPL_MALLOC, since that hasn't actually done anything.
Rasmus Villemoes (2): spl: make SYS_SPL_MALLOC depend on !(SPL_STACK_R && SPL_SYS_MALLOC_SIMPLE) malloc_simple: add mem_malloc_init_simple()
common/dlmalloc.c | 2 +- common/malloc_simple.c | 7 +++++++ common/spl/Kconfig | 1 + include/malloc.h | 7 +++++-- 4 files changed, 14 insertions(+), 3 deletions(-)
-- 2.37.2
Regards, Simon
On 21/09/2023 03.02, Simon Glass wrote:
Hi Rasmus,
On Fri, 15 Sept 2023 at 11:51, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
Currently, setting both CONFIG_SPL_SYS_MALLOC_SIMPLE and CONFIG_SYS_SPL_MALLOC (but not CONFIG_SPL_STACK_R) doesn't work as expected: The SIMPLE option means that all malloc etc. calls are directed at build-time to the implementation in malloc_simple.c, but
Sort-of. It is control by both a CONFIG and GD_FLG_FULL_MALLOC_INIT.
Eh, no? The CONFIG option completely preempts any run-time gd flag checking
#if CONFIG_IS_ENABLED(SYS_MALLOC_SIMPLE) #define malloc malloc_simple #define realloc realloc_simple #define memalign memalign_simple
Yes, _without_ that CONFIG option, the "real" malloc() functions do check that gd flag and if not set fall back to the _simple variants. But what I wrote is not merely "sort-of" correct.
Rasmus
Hi Rasmus,
On Mon, 25 Sept 2023 at 02:54, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
On 21/09/2023 03.02, Simon Glass wrote:
Hi Rasmus,
On Fri, 15 Sept 2023 at 11:51, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
Currently, setting both CONFIG_SPL_SYS_MALLOC_SIMPLE and CONFIG_SYS_SPL_MALLOC (but not CONFIG_SPL_STACK_R) doesn't work as expected: The SIMPLE option means that all malloc etc. calls are directed at build-time to the implementation in malloc_simple.c, but
Sort-of. It is control by both a CONFIG and GD_FLG_FULL_MALLOC_INIT.
Eh, no? The CONFIG option completely preempts any run-time gd flag checking
#if CONFIG_IS_ENABLED(SYS_MALLOC_SIMPLE) #define malloc malloc_simple #define realloc realloc_simple #define memalign memalign_simple
Yes, _without_ that CONFIG option, the "real" malloc() functions do check that gd flag and if not set fall back to the _simple variants. But what I wrote is not merely "sort-of" correct.
OK I see what you mean, yes.
Regards, Simon
participants (3)
-
Rasmus Villemoes -
Simon Glass -
Tom Rini