| Message ID | 20250213131104.186663-4-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_add_region_flags() first checks if the new region to be added > can be coalesced with existing regions. The check stops if the two > regions are adjecent but their flags do not match. However, it is > possible that the newly added region might be adjacent with the next > existing region and with matching flags. Check for this possibility by > not breaking out of the loop. > What do you mean exactly? I know nothing about LMB, but I assume a region can only ever exist once and with one set of flags? This means a region can only ever have two adjacent regions, before itself and after itself. It doesn't make sense to continue iterating if the adjacent region that has a different flag is after the region to be created since we won't find a new one, so we could break if base1 < base2 for ret > 0 below? What do you think? Cheers, Quentin
On Mon, 17 Feb 2025 at 20:59, Quentin Schulz <quentin.schulz@cherry.de> wrote: > > Hi Sughosh, > > On 2/13/25 2:11 PM, Sughosh Ganu wrote: > > The lmb_add_region_flags() first checks if the new region to be added > > can be coalesced with existing regions. The check stops if the two > > regions are adjecent but their flags do not match. However, it is > > possible that the newly added region might be adjacent with the next > > existing region and with matching flags. Check for this possibility by > > not breaking out of the loop. > > > > What do you mean exactly? > > I know nothing about LMB, but I assume a region can only ever exist once > and with one set of flags? This means a region can only ever have two > adjacent regions, before itself and after itself. There could be two regions r1 and r2 (r2 > r1) with a hole in between [1]. And this is a case of adding a new region r3 where r3 is adjacent to both r1 and r2, and where the flags of r3 and r2 are matching, but r1 and r3 are not. The current logic will not allow merging of r2 and r3 into a single region. > > It doesn't make sense to continue iterating if the adjacent region that > has a different flag is after the region to be created since we won't > find a new one, so we could break if base1 < base2 for ret > 0 below? You are right in that the change is only needed for the scenario where the added region is after an existing region. So we can indeed break for the case of ret > 0. I will update this in the next version. -sughosh > > What do you think? [1] - https://paste.ack.tf/351fc7 > > Cheers, > Quentin
diff --git a/lib/lmb.c b/lib/lmb.c index a5216bdccc7..a55bfe289db 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -204,14 +204,14 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); if (ret > 0) { if (flags != rgnflags) - break; + continue; rgn[i].base -= size; rgn[i].size += size; coalesced++; break; } else if (ret < 0) { if (flags != rgnflags) - break; + continue; rgn[i].size += size; coalesced++; break;
The lmb_add_region_flags() first checks if the new region to be added can be coalesced with existing regions. The check stops if the two regions are adjecent but their flags do not match. However, it is possible that the newly added region might be adjacent with the next existing region and with matching flags. Check for this possibility by not breaking out of the loop. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- lib/lmb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)