| Message ID | 20240724060224.3071065-5-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Make LMB memory map global and persistent | expand |
On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > From: Simon Glass <sjg@chromium.org> > > Use this new data structure in the utility function. > > Signed-off-by: Simon Glass <sjg@chromium.org> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > lib/strto.c | 35 +++++++++++++++++++---------------- > 1 file changed, 19 insertions(+), 16 deletions(-) This is rather big growth when we didn't already have realloc: 05: lib: Convert str_to_list() to use alist aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) function old new delta realloc - 1120 +1120 alist_ensure_ptr - 140 +140 alist_expand_to - 136 +136 alist_init - 108 +108 alist_uninit_move_ptr - 76 +76 alist_add_ptr - 72 +72 alist_uninit - 48 +48 str_to_list 204 232 +28
Hi Tom, On Wed, 24 Jul 2024 at 14:54, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > From: Simon Glass <sjg@chromium.org> > > > > Use this new data structure in the utility function. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > lib/strto.c | 35 +++++++++++++++++++---------------- > > 1 file changed, 19 insertions(+), 16 deletions(-) > > This is rather big growth when we didn't already have realloc: > 05: lib: Convert str_to_list() to use alist > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > function old new delta > realloc - 1120 +1120 > alist_ensure_ptr - 140 +140 > alist_expand_to - 136 +136 > alist_init - 108 +108 > alist_uninit_move_ptr - 76 +76 > alist_add_ptr - 72 +72 > alist_uninit - 48 +48 > str_to_list 204 232 +28 > > -- > Tom We can just drop this patch, perhaps? It was really just a demo of alist. Regards, SImon
On Wed, Jul 24, 2024 at 04:17:32PM -0600, Simon Glass wrote: > Hi Tom, > > On Wed, 24 Jul 2024 at 14:54, Tom Rini <trini@konsulko.com> wrote: > > > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > > > From: Simon Glass <sjg@chromium.org> > > > > > > Use this new data structure in the utility function. > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > lib/strto.c | 35 +++++++++++++++++++---------------- > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > This is rather big growth when we didn't already have realloc: > > 05: lib: Convert str_to_list() to use alist > > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > > function old new delta > > realloc - 1120 +1120 > > alist_ensure_ptr - 140 +140 > > alist_expand_to - 136 +136 > > alist_init - 108 +108 > > alist_uninit_move_ptr - 76 +76 > > alist_add_ptr - 72 +72 > > alist_uninit - 48 +48 > > str_to_list 204 232 +28 > > We can just drop this patch, perhaps? It was really just a demo of alist. Yes, and no. The next good example would be on cortina_presidio-asic-pnand where the reason this series is a size growth rather than reduction (where it is BTW on LTO using platforms) is again, realloc. Is this just an unfortunate given then?
Hi Tom, On Wed, 24 Jul 2024 at 16:35, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Jul 24, 2024 at 04:17:32PM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Wed, 24 Jul 2024 at 14:54, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > > > > > From: Simon Glass <sjg@chromium.org> > > > > > > > > Use this new data structure in the utility function. > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > lib/strto.c | 35 +++++++++++++++++++---------------- > > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > > > This is rather big growth when we didn't already have realloc: > > > 05: lib: Convert str_to_list() to use alist > > > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > > > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > > > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > > > function old new delta > > > realloc - 1120 +1120 > > > alist_ensure_ptr - 140 +140 > > > alist_expand_to - 136 +136 > > > alist_init - 108 +108 > > > alist_uninit_move_ptr - 76 +76 > > > alist_add_ptr - 72 +72 > > > alist_uninit - 48 +48 > > > str_to_list 204 232 +28 > > > > We can just drop this patch, perhaps? It was really just a demo of alist. > > Yes, and no. The next good example would be on > cortina_presidio-asic-pnand where the reason this series is a size > growth rather than reduction (where it is BTW on LTO using platforms) is > again, realloc. Is this just an unfortunate given then? Hmmm maybe. We can of course malloc() and then free() rather than calling realloc(), although that is likely not as efficient. We could have a 'cut down' alist implementation which requires that the initial size be declared, but that defeats the point, really. Anyway we certainly don't need to apply this patch to strto...there is no particular benefit. I'll leave it up to you. Regards, Simon
On 7/24/24 22:54, Tom Rini wrote: > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > >> From: Simon Glass <sjg@chromium.org> >> >> Use this new data structure in the utility function. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> >> --- >> lib/strto.c | 35 +++++++++++++++++++---------------- >> 1 file changed, 19 insertions(+), 16 deletions(-) > > This is rather big growth when we didn't already have realloc: > 05: lib: Convert str_to_list() to use alist > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > function old new delta > realloc - 1120 +1120 > alist_ensure_ptr - 140 +140 > alist_expand_to - 136 +136 > alist_init - 108 +108 > alist_uninit_move_ptr - 76 +76 > alist_add_ptr - 72 +72 > alist_uninit - 48 +48 > str_to_list 204 232 +28 > this is definitely not acceptable. This mini configuration is running out of OCM and we are already pretty close to limit. What's the reason for this change? I can't see any explanation in commit message. Thanks, Michal
On Thu, 25 Jul 2024 at 02:24, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > From: Simon Glass <sjg@chromium.org> > > > > Use this new data structure in the utility function. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > lib/strto.c | 35 +++++++++++++++++++---------------- > > 1 file changed, 19 insertions(+), 16 deletions(-) > > This is rather big growth when we didn't already have realloc: > 05: lib: Convert str_to_list() to use alist > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > function old new delta > realloc - 1120 +1120 > alist_ensure_ptr - 140 +140 > alist_expand_to - 136 +136 > alist_init - 108 +108 > alist_uninit_move_ptr - 76 +76 > alist_add_ptr - 72 +72 > alist_uninit - 48 +48 > str_to_list 204 232 +28 > I am working on an implementation of lmb maps using lists. The list nodes are then allocated with calloc, which I believe is included in most of the board images. We can then compare the size impact with the two implementations (alist vs list). Thanks. -sughosh > -- > Tom
On 7/25/24 14:54, Sughosh Ganu wrote: > On Thu, 25 Jul 2024 at 02:24, Tom Rini <trini@konsulko.com> wrote: >> >> On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: >> >>> From: Simon Glass <sjg@chromium.org> >>> >>> Use this new data structure in the utility function. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> >>> --- >>> lib/strto.c | 35 +++++++++++++++++++---------------- >>> 1 file changed, 19 insertions(+), 16 deletions(-) >> >> This is rather big growth when we didn't already have realloc: >> 05: lib: Convert str_to_list() to use alist >> aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 >> xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 >> u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) >> function old new delta >> realloc - 1120 +1120 >> alist_ensure_ptr - 140 +140 >> alist_expand_to - 136 +136 >> alist_init - 108 +108 >> alist_uninit_move_ptr - 76 +76 >> alist_add_ptr - 72 +72 >> alist_uninit - 48 +48 >> str_to_list 204 232 +28 >> > > I am working on an implementation of lmb maps using lists. The list > nodes are then allocated with calloc, which I believe is included in > most of the board images. We can then compare the size impact with the > two implementations (alist vs list). Thanks. Inside LMB code it should be fine for us because as I wrote we are disabling LMB in these mini configurations. Thanks, Michal
Hi Michal, On Thu, 25 Jul 2024 at 00:14, Michal Simek <michal.simek@amd.com> wrote: > > > > On 7/24/24 22:54, Tom Rini wrote: > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > >> From: Simon Glass <sjg@chromium.org> > >> > >> Use this new data structure in the utility function. > >> > >> Signed-off-by: Simon Glass <sjg@chromium.org> > >> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > >> --- > >> lib/strto.c | 35 +++++++++++++++++++---------------- > >> 1 file changed, 19 insertions(+), 16 deletions(-) > > > > This is rather big growth when we didn't already have realloc: > > 05: lib: Convert str_to_list() to use alist > > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > > function old new delta > > realloc - 1120 +1120 > > alist_ensure_ptr - 140 +140 > > alist_expand_to - 136 +136 > > alist_init - 108 +108 > > alist_uninit_move_ptr - 76 +76 > > alist_add_ptr - 72 +72 > > alist_uninit - 48 +48 > > str_to_list 204 232 +28 > > > > this is definitely not acceptable. This mini configuration is running out of OCM > and we are already pretty close to limit. Also I forgot to mention that mx6sabresd exceeds its limit with this patch. > > What's the reason for this change? I can't see any explanation in commit message. Much like the abuf library (which initially had no users), I try to avoid adding dead code. So some sort of example is helpful. The bug that I found in str_to_list() was fixed in the earlier commit in the series, without needing alist. so it isn't necessary for that. Regards, Simon
Hi Sughosh, On Thu, 25 Jul 2024 at 06:54, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Thu, 25 Jul 2024 at 02:24, Tom Rini <trini@konsulko.com> wrote: > > > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > > > From: Simon Glass <sjg@chromium.org> > > > > > > Use this new data structure in the utility function. > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > lib/strto.c | 35 +++++++++++++++++++---------------- > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > This is rather big growth when we didn't already have realloc: > > 05: lib: Convert str_to_list() to use alist > > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > > function old new delta > > realloc - 1120 +1120 > > alist_ensure_ptr - 140 +140 > > alist_expand_to - 136 +136 > > alist_init - 108 +108 > > alist_uninit_move_ptr - 76 +76 > > alist_add_ptr - 72 +72 > > alist_uninit - 48 +48 > > str_to_list 204 232 +28 > > > > I am working on an implementation of lmb maps using lists. The list > nodes are then allocated with calloc, which I believe is included in > most of the board images. We can then compare the size impact with the > two implementations (alist vs list). Thanks. This is for use with EFI, which is very large, so I doubt it is worth it. The alist code is simpler to work with and will be used in UPL and likely some other areas. We could provide a variant which uses malloc()/free() instead of realloc(), as I mentioned/ The impact then would be very small. Regards, Simon
On Thu, Jul 25, 2024 at 07:59:52AM -0600, Simon Glass wrote: > Hi Sughosh, > > On Thu, 25 Jul 2024 at 06:54, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Thu, 25 Jul 2024 at 02:24, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > > > > > From: Simon Glass <sjg@chromium.org> > > > > > > > > Use this new data structure in the utility function. > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > lib/strto.c | 35 +++++++++++++++++++---------------- > > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > > > This is rather big growth when we didn't already have realloc: > > > 05: lib: Convert str_to_list() to use alist > > > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > > > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > > > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > > > function old new delta > > > realloc - 1120 +1120 > > > alist_ensure_ptr - 140 +140 > > > alist_expand_to - 136 +136 > > > alist_init - 108 +108 > > > alist_uninit_move_ptr - 76 +76 > > > alist_add_ptr - 72 +72 > > > alist_uninit - 48 +48 > > > str_to_list 204 232 +28 > > > > > > > I am working on an implementation of lmb maps using lists. The list > > nodes are then allocated with calloc, which I believe is included in > > most of the board images. We can then compare the size impact with the > > two implementations (alist vs list). Thanks. > > This is for use with EFI, Am I missing some context? The LMB rework is because what we have today doesn't work all that well for what we need in a modern system (and with modern security concious eyes on the code) with EFI_LOADER=n. > which is very large, so I doubt it is worth > it. The alist code is simpler to work with and will be used in UPL and > likely some other areas. > > We could provide a variant which uses malloc()/free() instead of > realloc(), as I mentioned/ The impact then would be very small. Switching to malloc/free instead might be fine as well, yes. Aside from the realloc growth most platforms are at a minor size shrink. If we address realloc then we get something more functional and smaller. This should make everyone happy.
On Thu, Jul 25, 2024 at 08:13:38AM +0200, Michal Simek wrote: > > > On 7/24/24 22:54, Tom Rini wrote: > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > > > From: Simon Glass <sjg@chromium.org> > > > > > > Use this new data structure in the utility function. > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > lib/strto.c | 35 +++++++++++++++++++---------------- > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > This is rather big growth when we didn't already have realloc: > > 05: lib: Convert str_to_list() to use alist > > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > > function old new delta > > realloc - 1120 +1120 > > alist_ensure_ptr - 140 +140 > > alist_expand_to - 136 +136 > > alist_init - 108 +108 > > alist_uninit_move_ptr - 76 +76 > > alist_add_ptr - 72 +72 > > alist_uninit - 48 +48 > > str_to_list 204 232 +28 > > > > this is definitely not acceptable. This mini configuration is running out of > OCM and we are already pretty close to limit. > > What's the reason for this change? I can't see any explanation in commit message. It was more clearly explained in the cover thread for when Simon posted alist. This conversion was an example, so dropping it from the lmb rework series is fine.
On 7/25/24 16:49, Tom Rini wrote: > On Thu, Jul 25, 2024 at 08:13:38AM +0200, Michal Simek wrote: >> >> >> On 7/24/24 22:54, Tom Rini wrote: >>> On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: >>> >>>> From: Simon Glass <sjg@chromium.org> >>>> >>>> Use this new data structure in the utility function. >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> >>>> --- >>>> lib/strto.c | 35 +++++++++++++++++++---------------- >>>> 1 file changed, 19 insertions(+), 16 deletions(-) >>> >>> This is rather big growth when we didn't already have realloc: >>> 05: lib: Convert str_to_list() to use alist >>> aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 >>> xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 >>> u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) >>> function old new delta >>> realloc - 1120 +1120 >>> alist_ensure_ptr - 140 +140 >>> alist_expand_to - 136 +136 >>> alist_init - 108 +108 >>> alist_uninit_move_ptr - 76 +76 >>> alist_add_ptr - 72 +72 >>> alist_uninit - 48 +48 >>> str_to_list 204 232 +28 >>> >> >> this is definitely not acceptable. This mini configuration is running out of >> OCM and we are already pretty close to limit. >> >> What's the reason for this change? I can't see any explanation in commit message. > > It was more clearly explained in the cover thread for when Simon posted > alist. This conversion was an example, so dropping it from the lmb > rework series is fine. Perfect. We will also extend our configurations to have limit setup. Thanks, Michal
On Thu, 25 Jul 2024 at 18:24, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Thu, 25 Jul 2024 at 02:24, Tom Rini <trini@konsulko.com> wrote: > > > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > > > From: Simon Glass <sjg@chromium.org> > > > > > > Use this new data structure in the utility function. > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > lib/strto.c | 35 +++++++++++++++++++---------------- > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > This is rather big growth when we didn't already have realloc: > > 05: lib: Convert str_to_list() to use alist > > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > > function old new delta > > realloc - 1120 +1120 > > alist_ensure_ptr - 140 +140 > > alist_expand_to - 136 +136 > > alist_init - 108 +108 > > alist_uninit_move_ptr - 76 +76 > > alist_add_ptr - 72 +72 > > alist_uninit - 48 +48 > > str_to_list 204 232 +28 > > > > I am working on an implementation of lmb maps using lists. The list > nodes are then allocated with calloc, which I believe is included in > most of the board images. We can then compare the size impact with the > two implementations (alist vs list). Thanks. I have worked on implementing the LMB maps using simple lists, and we do get a good size benefit with the list based implementation. I have used the script that was shared by Tom some time back to get the size impact [1]. The readings are with 1) alists with realloc 2) alist with malloc and 3) using linked list. These are on a RPI4 build, which has LMB enabled. I have checked the impact on a xilinx_versal_mini config as well, which does not have LMB enabled, and the size impact result is best with lists. The list based changes can be checked on my github [2]. -sughosh [1] - https://gist.github.com/sughoshg/d4f9bda8d8a33f715dab892738d192ba [2] - https://github.com/sughoshg/u-boot/tree/lmb_only_linked_lists_v3
Hi Sughosh, On Sun, 28 Jul 2024 at 12:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Thu, 25 Jul 2024 at 18:24, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Thu, 25 Jul 2024 at 02:24, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > > > > > From: Simon Glass <sjg@chromium.org> > > > > > > > > Use this new data structure in the utility function. > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > lib/strto.c | 35 +++++++++++++++++++---------------- > > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > > > This is rather big growth when we didn't already have realloc: > > > 05: lib: Convert str_to_list() to use alist > > > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > > > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > > > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > > > function old new delta > > > realloc - 1120 +1120 > > > alist_ensure_ptr - 140 +140 > > > alist_expand_to - 136 +136 > > > alist_init - 108 +108 > > > alist_uninit_move_ptr - 76 +76 > > > alist_add_ptr - 72 +72 > > > alist_uninit - 48 +48 > > > str_to_list 204 232 +28 > > > > > > > I am working on an implementation of lmb maps using lists. The list > > nodes are then allocated with calloc, which I believe is included in > > most of the board images. We can then compare the size impact with the > > two implementations (alist vs list). Thanks. > > I have worked on implementing the LMB maps using simple lists, and we > do get a good size benefit with the list based implementation. I have > used the script that was shared by Tom some time back to get the size > impact [1]. The readings are with 1) alists with realloc 2) alist with > malloc and 3) using linked list. These are on a RPI4 build, which has > LMB enabled. I have checked the impact on a xilinx_versal_mini config > as well, which does not have LMB enabled, and the size impact result > is best with lists. The list based changes can be checked on my github > [2]. Thanks for doing that. It is quite hard to read with the LTO enabled. You can try with the -L flag and should get a cleaner result. Also see my comment about how you are using alist directly instead of going through the API...that might affect things. Anyway (once you are using the API), my assumption from my previous refactor attempt was that lmb would be easier to deal with with simple arrays, which I why I wrote alist. Is that true, or not? > > -sughosh > > [1] - https://gist.github.com/sughoshg/d4f9bda8d8a33f715dab892738d192ba > [2] - https://github.com/sughoshg/u-boot/tree/lmb_only_linked_lists_v3 Regards, Simon
On Mon, 29 Jul 2024 at 20:59, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Sun, 28 Jul 2024 at 12:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Thu, 25 Jul 2024 at 18:24, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Thu, 25 Jul 2024 at 02:24, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > > > > > > > From: Simon Glass <sjg@chromium.org> > > > > > > > > > > Use this new data structure in the utility function. > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > --- > > > > > lib/strto.c | 35 +++++++++++++++++++---------------- > > > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > > > > > This is rather big growth when we didn't already have realloc: > > > > 05: lib: Convert str_to_list() to use alist > > > > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > > > > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > > > > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > > > > function old new delta > > > > realloc - 1120 +1120 > > > > alist_ensure_ptr - 140 +140 > > > > alist_expand_to - 136 +136 > > > > alist_init - 108 +108 > > > > alist_uninit_move_ptr - 76 +76 > > > > alist_add_ptr - 72 +72 > > > > alist_uninit - 48 +48 > > > > str_to_list 204 232 +28 > > > > > > > > > > I am working on an implementation of lmb maps using lists. The list > > > nodes are then allocated with calloc, which I believe is included in > > > most of the board images. We can then compare the size impact with the > > > two implementations (alist vs list). Thanks. > > > > I have worked on implementing the LMB maps using simple lists, and we > > do get a good size benefit with the list based implementation. I have > > used the script that was shared by Tom some time back to get the size > > impact [1]. The readings are with 1) alists with realloc 2) alist with > > malloc and 3) using linked list. These are on a RPI4 build, which has > > LMB enabled. I have checked the impact on a xilinx_versal_mini config > > as well, which does not have LMB enabled, and the size impact result > > is best with lists. The list based changes can be checked on my github > > [2]. > > Thanks for doing that. It is quite hard to read with the LTO enabled. > You can try with the -L flag and should get a cleaner result. Also see > my comment about how you are using alist directly instead of going > through the API...that might affect things. I can post the results with the -L flag used. Regarding your comment on using the alist API's, I don't think that has a bearing on the size of the image. Using the alist_add() might be faster in certain scenarios, but I don't see how it impacts the size. > > Anyway (once you are using the API), my assumption from my previous > refactor attempt was that lmb would be easier to deal with with simple > arrays, which I why I wrote alist. Is that true, or not? Yes, using the arrays keeps the code on similar lines to it's current state. But I thought that the main consideration here was the size impact. -sughosh > > > > > -sughosh > > > > [1] - https://gist.github.com/sughoshg/d4f9bda8d8a33f715dab892738d192ba > > [2] - https://github.com/sughoshg/u-boot/tree/lmb_only_linked_lists_v3 > > Regards, > Simon
On Mon, Jul 29, 2024 at 09:28:57AM -0600, Simon Glass wrote: > Hi Sughosh, > > On Sun, 28 Jul 2024 at 12:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Thu, 25 Jul 2024 at 18:24, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Thu, 25 Jul 2024 at 02:24, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > > > > > > > From: Simon Glass <sjg@chromium.org> > > > > > > > > > > Use this new data structure in the utility function. > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > --- > > > > > lib/strto.c | 35 +++++++++++++++++++---------------- > > > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > > > > > This is rather big growth when we didn't already have realloc: > > > > 05: lib: Convert str_to_list() to use alist > > > > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > > > > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > > > > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > > > > function old new delta > > > > realloc - 1120 +1120 > > > > alist_ensure_ptr - 140 +140 > > > > alist_expand_to - 136 +136 > > > > alist_init - 108 +108 > > > > alist_uninit_move_ptr - 76 +76 > > > > alist_add_ptr - 72 +72 > > > > alist_uninit - 48 +48 > > > > str_to_list 204 232 +28 > > > > > > > > > > I am working on an implementation of lmb maps using lists. The list > > > nodes are then allocated with calloc, which I believe is included in > > > most of the board images. We can then compare the size impact with the > > > two implementations (alist vs list). Thanks. > > > > I have worked on implementing the LMB maps using simple lists, and we > > do get a good size benefit with the list based implementation. I have > > used the script that was shared by Tom some time back to get the size > > impact [1]. The readings are with 1) alists with realloc 2) alist with > > malloc and 3) using linked list. These are on a RPI4 build, which has > > LMB enabled. I have checked the impact on a xilinx_versal_mini config > > as well, which does not have LMB enabled, and the size impact result > > is best with lists. The list based changes can be checked on my github > > [2]. > > Thanks for doing that. It is quite hard to read with the LTO enabled. I don't think LTO is enabled there?
On Mon, 29 Jul 2024 at 23:46, Tom Rini <trini@konsulko.com> wrote: > > On Mon, Jul 29, 2024 at 09:28:57AM -0600, Simon Glass wrote: > > Hi Sughosh, > > > > On Sun, 28 Jul 2024 at 12:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Thu, 25 Jul 2024 at 18:24, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > On Thu, 25 Jul 2024 at 02:24, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > > > > > > > > > From: Simon Glass <sjg@chromium.org> > > > > > > > > > > > > Use this new data structure in the utility function. > > > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > --- > > > > > > lib/strto.c | 35 +++++++++++++++++++---------------- > > > > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > > > > > > > This is rather big growth when we didn't already have realloc: > > > > > 05: lib: Convert str_to_list() to use alist > > > > > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > > > > > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > > > > > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > > > > > function old new delta > > > > > realloc - 1120 +1120 > > > > > alist_ensure_ptr - 140 +140 > > > > > alist_expand_to - 136 +136 > > > > > alist_init - 108 +108 > > > > > alist_uninit_move_ptr - 76 +76 > > > > > alist_add_ptr - 72 +72 > > > > > alist_uninit - 48 +48 > > > > > str_to_list 204 232 +28 > > > > > > > > > > > > > I am working on an implementation of lmb maps using lists. The list > > > > nodes are then allocated with calloc, which I believe is included in > > > > most of the board images. We can then compare the size impact with the > > > > two implementations (alist vs list). Thanks. > > > > > > I have worked on implementing the LMB maps using simple lists, and we > > > do get a good size benefit with the list based implementation. I have > > > used the script that was shared by Tom some time back to get the size > > > impact [1]. The readings are with 1) alists with realloc 2) alist with > > > malloc and 3) using linked list. These are on a RPI4 build, which has > > > LMB enabled. I have checked the impact on a xilinx_versal_mini config > > > as well, which does not have LMB enabled, and the size impact result > > > is best with lists. The list based changes can be checked on my github > > > [2]. > > > > Thanks for doing that. It is quite hard to read with the LTO enabled. > > I don't think LTO is enabled there? I ran the size checks with -L flag [1]. Do not see any difference. -sughosh [1] - https://gist.github.com/sughoshg/8a82946727904944601021a7ee732661
Hi Sughosh, On Mon, 29 Jul 2024 at 19:49, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Mon, 29 Jul 2024 at 23:46, Tom Rini <trini@konsulko.com> wrote: > > > > On Mon, Jul 29, 2024 at 09:28:57AM -0600, Simon Glass wrote: > > > Hi Sughosh, > > > > > > On Sun, 28 Jul 2024 at 12:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > On Thu, 25 Jul 2024 at 18:24, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > On Thu, 25 Jul 2024 at 02:24, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > > > On Wed, Jul 24, 2024 at 11:31:48AM +0530, Sughosh Ganu wrote: > > > > > > > > > > > > > From: Simon Glass <sjg@chromium.org> > > > > > > > > > > > > > > Use this new data structure in the utility function. > > > > > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > > --- > > > > > > > lib/strto.c | 35 +++++++++++++++++++---------------- > > > > > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > > > > > > > > > This is rather big growth when we didn't already have realloc: > > > > > > 05: lib: Convert str_to_list() to use alist > > > > > > aarch64: (for 1/1 boards) all +1765.0 rodata +37.0 text +1728.0 > > > > > > xilinx_versal_mini_emmc0: all +1765 rodata +37 text +1728 > > > > > > u-boot: add: 7/0, grow: 1/0 bytes: 1728/0 (1728) > > > > > > function old new delta > > > > > > realloc - 1120 +1120 > > > > > > alist_ensure_ptr - 140 +140 > > > > > > alist_expand_to - 136 +136 > > > > > > alist_init - 108 +108 > > > > > > alist_uninit_move_ptr - 76 +76 > > > > > > alist_add_ptr - 72 +72 > > > > > > alist_uninit - 48 +48 > > > > > > str_to_list 204 232 +28 > > > > > > > > > > > > > > > > I am working on an implementation of lmb maps using lists. The list > > > > > nodes are then allocated with calloc, which I believe is included in > > > > > most of the board images. We can then compare the size impact with the > > > > > two implementations (alist vs list). Thanks. > > > > > > > > I have worked on implementing the LMB maps using simple lists, and we > > > > do get a good size benefit with the list based implementation. I have > > > > used the script that was shared by Tom some time back to get the size > > > > impact [1]. The readings are with 1) alists with realloc 2) alist with > > > > malloc and 3) using linked list. These are on a RPI4 build, which has > > > > LMB enabled. I have checked the impact on a xilinx_versal_mini config > > > > as well, which does not have LMB enabled, and the size impact result > > > > is best with lists. The list based changes can be checked on my github > > > > [2]. > > > > > > Thanks for doing that. It is quite hard to read with the LTO enabled. > > > > I don't think LTO is enabled there? > > I ran the size checks with -L flag [1]. Do not see any difference.\ OK, thanks, as Tom says probably not actually enabled. It's just hard to compare two branches! I took a look at the lmb thing and decided to write a few patches to show what I am getting at, particularly with the testing side. I will send those as an RFC... Regards, Simon > > -sughosh > > [1] - https://gist.github.com/sughoshg/8a82946727904944601021a7ee732661
diff --git a/lib/strto.c b/lib/strto.c index f83ac67c66..277e83b20f 100644 --- a/lib/strto.c +++ b/lib/strto.c @@ -9,6 +9,7 @@ * Wirzenius wrote this portably, Torvalds fucked it up :-) */ +#include <alist.h> #include <errno.h> #include <malloc.h> #include <vsprintf.h> @@ -226,37 +227,39 @@ void str_to_upper(const char *in, char *out, size_t len) const char **str_to_list(const char *instr) { - const char **ptr; - char *str, *p; - int count, i; + struct alist alist; + char *str, *p, *start; /* don't allocate if the string is empty */ str = *instr ? strdup(instr) : (char *)instr; if (!str) return NULL; - /* count the number of space-separated strings */ - for (count = 0, p = str; *p; p++) { + alist_init_struct(&alist, char *); + + if (*str) + alist_add(&alist, str, char *); + for (start = str, p = str; *p; p++) { if (*p == ' ') { - count++; *p = '\0'; + start = p + 1; + if (*start) + alist_add(&alist, start, char *); } } - if (p != str && p[-1]) - count++; - /* allocate the pointer array, allowing for a NULL terminator */ - ptr = calloc(count + 1, sizeof(char *)); - if (!ptr) { - if (*str) + /* terminate list */ + p = NULL; + alist_add(&alist, p, char *); + if (alist_err(&alist)) { + alist_uninit(&alist); + + if (*instr) free(str); return NULL; } - for (i = 0, p = str; i < count; p += strlen(p) + 1, i++) - ptr[i] = p; - - return ptr; + return alist_uninit_move(&alist, NULL, const char *); } void str_free_list(const char **ptr)