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

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2025/9/30 18:15, David Hildenbrand wrote:
> 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?

The original cma_pages_valid() check start pfn whether it is in cma
range or not, and the range must be within the complete cma range.
The repeatedly check "VM_BUG_ON(pfn + count > end)" in cma_release()
is never performed since we return early after cma_pages_valid().

> 
> 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

So the VM_BUG_ON is not useful,
> 
> 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.
> 
> 

But for the error path, adding some debug info is better, a quick diff
based on this patch, what do you think?


diff --git a/mm/cma.c b/mm/cma.c
index 2af8c5bc58dd..88016f4aef7f 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -959,12 +959,19 @@ static bool __cma_release(struct cma *cma, const 
struct page *pages,
         for (r = 0; r < cma->nranges; r++) {
                 cmr = &cma->ranges[r];
                 end = cmr->base_pfn + cmr->count;
-               if (pfn >= cmr->base_pfn && pfn < end && pfn + count <= end)
-                       break;
+               if (pfn >= cmr->base_pfn && pfn < end) {
+                       if (pfn + count <= end)
+                               break;
+
+                       VM_WARN_ON_ONCE(1);
+               }
         }

-       if (r == cma->nranges)
+       if (r == cma->nranges) {
+               pr_debug("%s(no cma range match the page %p)\n",
+                        __func__, (void *)pages);
                 return false;
+       }

         free_contig_range(pfn, count);
         cma_clear_bitmap(cma, cmr, pfn, count);