[v2] mm/cma: move put_page_testzero() out of VM_WARN_ON in cma_release()

[PATCH v2] mm/cma: move put_page_testzero() out of VM_WARN_ON in cma_release()

Posted by Zi Yan 1 month, 2 weeks ago

When CONFIG_DEBUG_VM is not set, VM_WARN_ON is a NOP. Putting any statement
with side effect inside it is incorrect. Collect all put_page_testzero()
results and check the sum using WARN instead after the loop.

Fixes: 9bda131c6093 ("mm: cma: add cma_alloc_frozen{_compound}()")
Reported-by: Ron Economos <re@w6rz.net>
Closes: https://lore.kernel.org/all/1b17c38f-30d3-4bb4-a7e1-e74b19ada885@w6rz.net/
Suggested-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Signed-off-by: Zi Yan <ziy@nvidia.com>
---
From V1:
- Collect all put_page_testzero() results and do a single WARN after the
  loop.

 mm/cma.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mm/cma.c b/mm/cma.c
index 94b5da468a7d..ea540d3f473c 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -1013,6 +1013,7 @@ bool cma_release(struct cma *cma, const struct page *pages,
 		 unsigned long count)
 {
 	struct cma_memrange *cmr;
+	unsigned long ret = 0;
 	unsigned long i, pfn;
 
 	cmr = find_cma_memrange(cma, pages, count);
@@ -1021,7 +1022,9 @@ bool cma_release(struct cma *cma, const struct page *pages,
 
 	pfn = page_to_pfn(pages);
 	for (i = 0; i < count; i++, pfn++)
-		VM_WARN_ON(!put_page_testzero(pfn_to_page(pfn)));
+		ret += put_page_testzero(pfn_to_page(pfn));
+
+	WARN(ret, "%lu pages are still in use!\n", ret);
 
 	__cma_release_frozen(cma, cmr, pages, count);
 
-- 
2.51.0

Re: [PATCH v2] mm/cma: move put_page_testzero() out of VM_WARN_ON in cma_release()

Posted by SeongJae Park 1 month, 2 weeks ago

On Tue, 24 Feb 2026 21:28:52 -0500 Zi Yan <ziy@nvidia.com> wrote:

> When CONFIG_DEBUG_VM is not set, VM_WARN_ON is a NOP. Putting any statement
> with side effect inside it is incorrect. Collect all put_page_testzero()
> results and check the sum using WARN instead after the loop.
> 
> Fixes: 9bda131c6093 ("mm: cma: add cma_alloc_frozen{_compound}()")
> Reported-by: Ron Economos <re@w6rz.net>
> Closes: https://lore.kernel.org/all/1b17c38f-30d3-4bb4-a7e1-e74b19ada885@w6rz.net/
> Suggested-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
> >From V1:
> - Collect all put_page_testzero() results and do a single WARN after the
>   loop.
> 
>  mm/cma.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/cma.c b/mm/cma.c
> index 94b5da468a7d..ea540d3f473c 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -1013,6 +1013,7 @@ bool cma_release(struct cma *cma, const struct page *pages,
>  		 unsigned long count)
>  {
>  	struct cma_memrange *cmr;
> +	unsigned long ret = 0;
>  	unsigned long i, pfn;
>  
>  	cmr = find_cma_memrange(cma, pages, count);
> @@ -1021,7 +1022,9 @@ bool cma_release(struct cma *cma, const struct page *pages,
>  
>  	pfn = page_to_pfn(pages);
>  	for (i = 0; i < count; i++, pfn++)
> -		VM_WARN_ON(!put_page_testzero(pfn_to_page(pfn)));
> +		ret += put_page_testzero(pfn_to_page(pfn));

So, 'ret' will be incremented when put_page_testzero() returns 'true', right?
Shouldn't 'ret' be inceremented only when put_page_testzero() returns 'false'?

> +
> +	WARN(ret, "%lu pages are still in use!\n", ret);

This will trigger the warning even if CONFIG_DEBUG_VM is unset.  I'm fine with
the changed behavior, but I'm curious if that is the real intention of this
revision.


Thanks,
SJ

[...]

Re: [PATCH v2] mm/cma: move put_page_testzero() out of VM_WARN_ON in cma_release()

Posted by Zi Yan 1 month, 2 weeks ago

On 24 Feb 2026, at 21:54, SeongJae Park wrote:

> On Tue, 24 Feb 2026 21:28:52 -0500 Zi Yan <ziy@nvidia.com> wrote:
>
>> When CONFIG_DEBUG_VM is not set, VM_WARN_ON is a NOP. Putting any statement
>> with side effect inside it is incorrect. Collect all put_page_testzero()
>> results and check the sum using WARN instead after the loop.
>>
>> Fixes: 9bda131c6093 ("mm: cma: add cma_alloc_frozen{_compound}()")
>> Reported-by: Ron Economos <re@w6rz.net>
>> Closes: https://lore.kernel.org/all/1b17c38f-30d3-4bb4-a7e1-e74b19ada885@w6rz.net/
>> Suggested-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> ---
>>> From V1:
>> - Collect all put_page_testzero() results and do a single WARN after the
>>   loop.
>>
>>  mm/cma.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/cma.c b/mm/cma.c
>> index 94b5da468a7d..ea540d3f473c 100644
>> --- a/mm/cma.c
>> +++ b/mm/cma.c
>> @@ -1013,6 +1013,7 @@ bool cma_release(struct cma *cma, const struct page *pages,
>>  		 unsigned long count)
>>  {
>>  	struct cma_memrange *cmr;
>> +	unsigned long ret = 0;
>>  	unsigned long i, pfn;
>>
>>  	cmr = find_cma_memrange(cma, pages, count);
>> @@ -1021,7 +1022,9 @@ bool cma_release(struct cma *cma, const struct page *pages,
>>
>>  	pfn = page_to_pfn(pages);
>>  	for (i = 0; i < count; i++, pfn++)
>> -		VM_WARN_ON(!put_page_testzero(pfn_to_page(pfn)));
>> +		ret += put_page_testzero(pfn_to_page(pfn));
>
> So, 'ret' will be incremented when put_page_testzero() returns 'true', right?
> Shouldn't 'ret' be inceremented only when put_page_testzero() returns 'false'?

Good catch. Let me fix it. Thanks.

>
>> +
>> +	WARN(ret, "%lu pages are still in use!\n", ret);
>
> This will trigger the warning even if CONFIG_DEBUG_VM is unset.  I'm fine with
> the changed behavior, but I'm curious if that is the real intention of this
> revision.

It was in free_contig_range()[1], which was called by cma_release() before.

[1] https://elixir.bootlin.com/linux/v6.19/source/mm/page_alloc.c#L7238


--
Best Regards,
Yan, Zi

Re: [PATCH v2] mm/cma: move put_page_testzero() out of VM_WARN_ON in cma_release()

Posted by SeongJae Park 1 month, 2 weeks ago

On Tue, 24 Feb 2026 22:04:15 -0500 Zi Yan <ziy@nvidia.com> wrote:

> On 24 Feb 2026, at 21:54, SeongJae Park wrote:
> 
> > On Tue, 24 Feb 2026 21:28:52 -0500 Zi Yan <ziy@nvidia.com> wrote:
> >
> >> When CONFIG_DEBUG_VM is not set, VM_WARN_ON is a NOP. Putting any statement
> >> with side effect inside it is incorrect. Collect all put_page_testzero()
> >> results and check the sum using WARN instead after the loop.
> >>
> >> Fixes: 9bda131c6093 ("mm: cma: add cma_alloc_frozen{_compound}()")
> >> Reported-by: Ron Economos <re@w6rz.net>
> >> Closes: https://lore.kernel.org/all/1b17c38f-30d3-4bb4-a7e1-e74b19ada885@w6rz.net/
> >> Suggested-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> >> Signed-off-by: Zi Yan <ziy@nvidia.com>
> >> ---
> >>> From V1:
> >> - Collect all put_page_testzero() results and do a single WARN after the
> >>   loop.
> >>
> >>  mm/cma.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/mm/cma.c b/mm/cma.c
> >> index 94b5da468a7d..ea540d3f473c 100644
> >> --- a/mm/cma.c
> >> +++ b/mm/cma.c
> >> @@ -1013,6 +1013,7 @@ bool cma_release(struct cma *cma, const struct page *pages,
> >>  		 unsigned long count)
> >>  {
> >>  	struct cma_memrange *cmr;
> >> +	unsigned long ret = 0;
> >>  	unsigned long i, pfn;
> >>
> >>  	cmr = find_cma_memrange(cma, pages, count);
> >> @@ -1021,7 +1022,9 @@ bool cma_release(struct cma *cma, const struct page *pages,
> >>
> >>  	pfn = page_to_pfn(pages);
> >>  	for (i = 0; i < count; i++, pfn++)
> >> -		VM_WARN_ON(!put_page_testzero(pfn_to_page(pfn)));
> >> +		ret += put_page_testzero(pfn_to_page(pfn));
> >
> > So, 'ret' will be incremented when put_page_testzero() returns 'true', right?
> > Shouldn't 'ret' be inceremented only when put_page_testzero() returns 'false'?
> 
> Good catch. Let me fix it. Thanks.
> 
> >
> >> +
> >> +	WARN(ret, "%lu pages are still in use!\n", ret);
> >
> > This will trigger the warning even if CONFIG_DEBUG_VM is unset.  I'm fine with
> > the changed behavior, but I'm curious if that is the real intention of this
> > revision.
> 
> It was in free_contig_range()[1], which was called by cma_release() before.

Makes sense, thank you for clarifying, Zi :)

> 
> [1] https://elixir.bootlin.com/linux/v6.19/source/mm/page_alloc.c#L7238


Thanks,
SJ

[...]