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

[Mike Rapoport <rppt@kernel.org>]

On Mon, Nov 09, 2026 at 09:56:20PM +0100, David Hildenbrand (Arm) wrote:
> 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.

Ack.
 
> > +	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 :)

I'd keep it here in order not to encourage broader use :)
 
> [...]
> 
> > +	/* 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>

Thanks! 
 
> -- 
> Cheers,
> 
> David

-- 
Sincerely yours,
Mike.