Message ID | 20230928144458.2511087-1-sean.anderson@seco.com |
---|---|
State | RFC |
Delegated to: | Tom Rini |
Headers | show |
Hi Sean, On Thu, 28 Sept 2023 at 08:45, Sean Anderson <sean.anderson@seco.com> wrote: > > In my efforts to get SPL to fit into flash after some changes I made, I > noticed that av_ is one of the largest variables in SPL. As it turns > out, we can generate it at runtime, and the code is already there. This > has the potential to save 1-2k across the board, for some (very) minor > boot time increase. > > This series is based on [1], since this makes checking for SYS_MALLOC_F > easier. Passing CI at [2]. > > To measure the boot time difference, I applied the following patch: > > --- > common/board_r.c | 5 +++++ > common/spl/spl.c | 4 ++++ > 2 files changed, 9 insertions(+) > > diff --git a/common/board_r.c b/common/board_r.c > index 58a5986aa54..ca624b20d46 100644 > --- a/common/board_r.c > +++ b/common/board_r.c > @@ -194,6 +194,7 @@ static int initr_barrier(void) > return 0; > } > > +static ulong malloc_begin, malloc_end; > static int initr_malloc(void) > { > ulong malloc_start; > @@ -208,8 +209,10 @@ static int initr_malloc(void) > * reserve_noncached(). > */ > malloc_start = gd->relocaddr - TOTAL_MALLOC_LEN; > + malloc_begin = timer_get_boot_us(); Perhaps this would be better done with bootstage, since then the timing can be enabled / disabled, and reported along with other timings. > mem_malloc_init((ulong)map_sysmem(malloc_start, TOTAL_MALLOC_LEN), > TOTAL_MALLOC_LEN); > + malloc_end = timer_get_boot_us(); > gd->flags |= GD_FLG_FULL_MALLOC_INIT; > return 0; > } > @@ -570,6 +573,8 @@ static int dm_announce(void) > > static int run_main_loop(void) > { > + printf("malloc_init took %luus (%lu %lu)\n", malloc_end - malloc_begin, > + malloc_begin, malloc_end); > #ifdef CONFIG_SANDBOX > sandbox_main_loop_init(); > #endif > diff --git a/common/spl/spl.c b/common/spl/spl.c > index d74acec10b5..b34d1f4b4e6 100644 > --- a/common/spl/spl.c > +++ b/common/spl/spl.c > @@ -755,7 +755,9 @@ void board_init_r(gd_t *dummy1, ulong dummy2) > spl_set_bd(); > > #if defined(CONFIG_SYS_SPL_MALLOC) > + ulong malloc_begin = timer_get_boot_us(); > mem_malloc_init(SYS_SPL_MALLOC_START, CONFIG_SYS_SPL_MALLOC_SIZE); > + ulong malloc_end = timer_get_boot_us(); > gd->flags |= GD_FLG_FULL_MALLOC_INIT; > #endif > if (!(gd->flags & GD_FLG_SPL_INIT)) { > @@ -817,6 +819,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2) > spl_image.boot_device = BOOT_DEVICE_NONE; > board_boot_order(spl_boot_list); > > + printf("malloc_init took %luus (%lu %lu)\n", malloc_end - malloc_begin, > + malloc_begin, malloc_end); debug() ? > ret = boot_from_devices(&spl_image, spl_boot_list, > ARRAY_SIZE(spl_boot_list)); > if (ret) { > -- > 2.25.1 > > I found that MALLOC_CLEAR_ON_INIT dominated the mem_malloc_init time > (taking around 150 ms in SPL on my board). After disabling it, I found > that MALLOC_RUNTIME_INIT took around 5 us on average. > > [1] https://lore.kernel.org/u-boot/20230926141514.2101787-1-sjg@chromium.org/ > [2] https://source.denx.de/u-boot/custodians/u-boot-clk/-/pipelines/17900 > > Changes in v3: > - Use CONFIG_IS_ENABLED in conditionals > - Don't enable SPL_SYS_MALLOC_RUNTIME_INIT if we are short on BSS > > Changes in v2: > - Only mark malloc initialized after mem_malloc_init > - Fix cALLOc condition > > Sean Anderson (4): > common: Only mark malloc initialized after mem_malloc_init > malloc: Don't use ifdefs for SYS_MALLOC_DEFAULT_TO_INIT > malloc: Don't statically initialize av_ if using malloc_init > malloc: Enable SYS_MALLOC_RUNTIME_INIT by default in SPL > > Kconfig | 27 +++++++++++++++++---------- > common/board_r.c | 3 ++- > common/dlmalloc.c | 16 ++++++++-------- > 3 files changed, 27 insertions(+), 19 deletions(-) > > -- > 2.35.1.1320.gc452695387.dirty > REgards, SImon
On 10/1/23 21:16, Simon Glass wrote: > Hi Sean, > > On Thu, 28 Sept 2023 at 08:45, Sean Anderson <sean.anderson@seco.com> wrote: >> >> In my efforts to get SPL to fit into flash after some changes I made, I >> noticed that av_ is one of the largest variables in SPL. As it turns >> out, we can generate it at runtime, and the code is already there. This >> has the potential to save 1-2k across the board, for some (very) minor >> boot time increase. >> >> This series is based on [1], since this makes checking for SYS_MALLOC_F >> easier. Passing CI at [2]. >> >> To measure the boot time difference, I applied the following patch: >> >> --- >> common/board_r.c | 5 +++++ >> common/spl/spl.c | 4 ++++ >> 2 files changed, 9 insertions(+) >> >> diff --git a/common/board_r.c b/common/board_r.c >> index 58a5986aa54..ca624b20d46 100644 >> --- a/common/board_r.c >> +++ b/common/board_r.c >> @@ -194,6 +194,7 @@ static int initr_barrier(void) >> return 0; >> } >> >> +static ulong malloc_begin, malloc_end; >> static int initr_malloc(void) >> { >> ulong malloc_start; >> @@ -208,8 +209,10 @@ static int initr_malloc(void) >> * reserve_noncached(). >> */ >> malloc_start = gd->relocaddr - TOTAL_MALLOC_LEN; >> + malloc_begin = timer_get_boot_us(); > > Perhaps this would be better done with bootstage, since then the > timing can be enabled / disabled, and reported along with other > timings. I'll try that out next time. >> mem_malloc_init((ulong)map_sysmem(malloc_start, TOTAL_MALLOC_LEN), >> TOTAL_MALLOC_LEN); >> + malloc_end = timer_get_boot_us(); >> gd->flags |= GD_FLG_FULL_MALLOC_INIT; >> return 0; >> } >> @@ -570,6 +573,8 @@ static int dm_announce(void) >> >> static int run_main_loop(void) >> { >> + printf("malloc_init took %luus (%lu %lu)\n", malloc_end - malloc_begin, >> + malloc_begin, malloc_end); >> #ifdef CONFIG_SANDBOX >> sandbox_main_loop_init(); >> #endif >> diff --git a/common/spl/spl.c b/common/spl/spl.c >> index d74acec10b5..b34d1f4b4e6 100644 >> --- a/common/spl/spl.c >> +++ b/common/spl/spl.c >> @@ -755,7 +755,9 @@ void board_init_r(gd_t *dummy1, ulong dummy2) >> spl_set_bd(); >> >> #if defined(CONFIG_SYS_SPL_MALLOC) >> + ulong malloc_begin = timer_get_boot_us(); >> mem_malloc_init(SYS_SPL_MALLOC_START, CONFIG_SYS_SPL_MALLOC_SIZE); >> + ulong malloc_end = timer_get_boot_us(); >> gd->flags |= GD_FLG_FULL_MALLOC_INIT; >> #endif >> if (!(gd->flags & GD_FLG_SPL_INIT)) { >> @@ -817,6 +819,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2) >> spl_image.boot_device = BOOT_DEVICE_NONE; >> board_boot_order(spl_boot_list); >> >> + printf("malloc_init took %luus (%lu %lu)\n", malloc_end - malloc_begin, >> + malloc_begin, malloc_end); > > debug() ? Well, this is not going to be applied, so I used the easiest thing :) --Sean >> ret = boot_from_devices(&spl_image, spl_boot_list, >> ARRAY_SIZE(spl_boot_list)); >> if (ret) { >> -- >> 2.25.1 >> >> I found that MALLOC_CLEAR_ON_INIT dominated the mem_malloc_init time >> (taking around 150 ms in SPL on my board). After disabling it, I found >> that MALLOC_RUNTIME_INIT took around 5 us on average. >> >> [1] https://lore.kernel.org/u-boot/20230926141514.2101787-1-sjg@chromium.org/ >> [2] https://source.denx.de/u-boot/custodians/u-boot-clk/-/pipelines/17900 >> >> Changes in v3: >> - Use CONFIG_IS_ENABLED in conditionals >> - Don't enable SPL_SYS_MALLOC_RUNTIME_INIT if we are short on BSS >> >> Changes in v2: >> - Only mark malloc initialized after mem_malloc_init >> - Fix cALLOc condition >> >> Sean Anderson (4): >> common: Only mark malloc initialized after mem_malloc_init >> malloc: Don't use ifdefs for SYS_MALLOC_DEFAULT_TO_INIT >> malloc: Don't statically initialize av_ if using malloc_init >> malloc: Enable SYS_MALLOC_RUNTIME_INIT by default in SPL >> >> Kconfig | 27 +++++++++++++++++---------- >> common/board_r.c | 3 ++- >> common/dlmalloc.c | 16 ++++++++-------- >> 3 files changed, 27 insertions(+), 19 deletions(-) >> >> -- >> 2.35.1.1320.gc452695387.dirty >> > > REgards, > SImon
diff --git a/common/board_r.c b/common/board_r.c index 58a5986aa54..ca624b20d46 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -194,6 +194,7 @@ static int initr_barrier(void) return 0; } +static ulong malloc_begin, malloc_end; static int initr_malloc(void) { ulong malloc_start; @@ -208,8 +209,10 @@ static int initr_malloc(void) * reserve_noncached(). */ malloc_start = gd->relocaddr - TOTAL_MALLOC_LEN; + malloc_begin = timer_get_boot_us(); mem_malloc_init((ulong)map_sysmem(malloc_start, TOTAL_MALLOC_LEN), TOTAL_MALLOC_LEN); + malloc_end = timer_get_boot_us(); gd->flags |= GD_FLG_FULL_MALLOC_INIT; return 0; } @@ -570,6 +573,8 @@ static int dm_announce(void) static int run_main_loop(void) { + printf("malloc_init took %luus (%lu %lu)\n", malloc_end - malloc_begin, + malloc_begin, malloc_end); #ifdef CONFIG_SANDBOX sandbox_main_loop_init(); #endif diff --git a/common/spl/spl.c b/common/spl/spl.c index d74acec10b5..b34d1f4b4e6 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -755,7 +755,9 @@ void board_init_r(gd_t *dummy1, ulong dummy2) spl_set_bd(); #if defined(CONFIG_SYS_SPL_MALLOC) + ulong malloc_begin = timer_get_boot_us(); mem_malloc_init(SYS_SPL_MALLOC_START, CONFIG_SYS_SPL_MALLOC_SIZE); + ulong malloc_end = timer_get_boot_us(); gd->flags |= GD_FLG_FULL_MALLOC_INIT; #endif if (!(gd->flags & GD_FLG_SPL_INIT)) { @@ -817,6 +819,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2) spl_image.boot_device = BOOT_DEVICE_NONE; board_boot_order(spl_boot_list); + printf("malloc_init took %luus (%lu %lu)\n", malloc_end - malloc_begin, + malloc_begin, malloc_end); ret = boot_from_devices(&spl_image, spl_boot_list, ARRAY_SIZE(spl_boot_list)); if (ret) {