Re: [PATCH 1/9] mm/rmap: make nr_pages signed in try_to_unmap_one

[Dev Jain <dev.jain@arm.com>]

On 10/03/26 1:36 pm, David Hildenbrand (Arm) wrote:
> On 3/10/26 08:56, Lorenzo Stoakes (Oracle) wrote:
>> On Tue, Mar 10, 2026 at 01:00:05PM +0530, Dev Jain wrote:
>>> Currently, nr_pages is defined as unsigned long. We use nr_pages to
>>> manipulate mm rss counters for lazyfree folios as follows:
>>>
>>> 	add_mm_counter(mm, MM_ANONPAGES, -nr_pages);
>>>
>>> Suppose nr_pages = 3. -nr_pages underflows and becomes ULONG_MAX - 2. Then,
>>> since add_mm_counter() uses this -nr_pages as a long, ULONG_MAX - 2 does
>>> not fit into the positive range of long, and is converted to -3. Eventually
>>> all of this works out, but for keeping things simple, declare nr_pages as
>>> a signed variable.
>>>
>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>> ---
>>>  mm/rmap.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>> index 6398d7eef393f..087c9f5b884fe 100644
>>> --- a/mm/rmap.c
>>> +++ b/mm/rmap.c
>>> @@ -1979,9 +1979,10 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>>  	struct page *subpage;
>>>  	struct mmu_notifier_range range;
>>>  	enum ttu_flags flags = (enum ttu_flags)(long)arg;
>>> -	unsigned long nr_pages = 1, end_addr;
>>> +	unsigned long end_addr;
>>>  	unsigned long pfn;
>>>  	unsigned long hsz = 0;
>>> +	long nr_pages = 1;
>>
>> This is a non-issue that makes the code confusing, so let's not?
>>
>> The convention throughout the kernel is nr_pages generally is unsigned because
>> you can't have negative nr_pages.
> 
> Indeed. Documented in:
> 
> commit fa17bcd5f65ed702df001579cca8c885fa6bf3e7
> Author: Aristeu Rozanski <aris@ruivo.org>
> Date:   Tue Aug 26 11:37:21 2025 -0400
> 
>     mm: make folio page count functions return unsigned
>     
>     As raised by Andrew [1], a folio/compound page never spans a negative
>     number of pages.  Consequently, let's use "unsigned long" instead of
>     "long" consistently for folio_nr_pages(), folio_large_nr_pages() and
>     compound_nr().
>     
>     Using "unsigned long" as return value is fine, because even
>     "(long)-folio_nr_pages()" will keep on working as expected.  Using
>     "unsigned int" instead would actually break these use cases.
>     
>     This patch takes the first step changing these to return unsigned long
>     (and making drm_gem_get_pages() use the new types instead of replacing
>     min()).
>     
>     In the future, we might want to make more callers of these functions to
>     consistently use "unsigned long".

So when I was playing around with the code, I noticed that passing
unsigned int nr_pages to add_mm_counter(-nr_pages) messes up things. Then
I noticed we have an unsigned long here, which prevents that. This is quite
non-trivial information for me, especially when I searched around the
codebase and found this is the only place where we pass a negative unsigned
long.

But thanks for pointing out this commit. If it is a well-known fact that
(long)-folio_nr_pages() will work correctly, then we can drop this patch.

>     
> 
>