[PATCH v3] mempolicy: Optimize queue_folios_pte_range by PTE batching

[PATCH v3] mempolicy: Optimize queue_folios_pte_range by PTE batching

[Dev Jain]

After the check for queue_folio_required(), the code only cares about the
folio in the for loop, i.e the PTEs are redundant. Therefore, optimize
this loop by skipping over a PTE batch mapping the same folio.

With a test program migrating pages of the calling process, which includes
a mapped VMA of size 4GB with pte-mapped large folios of order-9, and
migrating once back and forth node-0 and node-1, the average execution
time reduces from 7.5 to 4 seconds, giving an approx 47% speedup.

v2->v3:
 - Don't use assignment in if condition

v1->v2:
 - Follow reverse xmas tree declarations
 - Don't initialize nr
 - Move folio_pte_batch() immediately after retrieving a normal folio
 - increment nr_failed in one shot

Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Dev Jain <dev.jain@arm.com>
---
 mm/mempolicy.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index b28a1e6ae096..4d2dc8b63965 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -566,6 +566,7 @@ static void queue_folios_pmd(pmd_t *pmd, struct mm_walk *walk)
 static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
 			unsigned long end, struct mm_walk *walk)
 {
+	const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
 	struct vm_area_struct *vma = walk->vma;
 	struct folio *folio;
 	struct queue_pages *qp = walk->private;
@@ -573,6 +574,7 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
 	pte_t *pte, *mapped_pte;
 	pte_t ptent;
 	spinlock_t *ptl;
+	int max_nr, nr;
 
 	ptl = pmd_trans_huge_lock(pmd, vma);
 	if (ptl) {
@@ -586,7 +588,9 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
 		walk->action = ACTION_AGAIN;
 		return 0;
 	}
-	for (; addr != end; pte++, addr += PAGE_SIZE) {
+	for (; addr != end; pte += nr, addr += nr * PAGE_SIZE) {
+		max_nr = (end - addr) >> PAGE_SHIFT;
+		nr = 1;
 		ptent = ptep_get(pte);
 		if (pte_none(ptent))
 			continue;
@@ -598,6 +602,10 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
 		folio = vm_normal_folio(vma, addr, ptent);
 		if (!folio || folio_is_zone_device(folio))
 			continue;
+		if (folio_test_large(folio) && max_nr != 1)
+			nr = folio_pte_batch(folio, addr, pte, ptent,
+					     max_nr, fpb_flags,
+					     NULL, NULL, NULL);
 		/*
 		 * vm_normal_folio() filters out zero pages, but there might
 		 * still be reserved folios to skip, perhaps in a VDSO.
@@ -630,7 +638,7 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
 		if (!(flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) ||
 		    !vma_migratable(vma) ||
 		    !migrate_folio_add(folio, qp->pagelist, flags)) {
-			qp->nr_failed++;
+			qp->nr_failed += nr;
 			if (strictly_unmovable(flags))
 				break;
 		}
-- 
2.30.2

Re: [PATCH v3] mempolicy: Optimize queue_folios_pte_range by PTE batching

[Baolin Wang]

On 2025/4/16 13:30, Dev Jain wrote:
> After the check for queue_folio_required(), the code only cares about the
> folio in the for loop, i.e the PTEs are redundant. Therefore, optimize
> this loop by skipping over a PTE batch mapping the same folio.
> 
> With a test program migrating pages of the calling process, which includes
> a mapped VMA of size 4GB with pte-mapped large folios of order-9, and
> migrating once back and forth node-0 and node-1, the average execution
> time reduces from 7.5 to 4 seconds, giving an approx 47% speedup.
> 
> v2->v3:
>   - Don't use assignment in if condition
> 
> v1->v2:
>   - Follow reverse xmas tree declarations
>   - Don't initialize nr
>   - Move folio_pte_batch() immediately after retrieving a normal folio
>   - increment nr_failed in one shot
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Dev Jain <dev.jain@arm.com>
> ---
>   mm/mempolicy.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index b28a1e6ae096..4d2dc8b63965 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -566,6 +566,7 @@ static void queue_folios_pmd(pmd_t *pmd, struct mm_walk *walk)
>   static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>   			unsigned long end, struct mm_walk *walk)
>   {
> +	const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>   	struct vm_area_struct *vma = walk->vma;
>   	struct folio *folio;
>   	struct queue_pages *qp = walk->private;
> @@ -573,6 +574,7 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>   	pte_t *pte, *mapped_pte;
>   	pte_t ptent;
>   	spinlock_t *ptl;
> +	int max_nr, nr;
>   
>   	ptl = pmd_trans_huge_lock(pmd, vma);
>   	if (ptl) {
> @@ -586,7 +588,9 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>   		walk->action = ACTION_AGAIN;
>   		return 0;
>   	}
> -	for (; addr != end; pte++, addr += PAGE_SIZE) {
> +	for (; addr != end; pte += nr, addr += nr * PAGE_SIZE) {
> +		max_nr = (end - addr) >> PAGE_SHIFT;
> +		nr = 1;
>   		ptent = ptep_get(pte);
>   		if (pte_none(ptent))
>   			continue;
> @@ -598,6 +602,10 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>   		folio = vm_normal_folio(vma, addr, ptent);
>   		if (!folio || folio_is_zone_device(folio))
>   			continue;
> +		if (folio_test_large(folio) && max_nr != 1)
> +			nr = folio_pte_batch(folio, addr, pte, ptent,
> +					     max_nr, fpb_flags,
> +					     NULL, NULL, NULL);
>   		/*
>   		 * vm_normal_folio() filters out zero pages, but there might
>   		 * still be reserved folios to skip, perhaps in a VDSO.
> @@ -630,7 +638,7 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>   		if (!(flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) ||
>   		    !vma_migratable(vma) ||
>   		    !migrate_folio_add(folio, qp->pagelist, flags)) {
> -			qp->nr_failed++;
> +			qp->nr_failed += nr;

Sorry for chiming in late, but I am not convinced that 'qp->nr_failed' 
should add 'nr' when isolation fails.

 From the comments of queue_pages_range():
"
* >0 - this number of misplaced folios could not be queued for moving
  *      (a hugetlbfs page or a transparent huge page being counted as 1).
"

That means if a large folio is failed to isolate, we should only add '1' 
for qp->nr_failed instead of the number of pages in this large folio. Right?

Re: [PATCH v3] mempolicy: Optimize queue_folios_pte_range by PTE batching

[David Hildenbrand]

On 16.04.25 08:32, Baolin Wang wrote:
> 
> 
> On 2025/4/16 13:30, Dev Jain wrote:
>> After the check for queue_folio_required(), the code only cares about the
>> folio in the for loop, i.e the PTEs are redundant. Therefore, optimize
>> this loop by skipping over a PTE batch mapping the same folio.
>>
>> With a test program migrating pages of the calling process, which includes
>> a mapped VMA of size 4GB with pte-mapped large folios of order-9, and
>> migrating once back and forth node-0 and node-1, the average execution
>> time reduces from 7.5 to 4 seconds, giving an approx 47% speedup.
>>
>> v2->v3:
>>    - Don't use assignment in if condition
>>
>> v1->v2:
>>    - Follow reverse xmas tree declarations
>>    - Don't initialize nr
>>    - Move folio_pte_batch() immediately after retrieving a normal folio
>>    - increment nr_failed in one shot
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>> ---
>>    mm/mempolicy.c | 12 ++++++++++--
>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index b28a1e6ae096..4d2dc8b63965 100644
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -566,6 +566,7 @@ static void queue_folios_pmd(pmd_t *pmd, struct mm_walk *walk)
>>    static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>>    			unsigned long end, struct mm_walk *walk)
>>    {
>> +	const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>    	struct vm_area_struct *vma = walk->vma;
>>    	struct folio *folio;
>>    	struct queue_pages *qp = walk->private;
>> @@ -573,6 +574,7 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>>    	pte_t *pte, *mapped_pte;
>>    	pte_t ptent;
>>    	spinlock_t *ptl;
>> +	int max_nr, nr;
>>    
>>    	ptl = pmd_trans_huge_lock(pmd, vma);
>>    	if (ptl) {
>> @@ -586,7 +588,9 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>>    		walk->action = ACTION_AGAIN;
>>    		return 0;
>>    	}
>> -	for (; addr != end; pte++, addr += PAGE_SIZE) {
>> +	for (; addr != end; pte += nr, addr += nr * PAGE_SIZE) {
>> +		max_nr = (end - addr) >> PAGE_SHIFT;
>> +		nr = 1;
>>    		ptent = ptep_get(pte);
>>    		if (pte_none(ptent))
>>    			continue;
>> @@ -598,6 +602,10 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>>    		folio = vm_normal_folio(vma, addr, ptent);
>>    		if (!folio || folio_is_zone_device(folio))
>>    			continue;
>> +		if (folio_test_large(folio) && max_nr != 1)
>> +			nr = folio_pte_batch(folio, addr, pte, ptent,
>> +					     max_nr, fpb_flags,
>> +					     NULL, NULL, NULL);
>>    		/*
>>    		 * vm_normal_folio() filters out zero pages, but there might
>>    		 * still be reserved folios to skip, perhaps in a VDSO.
>> @@ -630,7 +638,7 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>>    		if (!(flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) ||
>>    		    !vma_migratable(vma) ||
>>    		    !migrate_folio_add(folio, qp->pagelist, flags)) {
>> -			qp->nr_failed++;
>> +			qp->nr_failed += nr;
> 
> Sorry for chiming in late, but I am not convinced that 'qp->nr_failed'
> should add 'nr' when isolation fails.

This patch does not change the existing behavior. But I stumbled over 
that as well ... and scratched my head.

> 
>   From the comments of queue_pages_range():
> "
> * >0 - this number of misplaced folios could not be queued for moving
>    *      (a hugetlbfs page or a transparent huge page being counted as 1).
> "
> 
> That means if a large folio is failed to isolate, we should only add '1'
> for qp->nr_failed instead of the number of pages in this large folio. Right?

I think what the doc really meant is "PMD-mapped THP". PTE-mapped THPs 
always had the same behavior: per PTE of the THP we would increment 
nr_failed by 1.

I assume returning "1" for PMD-mapped THPs was wrong from the beginning; 
it might only have been right for hugetlb pages.

With COW and similar things (VMA splits), achieving "count each folio 
only once" reliably is a very hard thing to achieve.


Let's explore how "nr_failed" will get used.

1) do_mbind()

Only cares if "any failed", not the exact number.


2) migrate_pages()

Will return the number to user space, where documentation says:

"On success migrate_pages() returns the number of pages that could not 
be moved (i.e., a return of zero means that all pages were successfully 
moved)."

man-page does not document THP specifics AFAIKs. I would assume most 
users care about "all migrated vs. any not migrated".


I would even feel confident to change the THP PMD-handling to return the 
actual *pages*.

-- 
Cheers,

David / dhildenb

Re: [PATCH v3] mempolicy: Optimize queue_folios_pte_range by PTE batching

[Baolin Wang]

On 2025/4/16 16:22, David Hildenbrand wrote:
> On 16.04.25 08:32, Baolin Wang wrote:
>>
>>
>> On 2025/4/16 13:30, Dev Jain wrote:
>>> After the check for queue_folio_required(), the code only cares about 
>>> the
>>> folio in the for loop, i.e the PTEs are redundant. Therefore, optimize
>>> this loop by skipping over a PTE batch mapping the same folio.
>>>
>>> With a test program migrating pages of the calling process, which 
>>> includes
>>> a mapped VMA of size 4GB with pte-mapped large folios of order-9, and
>>> migrating once back and forth node-0 and node-1, the average execution
>>> time reduces from 7.5 to 4 seconds, giving an approx 47% speedup.
>>>
>>> v2->v3:
>>>    - Don't use assignment in if condition
>>>
>>> v1->v2:
>>>    - Follow reverse xmas tree declarations
>>>    - Don't initialize nr
>>>    - Move folio_pte_batch() immediately after retrieving a normal folio
>>>    - increment nr_failed in one shot
>>>
>>> Acked-by: David Hildenbrand <david@redhat.com>
>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>> ---
>>>    mm/mempolicy.c | 12 ++++++++++--
>>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>>> index b28a1e6ae096..4d2dc8b63965 100644
>>> --- a/mm/mempolicy.c
>>> +++ b/mm/mempolicy.c
>>> @@ -566,6 +566,7 @@ static void queue_folios_pmd(pmd_t *pmd, struct 
>>> mm_walk *walk)
>>>    static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>>>                unsigned long end, struct mm_walk *walk)
>>>    {
>>> +    const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>        struct vm_area_struct *vma = walk->vma;
>>>        struct folio *folio;
>>>        struct queue_pages *qp = walk->private;
>>> @@ -573,6 +574,7 @@ static int queue_folios_pte_range(pmd_t *pmd, 
>>> unsigned long addr,
>>>        pte_t *pte, *mapped_pte;
>>>        pte_t ptent;
>>>        spinlock_t *ptl;
>>> +    int max_nr, nr;
>>>        ptl = pmd_trans_huge_lock(pmd, vma);
>>>        if (ptl) {
>>> @@ -586,7 +588,9 @@ static int queue_folios_pte_range(pmd_t *pmd, 
>>> unsigned long addr,
>>>            walk->action = ACTION_AGAIN;
>>>            return 0;
>>>        }
>>> -    for (; addr != end; pte++, addr += PAGE_SIZE) {
>>> +    for (; addr != end; pte += nr, addr += nr * PAGE_SIZE) {
>>> +        max_nr = (end - addr) >> PAGE_SHIFT;
>>> +        nr = 1;
>>>            ptent = ptep_get(pte);
>>>            if (pte_none(ptent))
>>>                continue;
>>> @@ -598,6 +602,10 @@ static int queue_folios_pte_range(pmd_t *pmd, 
>>> unsigned long addr,
>>>            folio = vm_normal_folio(vma, addr, ptent);
>>>            if (!folio || folio_is_zone_device(folio))
>>>                continue;
>>> +        if (folio_test_large(folio) && max_nr != 1)
>>> +            nr = folio_pte_batch(folio, addr, pte, ptent,
>>> +                         max_nr, fpb_flags,
>>> +                         NULL, NULL, NULL);
>>>            /*
>>>             * vm_normal_folio() filters out zero pages, but there might
>>>             * still be reserved folios to skip, perhaps in a VDSO.
>>> @@ -630,7 +638,7 @@ static int queue_folios_pte_range(pmd_t *pmd, 
>>> unsigned long addr,
>>>            if (!(flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) ||
>>>                !vma_migratable(vma) ||
>>>                !migrate_folio_add(folio, qp->pagelist, flags)) {
>>> -            qp->nr_failed++;
>>> +            qp->nr_failed += nr;
>>
>> Sorry for chiming in late, but I am not convinced that 'qp->nr_failed'
>> should add 'nr' when isolation fails.
> 
> This patch does not change the existing behavior. But I stumbled over 
> that as well ... and scratched my head.
> 
>>
>>   From the comments of queue_pages_range():
>> "
>> * >0 - this number of misplaced folios could not be queued for moving
>>    *      (a hugetlbfs page or a transparent huge page being counted 
>> as 1).
>> "
>>
>> That means if a large folio is failed to isolate, we should only add '1'
>> for qp->nr_failed instead of the number of pages in this large folio. 
>> Right?
> 
> I think what the doc really meant is "PMD-mapped THP". PTE-mapped THPs 
> always had the same behavior: per PTE of the THP we would increment 
> nr_failed by 1.

No? For pte-mapped THPs, it only adds 1 for the large folio, since we 
have below check in queue_folios_pte_range().

if (folio == qp->large)
	continue;

Or I missed anything else?

> I assume returning "1" for PMD-mapped THPs was wrong from the beginning; 
> it might only have been right for hugetlb pages.
> 
> With COW and similar things (VMA splits), achieving "count each folio 
> only once" reliably is a very hard thing to achieve.
> 
> 
> Let's explore how "nr_failed" will get used.
> 
> 1) do_mbind()
> 
> Only cares if "any failed", not the exact number.
> 
> 
> 2) migrate_pages()
> 
> Will return the number to user space, where documentation says:
> 
> "On success migrate_pages() returns the number of pages that could not 
> be moved (i.e., a return of zero means that all pages were successfully 
> moved)."
> 
> man-page does not document THP specifics AFAIKs. I would assume most 
> users care about "all migrated vs. any not migrated".
> 
> 
> I would even feel confident to change the THP PMD-handling to return the 
> actual *pages*.
> 

Re: [PATCH v3] mempolicy: Optimize queue_folios_pte_range by PTE batching

[David Hildenbrand]

On 16.04.25 10:41, Baolin Wang wrote:
> 
> 
> On 2025/4/16 16:22, David Hildenbrand wrote:
>> On 16.04.25 08:32, Baolin Wang wrote:
>>>
>>>
>>> On 2025/4/16 13:30, Dev Jain wrote:
>>>> After the check for queue_folio_required(), the code only cares about
>>>> the
>>>> folio in the for loop, i.e the PTEs are redundant. Therefore, optimize
>>>> this loop by skipping over a PTE batch mapping the same folio.
>>>>
>>>> With a test program migrating pages of the calling process, which
>>>> includes
>>>> a mapped VMA of size 4GB with pte-mapped large folios of order-9, and
>>>> migrating once back and forth node-0 and node-1, the average execution
>>>> time reduces from 7.5 to 4 seconds, giving an approx 47% speedup.
>>>>
>>>> v2->v3:
>>>>     - Don't use assignment in if condition
>>>>
>>>> v1->v2:
>>>>     - Follow reverse xmas tree declarations
>>>>     - Don't initialize nr
>>>>     - Move folio_pte_batch() immediately after retrieving a normal folio
>>>>     - increment nr_failed in one shot
>>>>
>>>> Acked-by: David Hildenbrand <david@redhat.com>
>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>> ---
>>>>     mm/mempolicy.c | 12 ++++++++++--
>>>>     1 file changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>>>> index b28a1e6ae096..4d2dc8b63965 100644
>>>> --- a/mm/mempolicy.c
>>>> +++ b/mm/mempolicy.c
>>>> @@ -566,6 +566,7 @@ static void queue_folios_pmd(pmd_t *pmd, struct
>>>> mm_walk *walk)
>>>>     static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>>>>                 unsigned long end, struct mm_walk *walk)
>>>>     {
>>>> +    const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>         struct vm_area_struct *vma = walk->vma;
>>>>         struct folio *folio;
>>>>         struct queue_pages *qp = walk->private;
>>>> @@ -573,6 +574,7 @@ static int queue_folios_pte_range(pmd_t *pmd,
>>>> unsigned long addr,
>>>>         pte_t *pte, *mapped_pte;
>>>>         pte_t ptent;
>>>>         spinlock_t *ptl;
>>>> +    int max_nr, nr;
>>>>         ptl = pmd_trans_huge_lock(pmd, vma);
>>>>         if (ptl) {
>>>> @@ -586,7 +588,9 @@ static int queue_folios_pte_range(pmd_t *pmd,
>>>> unsigned long addr,
>>>>             walk->action = ACTION_AGAIN;
>>>>             return 0;
>>>>         }
>>>> -    for (; addr != end; pte++, addr += PAGE_SIZE) {
>>>> +    for (; addr != end; pte += nr, addr += nr * PAGE_SIZE) {
>>>> +        max_nr = (end - addr) >> PAGE_SHIFT;
>>>> +        nr = 1;
>>>>             ptent = ptep_get(pte);
>>>>             if (pte_none(ptent))
>>>>                 continue;
>>>> @@ -598,6 +602,10 @@ static int queue_folios_pte_range(pmd_t *pmd,
>>>> unsigned long addr,
>>>>             folio = vm_normal_folio(vma, addr, ptent);
>>>>             if (!folio || folio_is_zone_device(folio))
>>>>                 continue;
>>>> +        if (folio_test_large(folio) && max_nr != 1)
>>>> +            nr = folio_pte_batch(folio, addr, pte, ptent,
>>>> +                         max_nr, fpb_flags,
>>>> +                         NULL, NULL, NULL);
>>>>             /*
>>>>              * vm_normal_folio() filters out zero pages, but there might
>>>>              * still be reserved folios to skip, perhaps in a VDSO.
>>>> @@ -630,7 +638,7 @@ static int queue_folios_pte_range(pmd_t *pmd,
>>>> unsigned long addr,
>>>>             if (!(flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) ||
>>>>                 !vma_migratable(vma) ||
>>>>                 !migrate_folio_add(folio, qp->pagelist, flags)) {
>>>> -            qp->nr_failed++;
>>>> +            qp->nr_failed += nr;
>>>
>>> Sorry for chiming in late, but I am not convinced that 'qp->nr_failed'
>>> should add 'nr' when isolation fails.
>>
>> This patch does not change the existing behavior. But I stumbled over
>> that as well ... and scratched my head.
>>
>>>
>>>    From the comments of queue_pages_range():
>>> "
>>> * >0 - this number of misplaced folios could not be queued for moving
>>>     *      (a hugetlbfs page or a transparent huge page being counted
>>> as 1).
>>> "
>>>
>>> That means if a large folio is failed to isolate, we should only add '1'
>>> for qp->nr_failed instead of the number of pages in this large folio.
>>> Right?
>>
>> I think what the doc really meant is "PMD-mapped THP". PTE-mapped THPs
>> always had the same behavior: per PTE of the THP we would increment
>> nr_failed by 1.
> 
> No? For pte-mapped THPs, it only adds 1 for the large folio, since we
> have below check in queue_folios_pte_range().
> 
> if (folio == qp->large)
> 	continue;
> 
> Or I missed anything else?

Ah, I got confused by that and thought it would only be for LRU 
isolation purposes.

Yeah, it will kind-of work for now and I think you are correct that we 
would only increment nr_failed by 1.

I still think that counting nr_failed that way is dubious. We should be 
counting pages, which is something that user space from migrate_pages() 
could understand. Having it count arbitrary THPs/large folio sizes is 
really questionable.

But that is indeed a separate issue to resolve.

-- 
Cheers,

David / dhildenb

Re: [PATCH v3] mempolicy: Optimize queue_folios_pte_range by PTE batching

[David Hildenbrand]

On 16.04.25 10:51, David Hildenbrand wrote:
> On 16.04.25 10:41, Baolin Wang wrote:
>>
>>
>> On 2025/4/16 16:22, David Hildenbrand wrote:
>>> On 16.04.25 08:32, Baolin Wang wrote:
>>>>
>>>>
>>>> On 2025/4/16 13:30, Dev Jain wrote:
>>>>> After the check for queue_folio_required(), the code only cares about
>>>>> the
>>>>> folio in the for loop, i.e the PTEs are redundant. Therefore, optimize
>>>>> this loop by skipping over a PTE batch mapping the same folio.
>>>>>
>>>>> With a test program migrating pages of the calling process, which
>>>>> includes
>>>>> a mapped VMA of size 4GB with pte-mapped large folios of order-9, and
>>>>> migrating once back and forth node-0 and node-1, the average execution
>>>>> time reduces from 7.5 to 4 seconds, giving an approx 47% speedup.
>>>>>
>>>>> v2->v3:
>>>>>      - Don't use assignment in if condition
>>>>>
>>>>> v1->v2:
>>>>>      - Follow reverse xmas tree declarations
>>>>>      - Don't initialize nr
>>>>>      - Move folio_pte_batch() immediately after retrieving a normal folio
>>>>>      - increment nr_failed in one shot
>>>>>
>>>>> Acked-by: David Hildenbrand <david@redhat.com>
>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>> ---
>>>>>      mm/mempolicy.c | 12 ++++++++++--
>>>>>      1 file changed, 10 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>>>>> index b28a1e6ae096..4d2dc8b63965 100644
>>>>> --- a/mm/mempolicy.c
>>>>> +++ b/mm/mempolicy.c
>>>>> @@ -566,6 +566,7 @@ static void queue_folios_pmd(pmd_t *pmd, struct
>>>>> mm_walk *walk)
>>>>>      static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>                  unsigned long end, struct mm_walk *walk)
>>>>>      {
>>>>> +    const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>          struct vm_area_struct *vma = walk->vma;
>>>>>          struct folio *folio;
>>>>>          struct queue_pages *qp = walk->private;
>>>>> @@ -573,6 +574,7 @@ static int queue_folios_pte_range(pmd_t *pmd,
>>>>> unsigned long addr,
>>>>>          pte_t *pte, *mapped_pte;
>>>>>          pte_t ptent;
>>>>>          spinlock_t *ptl;
>>>>> +    int max_nr, nr;
>>>>>          ptl = pmd_trans_huge_lock(pmd, vma);
>>>>>          if (ptl) {
>>>>> @@ -586,7 +588,9 @@ static int queue_folios_pte_range(pmd_t *pmd,
>>>>> unsigned long addr,
>>>>>              walk->action = ACTION_AGAIN;
>>>>>              return 0;
>>>>>          }
>>>>> -    for (; addr != end; pte++, addr += PAGE_SIZE) {
>>>>> +    for (; addr != end; pte += nr, addr += nr * PAGE_SIZE) {
>>>>> +        max_nr = (end - addr) >> PAGE_SHIFT;
>>>>> +        nr = 1;
>>>>>              ptent = ptep_get(pte);
>>>>>              if (pte_none(ptent))
>>>>>                  continue;
>>>>> @@ -598,6 +602,10 @@ static int queue_folios_pte_range(pmd_t *pmd,
>>>>> unsigned long addr,
>>>>>              folio = vm_normal_folio(vma, addr, ptent);
>>>>>              if (!folio || folio_is_zone_device(folio))
>>>>>                  continue;
>>>>> +        if (folio_test_large(folio) && max_nr != 1)
>>>>> +            nr = folio_pte_batch(folio, addr, pte, ptent,
>>>>> +                         max_nr, fpb_flags,
>>>>> +                         NULL, NULL, NULL);
>>>>>              /*
>>>>>               * vm_normal_folio() filters out zero pages, but there might
>>>>>               * still be reserved folios to skip, perhaps in a VDSO.
>>>>> @@ -630,7 +638,7 @@ static int queue_folios_pte_range(pmd_t *pmd,
>>>>> unsigned long addr,
>>>>>              if (!(flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) ||
>>>>>                  !vma_migratable(vma) ||
>>>>>                  !migrate_folio_add(folio, qp->pagelist, flags)) {
>>>>> -            qp->nr_failed++;
>>>>> +            qp->nr_failed += nr;
>>>>
>>>> Sorry for chiming in late, but I am not convinced that 'qp->nr_failed'
>>>> should add 'nr' when isolation fails.
>>>
>>> This patch does not change the existing behavior. But I stumbled over
>>> that as well ... and scratched my head.
>>>
>>>>
>>>>     From the comments of queue_pages_range():
>>>> "
>>>> * >0 - this number of misplaced folios could not be queued for moving
>>>>      *      (a hugetlbfs page or a transparent huge page being counted
>>>> as 1).
>>>> "
>>>>
>>>> That means if a large folio is failed to isolate, we should only add '1'
>>>> for qp->nr_failed instead of the number of pages in this large folio.
>>>> Right?
>>>
>>> I think what the doc really meant is "PMD-mapped THP". PTE-mapped THPs
>>> always had the same behavior: per PTE of the THP we would increment
>>> nr_failed by 1.
>>
>> No? For pte-mapped THPs, it only adds 1 for the large folio, since we
>> have below check in queue_folios_pte_range().
>>
>> if (folio == qp->large)
>> 	continue;
>>
>> Or I missed anything else?
> 
> Ah, I got confused by that and thought it would only be for LRU
> isolation purposes.
> 
> Yeah, it will kind-of work for now and I think you are correct that we
> would only increment nr_failed by 1.
> 
> I still think that counting nr_failed that way is dubious. We should be
> counting pages, which is something that user space from migrate_pages()
> could understand. Having it count arbitrary THPs/large folio sizes is
> really questionable.
> 
> But that is indeed a separate issue to resolve.

Digging into it:

commit 1cb5d11a370f661c5d0d888bb0cfc2cdc5791382
Author: Hugh Dickins <hughd@google.com>
Date:   Tue Oct 3 02:17:43 2023 -0700

     mempolicy: fix migrate_pages(2) syscall return nr_failed
     
     "man 2 migrate_pages" says "On success migrate_pages() returns the number
     of pages that could not be moved".  Although 5.3 and 5.4 commits fixed
     mbind(MPOL_MF_STRICT|MPOL_MF_MOVE*) to fail with EIO when not all pages
     could be moved (because some could not be isolated for migration),
     migrate_pages(2) was left still reporting only those pages failing at the
     migration stage, forgetting those failing at the earlier isolation stage.
     
     Fix that by accumulating a long nr_failed count in struct queue_pages,
     returned by queue_pages_range() when it's not returning an error, for
     adding on to the nr_failed count from migrate_pages() in mm/migrate.c.  A
     count of pages?  It's more a count of folios, but changing it to pages
     would entail more work (also in mm/migrate.c): does not seem justified.

Yeah, we should be counting pages, but likely nobody really cares, because we
only care if everything was migrated or something was not migrated.

-- 
Cheers,

David / dhildenb

Re: [PATCH v3] mempolicy: Optimize queue_folios_pte_range by PTE batching

[Baolin Wang]

On 2025/4/16 16:56, David Hildenbrand wrote:
> On 16.04.25 10:51, David Hildenbrand wrote:
>> On 16.04.25 10:41, Baolin Wang wrote:
>>>
>>>
>>> On 2025/4/16 16:22, David Hildenbrand wrote:
>>>> On 16.04.25 08:32, Baolin Wang wrote:
>>>>>
>>>>>
>>>>> On 2025/4/16 13:30, Dev Jain wrote:
>>>>>> After the check for queue_folio_required(), the code only cares about
>>>>>> the
>>>>>> folio in the for loop, i.e the PTEs are redundant. Therefore, 
>>>>>> optimize
>>>>>> this loop by skipping over a PTE batch mapping the same folio.
>>>>>>
>>>>>> With a test program migrating pages of the calling process, which
>>>>>> includes
>>>>>> a mapped VMA of size 4GB with pte-mapped large folios of order-9, and
>>>>>> migrating once back and forth node-0 and node-1, the average 
>>>>>> execution
>>>>>> time reduces from 7.5 to 4 seconds, giving an approx 47% speedup.
>>>>>>
>>>>>> v2->v3:
>>>>>>      - Don't use assignment in if condition
>>>>>>
>>>>>> v1->v2:
>>>>>>      - Follow reverse xmas tree declarations
>>>>>>      - Don't initialize nr
>>>>>>      - Move folio_pte_batch() immediately after retrieving a 
>>>>>> normal folio
>>>>>>      - increment nr_failed in one shot
>>>>>>
>>>>>> Acked-by: David Hildenbrand <david@redhat.com>
>>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>>> ---
>>>>>>      mm/mempolicy.c | 12 ++++++++++--
>>>>>>      1 file changed, 10 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>>>>>> index b28a1e6ae096..4d2dc8b63965 100644
>>>>>> --- a/mm/mempolicy.c
>>>>>> +++ b/mm/mempolicy.c
>>>>>> @@ -566,6 +566,7 @@ static void queue_folios_pmd(pmd_t *pmd, struct
>>>>>> mm_walk *walk)
>>>>>>      static int queue_folios_pte_range(pmd_t *pmd, unsigned long 
>>>>>> addr,
>>>>>>                  unsigned long end, struct mm_walk *walk)
>>>>>>      {
>>>>>> +    const fpb_t fpb_flags = FPB_IGNORE_DIRTY | 
>>>>>> FPB_IGNORE_SOFT_DIRTY;
>>>>>>          struct vm_area_struct *vma = walk->vma;
>>>>>>          struct folio *folio;
>>>>>>          struct queue_pages *qp = walk->private;
>>>>>> @@ -573,6 +574,7 @@ static int queue_folios_pte_range(pmd_t *pmd,
>>>>>> unsigned long addr,
>>>>>>          pte_t *pte, *mapped_pte;
>>>>>>          pte_t ptent;
>>>>>>          spinlock_t *ptl;
>>>>>> +    int max_nr, nr;
>>>>>>          ptl = pmd_trans_huge_lock(pmd, vma);
>>>>>>          if (ptl) {
>>>>>> @@ -586,7 +588,9 @@ static int queue_folios_pte_range(pmd_t *pmd,
>>>>>> unsigned long addr,
>>>>>>              walk->action = ACTION_AGAIN;
>>>>>>              return 0;
>>>>>>          }
>>>>>> -    for (; addr != end; pte++, addr += PAGE_SIZE) {
>>>>>> +    for (; addr != end; pte += nr, addr += nr * PAGE_SIZE) {
>>>>>> +        max_nr = (end - addr) >> PAGE_SHIFT;
>>>>>> +        nr = 1;
>>>>>>              ptent = ptep_get(pte);
>>>>>>              if (pte_none(ptent))
>>>>>>                  continue;
>>>>>> @@ -598,6 +602,10 @@ static int queue_folios_pte_range(pmd_t *pmd,
>>>>>> unsigned long addr,
>>>>>>              folio = vm_normal_folio(vma, addr, ptent);
>>>>>>              if (!folio || folio_is_zone_device(folio))
>>>>>>                  continue;
>>>>>> +        if (folio_test_large(folio) && max_nr != 1)
>>>>>> +            nr = folio_pte_batch(folio, addr, pte, ptent,
>>>>>> +                         max_nr, fpb_flags,
>>>>>> +                         NULL, NULL, NULL);
>>>>>>              /*
>>>>>>               * vm_normal_folio() filters out zero pages, but 
>>>>>> there might
>>>>>>               * still be reserved folios to skip, perhaps in a VDSO.
>>>>>> @@ -630,7 +638,7 @@ static int queue_folios_pte_range(pmd_t *pmd,
>>>>>> unsigned long addr,
>>>>>>              if (!(flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) ||
>>>>>>                  !vma_migratable(vma) ||
>>>>>>                  !migrate_folio_add(folio, qp->pagelist, flags)) {
>>>>>> -            qp->nr_failed++;
>>>>>> +            qp->nr_failed += nr;
>>>>>
>>>>> Sorry for chiming in late, but I am not convinced that 'qp->nr_failed'
>>>>> should add 'nr' when isolation fails.
>>>>
>>>> This patch does not change the existing behavior. But I stumbled over
>>>> that as well ... and scratched my head.
>>>>
>>>>>
>>>>>     From the comments of queue_pages_range():
>>>>> "
>>>>> * >0 - this number of misplaced folios could not be queued for moving
>>>>>      *      (a hugetlbfs page or a transparent huge page being counted
>>>>> as 1).
>>>>> "
>>>>>
>>>>> That means if a large folio is failed to isolate, we should only 
>>>>> add '1'
>>>>> for qp->nr_failed instead of the number of pages in this large folio.
>>>>> Right?
>>>>
>>>> I think what the doc really meant is "PMD-mapped THP". PTE-mapped THPs
>>>> always had the same behavior: per PTE of the THP we would increment
>>>> nr_failed by 1.
>>>
>>> No? For pte-mapped THPs, it only adds 1 for the large folio, since we
>>> have below check in queue_folios_pte_range().
>>>
>>> if (folio == qp->large)
>>>     continue;
>>>
>>> Or I missed anything else?
>>
>> Ah, I got confused by that and thought it would only be for LRU
>> isolation purposes.
>>
>> Yeah, it will kind-of work for now and I think you are correct that we
>> would only increment nr_failed by 1.
>>
>> I still think that counting nr_failed that way is dubious. We should be
>> counting pages, which is something that user space from migrate_pages()
>> could understand. Having it count arbitrary THPs/large folio sizes is
>> really questionable.
>>
>> But that is indeed a separate issue to resolve.
> 
> Digging into it:
> 
> commit 1cb5d11a370f661c5d0d888bb0cfc2cdc5791382
> Author: Hugh Dickins <hughd@google.com>
> Date:   Tue Oct 3 02:17:43 2023 -0700
> 
>      mempolicy: fix migrate_pages(2) syscall return nr_failed
>      "man 2 migrate_pages" says "On success migrate_pages() returns the 
> number
>      of pages that could not be moved".  Although 5.3 and 5.4 commits fixed
>      mbind(MPOL_MF_STRICT|MPOL_MF_MOVE*) to fail with EIO when not all 
> pages
>      could be moved (because some could not be isolated for migration),
>      migrate_pages(2) was left still reporting only those pages failing 
> at the
>      migration stage, forgetting those failing at the earlier isolation 
> stage.
>      Fix that by accumulating a long nr_failed count in struct queue_pages,
>      returned by queue_pages_range() when it's not returning an error, for
>      adding on to the nr_failed count from migrate_pages() in 
> mm/migrate.c.  A
>      count of pages?  It's more a count of folios, but changing it to pages
>      would entail more work (also in mm/migrate.c): does not seem 
> justified.
> 
> Yeah, we should be counting pages, but likely nobody really cares, 
> because we
> only care if everything was migrated or something was not migrated.

Agree. Like you said, we need a separate patch to do some cleanup for this.