| Message ID | 20250213131104.186663-3-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | lmb: miscellaneous fixes and improvements | expand |
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@linaro.org> > Reported-by: Quentin Schulz <quentin.schulz@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()? 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 Logically though, this patch seems to be ok if the purpose is to merge two regions. Thanks for looking into this, Quentin
On Mon, 17 Feb 2025 at 20:51, Quentin Schulz <quentin.schulz@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@linaro.org> > > Reported-by: Quentin Schulz <quentin.schulz@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
Hi Sughosh, On Thu, 13 Feb 2025 at 06:11, Sughosh Ganu <sughosh.ganu@linaro.org> 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@linaro.org> > Reported-by: Quentin Schulz <quentin.schulz@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(-) Would it be possible to go back through your dozen-or-so lmb patches and add some tests? Regards, Simon
On Tue, 18 Feb 2025 at 17:08, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Thu, 13 Feb 2025 at 06:11, Sughosh Ganu <sughosh.ganu@linaro.org> 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@linaro.org> > > Reported-by: Quentin Schulz <quentin.schulz@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(-) > > Would it be possible to go back through your dozen-or-so lmb patches > and add some tests? I am adding tests in the first patch of this series [1], where I am fixing the lmb_reserve() function. In certain scenarios, it allows to reserve memory regions on top of existing reservations. Please check that patch and suggest additional test cases you think that might be missing. Thanks. -sughosh [1] - https://lore.kernel.org/u-boot/20250213131104.186663-1-sughosh.ganu@linaro.org/T/#m64b8622d1de3973955a9b0239de1f1304bc6d831 > > Regards, > Simon
Hi Sughosh, On Tue, 18 Feb 2025 at 04:57, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Tue, 18 Feb 2025 at 17:08, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Thu, 13 Feb 2025 at 06:11, Sughosh Ganu <sughosh.ganu@linaro.org> 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@linaro.org> > > > Reported-by: Quentin Schulz <quentin.schulz@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(-) > > > > Would it be possible to go back through your dozen-or-so lmb patches > > and add some tests? > > I am adding tests in the first patch of this series [1], where I am > fixing the lmb_reserve() function. In certain scenarios, it allows to > reserve memory regions on top of existing reservations. Please check > that patch and suggest additional test cases you think that might be > missing. Thanks. OK, good, thanks. I'll leave it to you to figure out what is needed. Ideally you would add a test to cover each problem you fix. > > -sughosh > > [1] - https://lore.kernel.org/u-boot/20250213131104.186663-1-sughosh.ganu@linaro.org/T/#m64b8622d1de3973955a9b0239de1f1304bc6d831 > Regards, Simon
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); }
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@linaro.org> Reported-by: Quentin Schulz <quentin.schulz@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(-)