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

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

On 15/04/25 3:47 pm, David Hildenbrand wrote:
> On 11.04.25 10:13, 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.
>>
>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>> ---
>> Unfortunately I have only build tested this since my test environment is
>> broken.
>>
>>   mm/mempolicy.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index b28a1e6ae096..b019524da8a2 100644
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -573,6 +573,9 @@ 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;
>> +    const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>> +    int nr = 1;
> 
> Try sticking to reverse xmas tree, please. (not completely the case 
> here, but fpb_flags can easily be moved all he way to the top)

I thought that the initializations were to be kept at the bottom.
Asking for future patches, should I put all declarations in reverse-xmas 
fashion (even those which I don't intend to touch w.r.t the patch 
logic), or do I do that for only my additions?

> 
> Also, why are you initializing nr to 1 here if you reinitialize it below?

Yup no need, I thought pte += nr will blow up due to nr not being 
initialized, but it won't because it gets executed just before the start 
of the second iteration.

> 
>  >       ptl = pmd_trans_huge_lock(pmd, vma);>       if (ptl) {
>> @@ -586,7 +589,8 @@ 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) {
>> +        nr = 1;
>>           ptent = ptep_get(pte);
>>           if (pte_none(ptent))
>>               continue;
>> @@ -607,6 +611,11 @@ static int queue_folios_pte_range(pmd_t *pmd, 
>> unsigned long addr,
>>           if (!queue_folio_required(folio, qp))
>>               continue;
>>           if (folio_test_large(folio)) {
>> +            max_nr = (end - addr) >> PAGE_SHIFT;
>> +            if (max_nr != 1)
>> +                nr = folio_pte_batch(folio, addr, pte, ptent,
>> +                             max_nr, fpb_flags,
>> +                             NULL, NULL, NULL);
> 
> We should probably do that immediately after we verified that 
> vm_normal_folio() have us something reasonable.

But shouldn't we keep the small folio case separate to avoid the 
overhead of folio_pte_batch()?

> 
>>               /*
>>                * A large folio can only be isolated from LRU once,
>>                * but may be mapped by many PTEs (and Copy-On-Write may
>> @@ -633,6 +642,7 @@ static int queue_folios_pte_range(pmd_t *pmd, 
>> unsigned long addr,
>>               qp->nr_failed++;
>>               if (strictly_unmovable(flags))
>>                   break;
>> +            qp->nr_failed += nr - 1;
> 
> Can't we do qp->nr_failed += nr; above?

I did not dive deep into the significance of nr_failed, but I did that
to keep the code, before and after the change, equivalent:

Claim: if we reach qp->nr_failed++ for a single pte, we will reach here 
for all ptes belonging to the same batch.

Proof: We reach here => the if condition is true. Now, !(flags & 
(MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) and !vma_migratable(vma) do not 
depend on the ptes. So the other case is that !migrate_folio_add() is 
true => !folio_isolate_lru() is true, which depends on the folio and not 
the PTEs; if isolation fails for one PTE, it will definitely fail for 
the PTE batch.

So, before the change, if we iterate on a pte mapping a large folio, and 
strictly_unmovable(flags) is true, then nr_failed += 1 only. If not, 
then nr_failed++ will happen nr times for sure (because of the claim) 
and we can safely do qp->nr_failed += nr - 1.

> 
> Weird enough, queue_folios_pmd() also only does qp->nr_failed++, but 
> queue_pages_range() documents it that way.
> 
>>           }
>>       }
>>       pte_unmap_unlock(mapped_pte, ptl);
> 
>