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

[Dan Williams <dan.j.williams@intel.com>]

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