[RFC PATCH 1/7] lmb: Replace lmb_reserve() with lmb_reserve_flags()
Ilias Apalodimas
ilias.apalodimas at linaro.org
Wed Dec 11 00:04:22 CET 2024
Thanks Simon,
On Tue, 10 Dec 2024 at 20:58, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias,
>
> On Tue, 10 Dec 2024 at 11:00, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi Simon,
> >
> >
> > On Tue, 10 Dec 2024 at 18:16, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, 10 Dec 2024 at 02:54, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > >
> > > > On Sun, 8 Dec 2024 at 16:22, Ilias Apalodimas
> > > > <ilias.apalodimas at linaro.org> wrote:
> > > > >
> > > > > lmb_reserve() is just calling lmb_reserve_flags() with LMB_NONE.
> > > > > There's not much we gain from this abstraction, so let's remove the
> > > > > former and make the code a bit easier to follow.
> > > > >
> > > > > The code size increase is minimal - e.g for sandbox which includes all
> > > > > of the LMB tests
> > > > >
> > > > > add/remove: 0/1 grow/shrink: 12/0 up/down: 46/-4 (42)
> > > > > Function old new delta
> > > > > lib_test_lmb_overlapping_reserve 1018 1030 +12
> > > > > version_string 70 76 +6
> > > > > test_get_unreserved_size 1032 1038 +6
> > > > > test_alloc_addr 2933 2939 +6
> > > > > test_multi_alloc.constprop 3034 3036 +2
> > > > > test_bigblock 911 913 +2
> > > > > load_serial 946 948 +2
> > > > > lib_test_lmb_flags 2101 2103 +2
> > > > > do_bootz 526 528 +2
> > > > > do_bootm_linux 2067 2069 +2
> > > > > bootm_run_states 5275 5277 +2
> > > > > boot_relocate_fdt 599 601 +2
> > > > > lmb_reserve 4 - -4
> > > > > Total: Before=2492742, After=2492784, chg +0.00%
> > > > >
> > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > > > > ---
> > > >
> > > > Fwiw, I had attempted this earlier on similar lines -- have a single
> > > > API, which would take the flag as a parameter [1], but was asked not
> > > > to take this approach as part of the review [2].
> > > >
> > >
> > > You missed that sandbox has LTO enabled. With that disabled (like many boards):
> > >
> > > sandbox: (for 1/1 boards) all -32.0 text -32.0
> > >
> > > We just had a patch and discussion to remove a single function call to
> > > save 8 bytes[3]
> > >
> > > For firefly-rk3399 this adds 36 bytes:
> > >
> > > aarch64: (for 1/1 boards) all +68.0 bss +48.0 text +20.0
> >
> > Did you include DM tests in that config? Because applying the series
> > says otherwise
> >
> > for firefly-rk3399_defconfig
> > add/remove: 0/5 grow/shrink: 14/0 up/down: 480/-544 (-64)
> > Function old new delta
> > lmb_alloc_base_flags 76 324 +248
> > lmb_alloc_addr_flags 4 144 +140
> > version_string 50 70 +20
> > boot_relocate_fdt 488 508 +20
> > boot_ramdisk_high 268 284 +16
> > tftp_handler 1304 1308 +4
> > load_serial 512 516 +4
> > lmb_alloc 8 12 +4
> > image_setup_libfdt 380 384 +4
> > do_spi_flash 2164 2168 +4
> > do_load 732 736 +4
> > do_bootz 332 336 +4
> > do_booti 520 524 +4
> > bootm_run_states 2176 2180 +4
> > lmb_reserve 8 - -8
> > lmb_alloc_addr 8 - -8
> > lmb_alloc_base 80 - -80
> > _lmb_alloc_addr 144 - -144
> > _lmb_alloc_base 304 - -304
> > Total: Before=691587, After=691523, chg -0.01%
>
> OK good
>
> >
> > >
> > > You could run buildman on all arm64 boards to check it, perhaps.
> >
> > I did test on non-LTO platforms as well, building for all is kind of
> > pointless no? It's either LTO or no LTO.
>
> Right
>
> > FWIW you need to apply the entire series, not just a single patch.
> >
>
> Yes, understood, I was just replying to that one patch.
>
> > rpi_arm64_defconfig
> > add/remove: 0/5 grow/shrink: 11/0 up/down: 452/-544 (-92)
> > Function old new delta
> > lmb_alloc_base_flags 76 324 +248
> > lmb_alloc_addr_flags 4 144 +140
> > boot_relocate_fdt 516 536 +20
> > boot_ramdisk_high 268 284 +16
> > tftp_handler 1552 1556 +4
> > load_serial 512 516 +4
> > lmb_alloc 8 12 +4
> > image_setup_libfdt 456 460 +4
> > do_load 728 732 +4
> > do_booti 520 524 +4
> > bootm_run_states 1824 1828 +4
> > lmb_reserve 8 - -8
> > lmb_alloc_addr 8 - -8
> > lmb_alloc_base 80 - -80
> > _lmb_alloc_addr 144 - -144
> > _lmb_alloc_base 304 - -304
> > Total: Before=542062, After=541970, chg -0.02%
> >
> > qemu_arm64_lwip_defconfig
> > add/remove: 0/5 grow/shrink: 11/0 up/down: 452/-544 (-92)
> > Function old new delta
> > lmb_alloc_base_flags 76 324 +248
> > lmb_alloc_addr_flags 4 144 +140
> > boot_relocate_fdt 488 508 +20
> > boot_ramdisk_high 268 284 +16
> > load_serial 548 552 +4
> > lmb_alloc 8 12 +4
> > image_setup_libfdt 368 372 +4
> > do_load 728 732 +4
> > do_bootz 332 336 +4
> > do_booti 520 524 +4
> > bootm_run_states 2176 2180 +4
> > lmb_reserve 8 - -8
> > lmb_alloc_addr 8 - -8
> > lmb_alloc_base 80 - -80
> > _lmb_alloc_addr 144 - -144
> > _lmb_alloc_base 304 - -304
> > Total: Before=1020122, After=1020030, chg -0.01%
> >
> >
> > >
> > > The other thing is that there is actually only one place apart from
> > > tests where the flag is needed - in boot_fdt_add_mem_rsv_regions().
> > > Boards don't use that flag. It is the flags themselves which are
> > > confusing.
> > >
> >
> > I still think it's way easier to read without two levels of
> > abstraction. Since the series decreases the size -- unless you include
> > all DM Tests, I'd like to keep the flags as is for now, so I can send
> > the next round which is the actual cleanup.
> > We can re-introduce them if someone finds a 'size problem'. But since
> > it's only when you enable DM I doubt anyone wants to have them working
> > in SPL or production firmware.
>
> OK, well I don't mind, if there is no growth. I wonder if you could do
> better overall without this patch, but perhaps not.
Probably yes, but we are not looking into anything more than a few
bytes -- probably under 4 as well.
TBH if we all agree that removing the non-flag variants is fine, I can
name the functions without it.
/Ilias
>
> >
> > /Ilias
> > > Regards,
> > > Simon
> > >
> > > > -sughosh
> > > >
> > > > [1] - https://lists.denx.de/pipermail/u-boot/2024-July/559790.html
> > > > [2] - https://lists.denx.de/pipermail/u-boot/2024-August/561607.html
> > > >
> > > > > arch/powerpc/cpu/mpc85xx/mp.c | 2 +-
> > > > > arch/powerpc/lib/misc.c | 2 +-
> > > > > boot/bootm.c | 3 ++-
> > > > > boot/image-board.c | 2 +-
> > > > > boot/image-fdt.c | 5 +++--
> > > > > cmd/booti.c | 2 +-
> > > > > cmd/bootz.c | 2 +-
> > > > > cmd/load.c | 2 +-
> > > > > include/lmb.h | 1 -
> > > > > lib/lmb.c | 5 -----
> > > > > test/lib/lmb.c | 30 +++++++++++++++---------------
> > > > > 11 files changed, 26 insertions(+), 30 deletions(-)
> > > > >
> > >
> > > [3] https://patchwork.ozlabs.org/project/uboot/patch/f79dc1fbf796dd5ad290f6080608ee68d7652cfc.1730452668.git.michal.simek@amd.com/
>
> Regards,
> SImon
More information about the U-Boot
mailing list