| Message ID | 20231113221324.1118092-1-alison.schofield@intel.com |
|---|---|
| State | Accepted |
| Commit | 659aa050a53817157b7459529538598a6449c1d3 |
| Headers | show |
| Series | kernel/resource: Increment by align value in get_free_mem_region() | expand |
On 11/13/23 15:13, alison.schofield@intel.com wrote: > From: Alison Schofield <alison.schofield@intel.com> > > Currently get_free_mem_region() searches for available capacity > in increments equal to the region size being requested. This can > cause the search to take giant steps through the resource leaving > needless gaps and missing available space. > > Replace the size increment with an alignment increment so that the > next possible address is always examined for availability. > > Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()") > Reported-by: Dmytro Adamenko <dmytro.adamenko@intel.com> > Reported-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Alison Schofield <alison.schofield@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > > A couple of below the line items: > > The MAINTAINERS file and get_maintainers script did not emit a clear > recipient list for this one. Start with CXL folks and I can expand > it in a v2 with your help. > > I considered, but didn't, change the parameter naming in gfr_continue(), > gfr_next(). It's a choice as get_free_mem_region() is the only caller. > Thoughts? > > > kernel/resource.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/resource.c b/kernel/resource.c > index 866ef3663a0b..91be1bc50b60 100644 > --- a/kernel/resource.c > +++ b/kernel/resource.c > @@ -1844,8 +1844,8 @@ get_free_mem_region(struct device *dev, struct resource *base, > > write_lock(&resource_lock); > for (addr = gfr_start(base, size, align, flags); > - gfr_continue(base, addr, size, flags); > - addr = gfr_next(addr, size, flags)) { > + gfr_continue(base, addr, align, flags); > + addr = gfr_next(addr, align, flags)) { > if (__region_intersects(base, addr, size, 0, IORES_DESC_NONE) != > REGION_DISJOINT) > continue; > > base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
[ add Jason and Christoph for request_free_mem_region() impact ] alison.schofield@ wrote: > From: Alison Schofield <alison.schofield@intel.com> > > Currently get_free_mem_region() searches for available capacity > in increments equal to the region size being requested. This can > cause the search to take giant steps through the resource leaving > needless gaps and missing available space. The bug addressed by this patch is less likely to bite with request_free_mem_region() compared to alloc_free_mem_region() since the former does a descending search through the address space that is unlikely to collide with an existing allocation. Patch looks good to me, but I would clarify what a CXL end user would see, so I'll append the following on applying: "Specifically 'cxl create-region' fails with ERANGE even though capacity of the given size and CXL's expected 256M x InterleaveWays can be satisfied". > Replace the size increment with an alignment increment so that the > next possible address is always examined for availability. > > Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()") > Reported-by: Dmytro Adamenko <dmytro.adamenko@intel.com> > Reported-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > --- > > A couple of below the line items: > > The MAINTAINERS file and get_maintainers script did not emit a clear > recipient list for this one. Start with CXL folks and I can expand > it in a v2 with your help. For this it is sufficient to Cc the other users of get_free_mem_region() which I did above. > I considered, but didn't, change the parameter naming in gfr_continue(), > gfr_next(). It's a choice as get_free_mem_region() is the only caller. > Thoughts? Looks fine to me the @size parameter for those helpers should always be the step value which is @align from the parent routine. > > > kernel/resource.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/resource.c b/kernel/resource.c > index 866ef3663a0b..91be1bc50b60 100644 > --- a/kernel/resource.c > +++ b/kernel/resource.c > @@ -1844,8 +1844,8 @@ get_free_mem_region(struct device *dev, struct resource *base, > > write_lock(&resource_lock); > for (addr = gfr_start(base, size, align, flags); > - gfr_continue(base, addr, size, flags); > - addr = gfr_next(addr, size, flags)) { > + gfr_continue(base, addr, align, flags); > + addr = gfr_next(addr, align, flags)) { > if (__region_intersects(base, addr, size, 0, IORES_DESC_NONE) != > REGION_DISJOINT) > continue; > > base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86 > -- > 2.37.3 >
Looks good to me:
Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/kernel/resource.c b/kernel/resource.c index 866ef3663a0b..91be1bc50b60 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -1844,8 +1844,8 @@ get_free_mem_region(struct device *dev, struct resource *base, write_lock(&resource_lock); for (addr = gfr_start(base, size, align, flags); - gfr_continue(base, addr, size, flags); - addr = gfr_next(addr, size, flags)) { + gfr_continue(base, addr, align, flags); + addr = gfr_next(addr, align, flags)) { if (__region_intersects(base, addr, size, 0, IORES_DESC_NONE) != REGION_DISJOINT) continue;