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

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

On 16/04/25 1:03 pm, Baolin Wang wrote:
> 
> 
> On 2025/4/15 19:47, Dev Jain wrote:
>>
>>
>> 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.
> 
> Sorry, I missed the previous discussion (I replied to your new version). 
> I think this claim is incorrect, we will skip remaining ptes belonging 
> to the same batch with checking 'qp->large'.
> 
>          if (folio_test_large(folio)) {
>              if (folio == qp->large)
>                  continue;
>              qp->large = folio;
>          }

Oops you are right, I missed that.