[Patch v3] mm/huge_memory: simplify page tracking in remap_page() during folio split

[Patch v3] mm/huge_memory: simplify page tracking in remap_page() during folio split

[Wei Yang]

After splitting a large folio, the resulting anonymous folios must be
remapped.

Currently, the code tracks progress by recording the number of processed
pages in an index variable (e.g., @i) and comparing it against the
total. This commit simplifies the logic by directly subtracting the
processed pages from the remaining count. This approach is more
straightforward and reduces the number of local variables.

Additionally, this commit renames the variable nr to nr_pages to improve
code readability and self-documentation.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Cc: Zi Yan <ziy@nvidia.com>

---
v3: still use nr to count pages but subtract it directly
v2: move folio assignment in loop
---
 mm/huge_memory.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 40cf59301c21..d51aff0b7838 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3423,17 +3423,15 @@ bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
 	return __discard_anon_folio_pmd_locked(vma, addr, pmdp, folio);
 }
 
-static void remap_page(struct folio *folio, unsigned long nr, int flags)
+static void remap_page(struct folio *folio, unsigned long nr_pages, int flags)
 {
-	int i = 0;
-
 	/* If unmap_folio() uses try_to_migrate() on file, remove this check */
 	if (!folio_test_anon(folio))
 		return;
 	for (;;) {
 		remove_migration_ptes(folio, folio, RMP_LOCKED | flags);
-		i += folio_nr_pages(folio);
-		if (i >= nr)
+		nr_pages -= folio_nr_pages(folio);
+		if (!nr_pages)
 			break;
 		folio = folio_next(folio);
 	}
-- 
2.34.1

Re: [Patch v3] mm/huge_memory: simplify page tracking in remap_page() during folio split

[Andrew Morton]

On Mon, 22 Dec 2025 13:45:31 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:

> After splitting a large folio, the resulting anonymous folios must be
> remapped.
> 
> Currently, the code tracks progress by recording the number of processed
> pages in an index variable (e.g., @i) and comparing it against the
> total. This commit simplifies the logic by directly subtracting the
> processed pages from the remaining count. This approach is more
> straightforward and reduces the number of local variables.
> 
> Additionally, this commit renames the variable nr to nr_pages to improve
> code readability and self-documentation.
> 
> ...
>
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3423,17 +3423,15 @@ bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
>  	return __discard_anon_folio_pmd_locked(vma, addr, pmdp, folio);
>  }
>  
> -static void remap_page(struct folio *folio, unsigned long nr, int flags)
> +static void remap_page(struct folio *folio, unsigned long nr_pages, int flags)
>  {
> -	int i = 0;
> -
>  	/* If unmap_folio() uses try_to_migrate() on file, remove this check */
>  	if (!folio_test_anon(folio))
>  		return;
>  	for (;;) {
>  		remove_migration_ptes(folio, folio, RMP_LOCKED | flags);
> -		i += folio_nr_pages(folio);
> -		if (i >= nr)
> +		nr_pages -= folio_nr_pages(folio);
> +		if (!nr_pages)
>  			break;
>  		folio = folio_next(folio);
>  	}

This changes behavior when nr_pages was too large.  It would be best to
mention this and to describe why this is OK.  Or to check for
((signed)nr_pages <= 0).

Also, isn't it a bit nasty to alter an incoming arg?  I think it's best
to treat all those as const - part of the function's calling
environment.  Yes, C permits this hack but that was a mistake!

Re: [Patch v3] mm/huge_memory: simplify page tracking in remap_page() during folio split

[David Hildenbrand (Red Hat)]

On 12/22/25 19:14, Andrew Morton wrote:
> On Mon, 22 Dec 2025 13:45:31 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:
> 
>> After splitting a large folio, the resulting anonymous folios must be
>> remapped.
>>
>> Currently, the code tracks progress by recording the number of processed
>> pages in an index variable (e.g., @i) and comparing it against the
>> total. This commit simplifies the logic by directly subtracting the
>> processed pages from the remaining count. This approach is more
>> straightforward and reduces the number of local variables.
>>
>> Additionally, this commit renames the variable nr to nr_pages to improve
>> code readability and self-documentation.
>>
>> ...
>>
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -3423,17 +3423,15 @@ bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
>>   	return __discard_anon_folio_pmd_locked(vma, addr, pmdp, folio);
>>   }
>>   
>> -static void remap_page(struct folio *folio, unsigned long nr, int flags)
>> +static void remap_page(struct folio *folio, unsigned long nr_pages, int flags)
>>   {
>> -	int i = 0;
>> -
>>   	/* If unmap_folio() uses try_to_migrate() on file, remove this check */
>>   	if (!folio_test_anon(folio))
>>   		return;
>>   	for (;;) {
>>   		remove_migration_ptes(folio, folio, RMP_LOCKED | flags);
>> -		i += folio_nr_pages(folio);
>> -		if (i >= nr)
>> +		nr_pages -= folio_nr_pages(folio);
>> +		if (!nr_pages)
>>   			break;
>>   		folio = folio_next(folio);
>>   	}
> 
> This changes behavior when nr_pages was too large.  It would be best to
> mention this and to describe why this is OK.  Or to check for
> ((signed)nr_pages <= 0).
> 
> Also, isn't it a bit nasty to alter an incoming arg?  I think it's best
> to treat all those as const - part of the function's calling
> environment.  Yes, C permits this hack but that was a mistake!

For small functions I think it's quite acceptable.

But yeah, for bigger functions it can happen too easily that one ends up 
reusing a now-modified value by mistake.

-- 
Cheers

David