Re: [PATCH v2 8/9] mm: cma: add __cma_release()

[jane.chu@oracle.com]

On 9/2/2025 5:48 AM, Kefeng Wang wrote:
> Kill cma_pages_valid() which only used in cma_release(), also
> cleanup code duplication between cma pages valid checking and
> cma memrange finding, add __cma_release() helper to prepare for
> the upcoming frozen page release.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   include/linux/cma.h |  1 -
>   mm/cma.c            | 57 ++++++++++++---------------------------------
>   2 files changed, 15 insertions(+), 43 deletions(-)
> 
> diff --git a/include/linux/cma.h b/include/linux/cma.h
> index 62d9c1cf6326..e5745d2aec55 100644
> --- a/include/linux/cma.h
> +++ b/include/linux/cma.h
> @@ -49,7 +49,6 @@ extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
>   					struct cma **res_cma);
>   extern struct page *cma_alloc(struct cma *cma, unsigned long count, unsigned int align,
>   			      bool no_warn);
> -extern bool cma_pages_valid(struct cma *cma, const struct page *pages, unsigned long count);
>   extern bool cma_release(struct cma *cma, const struct page *pages, unsigned long count);
>   
>   extern int cma_for_each_area(int (*it)(struct cma *cma, void *data), void *data);
> diff --git a/mm/cma.c b/mm/cma.c
> index 3f3c96be67f7..b4413e382d5d 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -936,34 +936,36 @@ struct folio *cma_alloc_folio(struct cma *cma, int order, gfp_t gfp)
>   	return page ? page_folio(page) : NULL;
>   }
>   
> -bool cma_pages_valid(struct cma *cma, const struct page *pages,
> -		     unsigned long count)
> +static bool __cma_release(struct cma *cma, const struct page *pages,
> +			  unsigned long count)
>   {
>   	unsigned long pfn, end;
>   	int r;
>   	struct cma_memrange *cmr;
> -	bool ret;
> +
> +	pr_debug("%s(page %p, count %lu)\n", __func__, (void *)pages, count);
>   
>   	if (!cma || !pages || count > cma->count)
>   		return false;
>   
>   	pfn = page_to_pfn(pages);
> -	ret = false;
>   
>   	for (r = 0; r < cma->nranges; r++) {
>   		cmr = &cma->ranges[r];
>   		end = cmr->base_pfn + cmr->count;
> -		if (pfn >= cmr->base_pfn && pfn < end) {
> -			ret = pfn + count <= end;
> +		if (pfn >= cmr->base_pfn && pfn < end && pfn + count <= end)
>   			break;
> -		}
>   	}

The only difference from the previous code is the now missing VM_BUG_ON 
upon a given range that stretches across the CMA ranges.
The warning was introduced by
  c64be2bb1c6eb drivers: add Contiguous Memory Allocator
about 15 years ago as a caution only.
I am okay to just return false in the potential error case.

>   
> -	if (!ret)
> -		pr_debug("%s(page %p, count %lu)\n",
> -				__func__, (void *)pages, count);
> +	if (r == cma->nranges)
> +		return false;
>   
> -	return ret;
> +	free_contig_range(pfn, count);
> +	cma_clear_bitmap(cma, cmr, pfn, count);
> +	cma_sysfs_account_release_pages(cma, count);
> +	trace_cma_release(cma->name, pfn, pages, count);
> +
> +	return true;
>   }
>   
>   /**
> @@ -979,36 +981,7 @@ bool cma_pages_valid(struct cma *cma, const struct page *pages,
>   bool cma_release(struct cma *cma, const struct page *pages,
>   		 unsigned long count)
>   {
> -	struct cma_memrange *cmr;
> -	unsigned long pfn, end_pfn;
> -	int r;
> -
> -	pr_debug("%s(page %p, count %lu)\n", __func__, (void *)pages, count);
> -
> -	if (!cma_pages_valid(cma, pages, count))
> -		return false;
> -
> -	pfn = page_to_pfn(pages);
> -	end_pfn = pfn + count;
> -
> -	for (r = 0; r < cma->nranges; r++) {
> -		cmr = &cma->ranges[r];
> -		if (pfn >= cmr->base_pfn &&
> -		    pfn < (cmr->base_pfn + cmr->count)) {
> -			VM_BUG_ON(end_pfn > cmr->base_pfn + cmr->count);
> -			break;
> -		}
> -	}
> -
> -	if (r == cma->nranges)
> -		return false;
> -
> -	free_contig_range(pfn, count);
> -	cma_clear_bitmap(cma, cmr, pfn, count);
> -	cma_sysfs_account_release_pages(cma, count);
> -	trace_cma_release(cma->name, pfn, pages, count);
> -
> -	return true;
> +	return __cma_release(cma, pages, count);
>   }
>   
>   bool cma_free_folio(struct cma *cma, const struct folio *folio)
> @@ -1016,7 +989,7 @@ bool cma_free_folio(struct cma *cma, const struct folio *folio)
>   	if (WARN_ON(!folio_test_large(folio)))
>   		return false;
>   
> -	return cma_release(cma, &folio->page, folio_nr_pages(folio));
> +	return __cma_release(cma, &folio->page, folio_nr_pages(folio));
>   }
>   
>   int cma_for_each_area(int (*it)(struct cma *cma, void *data), void *data)

Nice clean up.

Reviewed-by: Jane Chu <jane.chu@oracle.com>

-jane