Re: [PATCH v2 2/3] mm: mprotect: avoid unnecessary struct page accessing if pte_protnone()

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2025/10/15 15:32, Dev Jain wrote:
> 
> On 14/10/25 5:03 pm, Kefeng Wang wrote:
>> If the pte_protnone() is true, we could avoid unnecessary struct page
>> accessing and reduce cache footprint when scanning page tables for prot
>> numa, the performance test of pmbench memory accessing benchmark
>> should be benifit, see more commit a818f5363a0e ("autonuma: reduce cache
>> footprint when scanning page tables").
>>
>> Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
> 
> Thanks.
> 
>>   mm/mprotect.c | 29 ++++++++++++-----------------
>>   1 file changed, 12 insertions(+), 17 deletions(-)
>>
>> diff --git a/mm/mprotect.c b/mm/mprotect.c
>> index bb59a42809b8..7affa88a6de7 100644
>> --- a/mm/mprotect.c
>> +++ b/mm/mprotect.c
>> @@ -118,18 +118,13 @@ static int mprotect_folio_pte_batch(struct folio 
>> *folio, pte_t *ptep,
>>       return folio_pte_batch_flags(folio, NULL, ptep, &pte, 
>> max_nr_ptes, flags);
>>   }
>> -static bool prot_numa_skip(struct vm_area_struct *vma, unsigned long 
>> addr,
>> -               pte_t oldpte, pte_t *pte, int target_node,
>> -               struct folio *folio)
>> +static bool prot_numa_skip(struct vm_area_struct *vma, int target_node,
>> +        struct folio *folio)
>>   {
>>       bool ret = true;
>>       bool toptier;
>>       int nid;
>> -    /* Avoid TLB flush if possible */
>> -    if (pte_protnone(oldpte))
>> -        goto skip;
>> -
>>       if (!folio)
>>           goto skip;
>> @@ -307,23 +302,23 @@ static long change_pte_range(struct mmu_gather 
>> *tlb,
>>               struct page *page;
>>               pte_t ptent;
>> +            /* Already in the desired state. */
>> +            if (prot_numa && pte_protnone(oldpte))
>> +                continue;
>> +
>>               page = vm_normal_page(vma, addr, oldpte);
>>               if (page)
>>                   folio = page_folio(page);
>> +
> 
> Unrelated change but I guess this is needed since we usually leave a line
> before starting a comment.
> 
>>               /*
>>                * Avoid trapping faults against the zero or KSM
>>                * pages. See similar comment in change_huge_pmd.
>>                */
>> -            if (prot_numa) {
>> -                int ret = prot_numa_skip(vma, addr, oldpte, pte,
>> -                             target_node, folio);
>> -                if (ret) {
>> -
>> -                    /* determine batch to skip */
>> -                    nr_ptes = mprotect_folio_pte_batch(folio,
>> -                          pte, oldpte, max_nr_ptes, /* flags = */ 0);
>> -                    continue;
>> -                }
>> +            if (prot_numa & prot_numa_skip(vma, target_node, folio)) {
> 
> Why not "&&" instead of "&"?

oops，will fix it.

> 
>> +                /* determine batch to skip */
>> +                nr_ptes = mprotect_folio_pte_batch(folio,
>> +                      pte, oldpte, max_nr_ptes, /* flags = */ 0);
>> +                continue;
>>               }
>>               nr_ptes = mprotect_folio_pte_batch(folio, pte, oldpte, 
>> max_nr_ptes, flags);
>