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

[David Hildenbrand <david@redhat.com>]

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