Re: [PATCH 3/3] cma: move allocation from HIGHMEM to a helper function

[Oscar Salvador <osalvador@suse.de>]

On Wed, Jul 02, 2025 at 08:36:05PM +0300, Mike Rapoport wrote:
> From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> 
> When CONFIG_HIGMEM is enabled, __cma_declare_contiguous_nid() first
> tries to allocate the area from HIGHMEM and if that fails it falls back
> to allocation from low memory.
> 
> Split allocation from HIGMEM into a helper function to further decouple
> logic related to CONFIG_HIGHMEM.
> 
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> ---
>  mm/cma.c | 52 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/mm/cma.c b/mm/cma.c
> index 1df8ff312d99..0a24c46f3296 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -376,6 +376,30 @@ static int __init cma_fixed_reserve(phys_addr_t base, phys_addr_t size)
>  	return 0;
>  }
>  
> +static phys_addr_t __init cma_alloc_highmem(phys_addr_t base, phys_addr_t size,
> +			phys_addr_t align, phys_addr_t *limit, int nid)
> +{
> +	phys_addr_t addr = 0;
> +
> +	if (IS_ENABLED(CONFIG_HIGHMEM)) {
> +		phys_addr_t highmem = __pa(high_memory - 1) + 1;
> +
> +		/*
> +		 * All pages in the reserved area must come from the same zone.
> +		 * If the requested region crosses the low/high memory boundary,
> +		 * try allocating from high memory first and fall back to low
> +		 * memory in case of failure.
> +		 */
> +		if (base < highmem && *limit > highmem) {
> +			addr = memblock_alloc_range_nid(size, align, highmem,
> +							*limit, nid, true);
> +			*limit = highmem;
> +		}
> +	}

Not a big deal, but maybe better to do it in one function? Maybe even move
the CONFIG_PHYS_ADDR_T_64BIT block in there as well? So memblock_alloc_range_nid()
calls would be contained in one place and the X86_64/HIGHMEM comments as
well.
Just a thought.

 diff --git a/mm/cma.c b/mm/cma.c
 index dd7643fc01db..532b56e6971a 100644
 --- a/mm/cma.c
 +++ b/mm/cma.c
 @@ -377,11 +377,12 @@ static int __init cma_fixed_reserve(phys_addr_t base, phys_addr_t size)
  	return 0;
  }
 
 -static phys_addr_t __init cma_alloc_highmem(phys_addr_t base, phys_addr_t size,
 -			phys_addr_t align, phys_addr_t *limit, int nid)
 +static phys_addr_t __init cma_alloc_mem(phys_addr_t base, phys_addr_t size,
 +			phys_addr_t align, phys_addr_t limit, int nid)
  {
  	phys_addr_t addr = 0;
 
 +	/* On systems with HIGHMEM try allocating from there first */
  	if (IS_ENABLED(CONFIG_HIGHMEM)) {
  		phys_addr_t highmem = __pa(high_memory - 1) + 1;
 
 @@ -393,11 +394,15 @@ static phys_addr_t __init cma_alloc_highmem(phys_addr_t base, phys_addr_t size,
  		 */
  		if (base < highmem && *limit > highmem) {
  			addr = memblock_alloc_range_nid(size, align, highmem,
 -							*limit, nid, true);
 +							limit, nid, true);
  			*limit = highmem;
  		}
  	}
 
 +	if (!addr)
 +		addr = memblock_alloc_range_nid(size, alignment, base,
 +						limit, nid, true);
 +
  	return addr;
  }
 
 @@ -487,16 +492,8 @@ static int __init __cma_declare_contiguous_nid(phys_addr_t *basep,
  		}
  #endif
 
 -		/* On systems with HIGHMEM try allocating from there first */
  		if (!addr)
 -			addr = cma_alloc_highmem(base, size, alignment, &limit,
 -						 nid);
 -		if (!addr) {
 -			addr = memblock_alloc_range_nid(size, alignment, base,
 -					limit, nid, true);
 -			if (!addr)
 -				return -ENOMEM;
 -		}
 +			addr = cma_alloc_mem(base, size, alignment, limit, nid);
 
  		/*
  		 * kmemleak scans/reads tracked objects for pointers to other



-- 
Oscar Salvador
SUSE Labs