[PATCH] kernel/resource: Increment by align value in get_free_mem_region()

[PATCH] kernel/resource: Increment by align value in get_free_mem_region()

[alison.schofield]

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>
---

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
-- 
2.37.3

Re: [PATCH] kernel/resource: Increment by align value in get_free_mem_region()

[Dave Jiang]

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

RE: [PATCH] kernel/resource: Increment by align value in get_free_mem_region()

[Dan Williams]

[ 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
> 

Re: [PATCH] kernel/resource: Increment by align value in get_free_mem_region()

[Christoph Hellwig]

Looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>