[PATCH 2/4] lmb: handle scenario of of encompassing overlap

[Sughosh Ganu]

On Mon, 17 Feb 2025 at 20:51, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> Hi Sughosh,
>
> On 2/13/25 2:11 PM, Sughosh Ganu wrote:
> > The lmb_fix_over_lap_regions() function is called if the added region
> > overlaps with an existing region. The function then fixes the overlap
> > and removes the redundant region. However, it makes an assumption that
> > the overlap would not encompass the existing region, and in such a
> > scenario, it prints a message and returns without making the
> > fix. Handle the case of an encompassing overlap also in the function.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > Reported-by: Quentin Schulz <quentin.schulz at cherry.de>
> > ---
> >
> > Note: To be applied after an A-b/R-b/T-b from the original author
> > of the lmb_fix_over_lap_regions() function on this
> >
> >   lib/lmb.c | 8 +++-----
> >   1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index aeaf120f57d..a5216bdccc7 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -111,11 +111,9 @@ static void lmb_fix_over_lap_regions(struct alist *lmb_rgn_lst,
> >       phys_addr_t base2 = rgn[r2].base;
> >       phys_size_t size2 = rgn[r2].size;
> >
> > -     if (base1 + size1 > base2 + size2) {
> > -             printf("This will not be a case any time\n");
> > -             return;
> > -     }
> > -     rgn[r1].size = base2 + size2 - base1;
> > +     if (base1 + size1 < base2 + size2)
> > +             rgn[r1].size = base2 + size2 - base1;
> > +
> >       lmb_remove_region(lmb_rgn_lst, r2);
>
> Mm I have a feeling this function was improperly named, this is
> essentially just merging two regions that are overlapping. Similarly to
> lmb_coalesce_regions except it handles overlaps too.
>
> I'm wondering if we shouldn't simply merge lmb_regions_adjacent and
> lmb_regions_overlap, and lmb_coalesce_regions and
> lmb_fix_over_lap_regions into one function each? e.g.
> lmb_regions_mergeable() and lmb_merge_regions()?

I will go through these functions and see what can be merged into a
single function. On these lines, for v2, I have made a change where
instead of calling lmb_fix_overlap_regions(), the same goal is being
achieved through lmb_resize_regions(). The current implementation of
lmb_fix_overlap_regions() only checks if the adjoining region is
overlapping, but there could be a scenario where more than one
existing regions are overlapping with what is being added. That will
get handled by calling lmb_resize_regions(). I am in the middle of
testing these changes, and will send the next version in a day or so.

>
> For what it's worth, I was able to trigger this printf with a pxe load
> at an address 0xb0b and have the kernel_addr_r at 0xb00 for example.
> What I don't understand is why
>
> 1) aside from the printf, it still booted seemingly properly
> 2) why interrupting the transfer and starting it again would not make
> those messages appear anymore

Although I have not tested this scenario, I suspect what happens is
that the region gets added during the first run, when it prints the
message. Then when the same command is run subsequently, the region
has already been added, so the condition in lmb_fix_overlap_regions()
is not hit, so no print. In any case, print or no print, the function
should be addressing the case of an encompassing overlap as well. But
like I mentioned above, a more generic fix for that would be a call to
lmb_resize_regions().

-sughosh

>
> Logically though, this patch seems to be ok if the purpose is to merge
> two regions.
>
> Thanks for looking into this,
> Quentin