[PATCH 23/40] lmb: add a flags parameter to the API's
Simon Glass
sjg at chromium.org
Fri Aug 9 17:58:11 CEST 2024
Hi Sughosh,
On Fri, 9 Aug 2024 at 02:25, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> On Thu, 8 Aug 2024 at 19:58, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Wed, 7 Aug 2024 at 00:32, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > On Wed, 7 Aug 2024 at 03:21, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Mon, 5 Aug 2024 at 05:55, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > On Mon, 29 Jul 2024 at 20:56, Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Mon, 29 Jul 2024 at 02:40, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > >
> > > > > > > On Fri, 26 Jul 2024 at 05:02, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Sughosh,
> > > > > > > >
> > > > > > > > On Wed, 24 Jul 2024 at 00:04, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > Add a flags parameter to the LMB API functions. The parameter can then
> > > > > > > > > be used to pass any other type of reservations or allocations needed
> > > > > > > > > by the callers. These will be used in a subsequent set of changes for
> > > > > > > > > allocation requests coming from the EFI subsystem.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > > > > > ---
> > > > > > > > > Changes since rfc: New patch
> > > > > > > > >
> > > > > > > > > arch/arm/mach-apple/board.c | 17 ++--
> > > > > > > > > arch/arm/mach-snapdragon/board.c | 2 +-
> > > > > > > > > arch/arm/mach-stm32mp/dram_init.c | 4 +-
> > > > > > > > > arch/powerpc/cpu/mpc85xx/mp.c | 2 +-
> > > > > > > > > arch/powerpc/lib/bootm.c | 2 +-
> > > > > > > > > board/xilinx/common/board.c | 4 +-
> > > > > > > > > boot/bootm.c | 5 +-
> > > > > > > > > boot/image-board.c | 15 ++-
> > > > > > > > > boot/image-fdt.c | 15 +--
> > > > > > > > > cmd/booti.c | 2 +-
> > > > > > > > > cmd/bootz.c | 2 +-
> > > > > > > > > cmd/load.c | 4 +-
> > > > > > > > > drivers/iommu/apple_dart.c | 6 +-
> > > > > > > > > drivers/iommu/sandbox_iommu.c | 6 +-
> > > > > > > > > fs/fs.c | 2 +-
> > > > > > > > > include/lmb.h | 23 ++---
> > > > > > > > > lib/lmb.c | 48 ++++------
> > > > > > > > > test/lib/lmb.c | 150 +++++++++++++++---------------
> > > > > > > > > 18 files changed, 150 insertions(+), 159 deletions(-)
> > > > > > > >
> > > > > > > > This negates any code-size advantage of dropping the lmb parameter.
> > > > > > > >
> > > > > > > > All of these are LMB_NONE. Can we have a separate function (e.g.
> > > > > > > > lmb_alloc_type()) for when we actually need to specify the type?
> > > > > > >
> > > > > > > We will be passing different values when we call the LMB API's from
> > > > > > > the EFI allocation function. This is only adding a parameter to the
> > > > > > > allocation API's, which I believe is better than adding separate
> > > > > > > functions which take a flag parameter only to be called from the EFI
> > > > > > > subsystem.
> > > > > >
> > > > > > No i believe it is worse, unless there are a lot of such functions.
> > > > > > The flags are a special case, not the common case.
> > > > >
> > > > > I have done some size impact tests on the two scenarios, one where we
> > > > > have a common set of lmb allocation API functions, with an added flags
> > > > > parameter, and second where we have separate API's to be called from
> > > > > the EFI memory module. I have put out the results of the size impact
> > > > > [1].
> > > > >
> > > > > You will see that with common API's, we are not losing much even on
> > > > > boards with EFI_LOADER disabled. But otoh, on boards which have
> > > > > EFI_LOADER enabled, the gains are pretty significant. I believe we
> > > > > should reconsider using a common LMB API with the flags parameter.
> > > >
> > > > Thanks for looking at it.
> > > >
> > > > Did you do special versions of just lmb_alloc() and lmb_add() which
> > > > call the flags versions? It seems that there is no size advantage with
> > > > EFI_LOADER and only a small one with !EFI_LOADER. Can you please point
> > > > me to the code?
> > >
> > > For the separate API version, I introduced new versions
> > > lmb_alloc_flags(), lmb_alloc_base_flags(), lmb_alloc_addr_flags() and
> > > lmb_free_flags(), which are being called from the EFI memory module. I
> > > have pushed the two branches [1] [2] on my github. Please take a look.
> > >
> > > Btw, both these branches are based on your v5 of the alist patches,
> > > and also incorporate the stack based implementation for running the
> > > lmb tests. So except for either having common API's, or not, there are
> > > no other differences between the two. Thanks.
> >
> > Thanks for the info.
> >
> > The non-flags functions can call the flags functions, so that you
> > don't create a new code path. Something like this:
> >
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index 726e6c38227..0a251c587fe 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -528,7 +528,7 @@ long lmb_free_flags(phys_addr_t base, phys_size_t size,
> >
> > long lmb_free(phys_addr_t base, phys_size_t size)
> > {
> > - return __lmb_free(base, size);
> > + return lmb_free_flags(base, size, LMB_NONE);
> > }
> >
> > long lmb_reserve_flags(phys_addr_t base, phys_size_t size, enum
> > lmb_flags flags)
> > @@ -624,7 +624,7 @@ static phys_addr_t __lmb_alloc_base(phys_size_t
> > size, ulong align,
> >
> > phys_addr_t lmb_alloc(phys_size_t size, ulong align)
> > {
> > - return lmb_alloc_base(size, align, LMB_ALLOC_ANYWHERE);
> > + return lmb_alloc_flags(size, align, LMB_NONE);
> > }
> >
> > phys_addr_t lmb_alloc_flags(phys_size_t size, ulong align, uint flags)
> > @@ -635,15 +635,7 @@ phys_addr_t lmb_alloc_flags(phys_size_t size,
> > ulong align, uint flags)
> >
> > phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr)
> > {
> > - phys_addr_t alloc;
> > -
> > - alloc = __lmb_alloc_base(size, align, max_addr, LMB_NONE);
> > -
> > - if (alloc == 0)
> > - printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n",
> > - (ulong)size, (ulong)max_addr);
> > -
> > - return alloc;
> > + return lmb_alloc_base_flags(size, align, max_addr, LMB_NONE);
> > }
> >
> > phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align,
> > @@ -691,7 +683,7 @@ static phys_addr_t __lmb_alloc_addr(phys_addr_t
> > base, phys_size_t size,
> > */
> > phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size)
> > {
> > - return __lmb_alloc_addr(base, size, LMB_NONE);
> > + return lmb_alloc_addr_flags(base, size, LMB_NONE);
> > }
> >
> > phys_addr_t lmb_alloc_addr_flags(phys_addr_t base, phys_size_t size,
> >
> > But it only saves about 40 bytes on Thumb2. You can save another 16 by
> > using the placeholder API:
>
> Can you please explain the issue that you see with having a common set
> of API's with the flags parameter added? The way I see it, the API's
> are already undergoing a change where we are removing the struct lmb
> instance as a parameter from all the API functions. With the change
> that I propose, we are simply replacing the lmb instance parameter
> with the flags parameter. So arguably we are not adding any additional
> size here. Also, like the size tests show, we get a pretty good size
> benefit when the EFI_LOADER is enabled.
>
> So, if your argument is to keep the API's similar to their earlier
> form, I think that they are undergoing a change in any case, so adding
> the flags parameter is not so much of an issue. If there is any
> problem that I am missing, I would like to understand that before we
> go with separate API's. Thanks.
I thought I explained this already, but perhaps not. We change APIs
all the time, so that is not a problem.
Almost all calls don't need to pass flags since it is LMB_NONE (which
really should be 0, BTW, not BIT(0)). We already have
lmb_reserve_flags() and lmb_reserve() to deal with this difference. So
passing a parameter which is almost always the same is not helpful for
code size.
Outside the tests, only one place (boot/image-fdt.c) uses
lmb_reserve_flags() - BTW some of these are using the flags version of
the function but passing LMB_NONE.
Here are the five functions:
lmb_reserve
lmb_alloc
lmb_alloc_base
lmb_alloc_addr
lmb_free
If some of them aren't worth having two versions, then I suppose we
don't need them all. But note that if some of my EFI patches make it
through code review, we won't need this circular relationship between
EFI and lmb, so some of the 'flags' versions can be dropped again.
But EFI only even seems to pass LMB_NOOVERWRITE | LMB_NONOTIFY...?
Part of my frustration with all of this is that I created an lmb
cleanup series[1] nearly a year ago, which was either ignored or
blocked (I'm not sure which). That series tidied up the code quite a
lot and took much effort. I'm not sure if you even saw it?
Finally, it would help the project if you could do some code reviews.
For example, how about [2] and [3] - they are very much in your area.
Regards,
Simon
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=371258&state=*
[2] https://patchwork.ozlabs.org/project/uboot/list/?series=417669
[3] https://patchwork.ozlabs.org/project/uboot/list/?series=418212
>
> -sughosh
>
> >
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index 0a251c587fe..f6c8f06629c 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -416,13 +416,12 @@ static long lmb_add_region_flags(struct alist
> > *lmb_rgn_lst, phys_addr_t base,
> > if (coalesced)
> > return coalesced;
> >
> > - if (alist_full(lmb_rgn_lst) &&
> > - !alist_expand_by(lmb_rgn_lst, lmb_rgn_lst->alloc))
> > + if (!alist_add_placeholder(lmb_rgn_lst))
> > return -1;
> > rgn = lmb_rgn_lst->data;
> >
> > /* Couldn't coalesce the LMB, so add it to the sorted table. */
> > - for (i = lmb_rgn_lst->count; i >= 0; i--) {
> > + for (i = lmb_rgn_lst->count - 1; i >= 0; i--) {
> > if (i && base < rgn[i - 1].base) {
> > rgn[i] = rgn[i - 1];
> > } else {
> > @@ -433,8 +432,6 @@ static long lmb_add_region_flags(struct alist
> > *lmb_rgn_lst, phys_addr_t base,
> > }
> > }
> >
> > - lmb_rgn_lst->count++;
> > -
> > return 0;
> > }
> >
> > @@ -444,7 +441,6 @@ static long lmb_add_region(struct alist
> > *lmb_rgn_lst, phys_addr_t base,
> > return lmb_add_region_flags(lmb_rgn_lst, base, size, LMB_NONE);
> > }
> >
> > -/* This routine may be called with relocation disabled. */
> > long lmb_add(phys_addr_t base, phys_size_t size)
> > {
> > long ret;
> >
> > --
> > 2.34.1
> >
> > (patches are a bit rough, but I didn't think it worth sending them to
> > the ML as real patches)
> >
> > If I am correct and we don't need to publish events, then that will
> > save a little more space.
> >
> > Regards,
> > Simon
> >
> > >
> > > -sughosh
> > >
> > > [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_common_apis_nrfc_v2
> > > [2] - https://github.com/sughoshg/u-boot/tree/lmb_efi_separate_flags_apis_nrfc_v2
> > >
> > > >
> > > > Regards,
> > > > Simon
> > > >
> > > >
> > > > >
> > > > > [1] - https://gist.github.com/sughoshg/a20207f26e19238fef86f710134d6efd
> > > > >
> > > > > >
> > > > > > Regards,
> > > > > > SImon
More information about the U-Boot
mailing list