Re: [PATCH] mm/cma: simplify cma_declare_contiguous_multi()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 11/6/26 13:56, Mike Rapoport (Microsoft) wrote:
> When cma_declare_contiguous_multi() cannot allocate a CMA area as a
> single range, it falls back to collecting a list of up to CMA_MAX_RANGES
> largest free ranges above 4G, reserves them in memblock and adds them to
> cma->ranges.
> 
> The list and subsequently cma->ranges are supposed to be sorted in the
> order of base addresses of the "allocated" ranges.
> 
> This implementation has two bugs:
> 
> * The allocation may fail even when there is enough free memory because
>   list_insert_sorted() sorts the list in the wrong order.
> 
>   When the list is full, the largest range is evicted rather than the
>   smallest, the final list is not sorted by address, and the smallest
>   ranges are used first.
> 
> * the function may report success while it reserved less memory than
>   requested because sizesum accounts for ranges that were skipped.
> 
> And overall it is a lot of list management and open coded range
> selection for no real gain.
> 
> Replace the current code with a loop that finds the largest free range
> above 4G, reserves as much of it as needed and repeats until the entire
> area is reserved or CMA_MAX_RANGES ranges are used.
> 
> After successful alocation sort the cma->ranges by their base address.
> 
> The new code scans free memory once for every range rather than once
> for all ranges, so it is slightly slower, but it is way simpler and more
> robust.
> 
> Fixes: c009da4258f9 ("mm, cma: support multiple contiguous ranges, if requested")
> Reported-by: Ivy Lopez <skunkolee@gmail.com>
> Closes: https://lore.kernel.org/all/cover.1792032699.git.skunkolee@gmail.com
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> ---

[...]


Quite the simplification :)

>  int __init cma_declare_contiguous_multi(phys_addr_t total_size,
>  			phys_addr_t align, unsigned int order_per_bit,
>  			const char *name, struct cma **res_cma, int nid)
>  {
> -	phys_addr_t start = 0, end;
> -	phys_addr_t size, sizesum, sizeleft;
> -	struct cma_init_memrange *mrp, *mlp, *failed;
> +	unsigned long size_mb = (unsigned long)total_size / SZ_1M;

Nit: could be const.

> +	phys_addr_t size, remaining, size_align;
>  	struct cma_memrange *cmrp;
> -	LIST_HEAD(ranges);
> -	LIST_HEAD(final_ranges);
> -	struct list_head *mp, *next;
> -	int ret, nr = 1;
> -	u64 i;
> +	phys_addr_t start = 0;
>  	struct cma *cma;
> +	int nr = 0;
> +	int ret;
>  
>  	/*
>  	 * First, try it the normal way, producing just one range.
>  	 */
>  	ret = __cma_declare_contiguous_nid(&start, total_size, 0, align,
>  			order_per_bit, false, name, res_cma, nid);
> +	if (!ret) {
> +		pr_info("Reserved %lu MiB at %pa\n", size_mb, &start);
> +		return 0;
> +	}
>  	if (ret != -ENOMEM)
> -		goto out;
> +		goto err;
>  
>  	/*
>  	 * Couldn't find one range that fits our needs, so try multiple
> @@ -572,151 +556,56 @@ int __init cma_declare_contiguous_multi(phys_addr_t total_size,
>  	 * aligned to @align, and a size that is aligned to
>  	 * both @align and @order_to_bit, things will work out.
>  	 */
> -	nr = 0;
> -	sizesum = 0;
> -	failed = NULL;
> -
>  	ret = cma_new_area(name, total_size, order_per_bit, &cma);
> -	if (ret != 0)
> -		goto out;
> +	if (ret)
> +		goto err;
>  
>  	align = max_t(phys_addr_t, align, CMA_MIN_ALIGNMENT_BYTES);
> -	/*
> -	 * Create a list of ranges above 4G, largest range first.
> -	 */
> -	for_each_free_mem_range(i, nid, MEMBLOCK_NONE, &start, &end, NULL) {
> -		if (upper_32_bits(start) == 0)
> -			continue;
> -
> -		start = ALIGN(start, align);
> -		if (start >= end)
> -			continue;
> -
> -		end = ALIGN_DOWN(end, align);
> -		if (end <= start)
> -			continue;
> +	size_align = max_t(phys_addr_t, align, PAGE_SIZE << order_per_bit);
>  
> -		size = end - start;
> -		size = ALIGN_DOWN(size, (PAGE_SIZE << order_per_bit));
> +	remaining = total_size;
> +	while (remaining && nr < CMA_MAX_RANGES) {
> +		size = cma_find_free_range(align, size_align, nid, &start);

That's more a memblock helper than a cma helper :)

[...]

> +	/* allocate from lower addresses first */
> +	sort(cma->ranges, nr, sizeof(cma->ranges[0]), cma_range_cmp, NULL);

That's nice.

Acked-by: David Hildenbrand (Arm) <david@kernel.org>


-- 
Cheers,

David