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

["David Hildenbrand (Arm)" <david@kernel.org>]

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


-- 
Cheers,

David