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

[David Hildenbrand <david@redhat.com>]

On 18.09.25 15:19, 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.
> 
> Reviewed-by: Jane Chu <jane.chu@oracle.com>
> 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 813e6dc7b095..2af8c5bc58dd 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -942,34 +942,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)

Are you afraid of overflows here, or why can't it simply be

	if (pfn >= cmr->base_pfn && pfn + count <= end)

But I wonder if we want to keep here

	if (pfn >= cmr->base_pfn && pfn < end)
	
And VM_WARN if the area does not completely fit into the range. See below.


>   			break;
> -		}
>   	}
>   
> -	if (!ret)
> -		pr_debug("%s(page %p, count %lu)\n",
> -				__func__, (void *)pages, count);
> +	if (r == cma->nranges)
> +		return false;

Would we want to warn one way or the other in that case? Is it valid 
that someone tries to free a wrong range?

Note that the original code had this pr_debug() in case no range for the 
start pfn was found (IIUC, it's confusing) and this  VM_BUG_ON(end_pfn > 
cmr->base_pfn + cmr->count) in case a range was found but it would not 
completely match the

You're not discussing that behavioral change in the changelog, and I 
think we would want to keep some sanity checks, likely in a 
VM_WARN_ON_ONCE() form.


-- 
Cheers

David / dhildenb