linux-kernel - Re: [PATCH] mempolicy: Optimize queue_folios_pte

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fa4ee2f1-223d-4af3-8e3e-a1371688a548@linux.alibaba.com>
Date: Wed, 16 Apr 2025 15:33:08 +0800
From: Baolin Wang <baolin.wang@...ux.alibaba.com>
To: Dev Jain <dev.jain@....com>, David Hildenbrand <david@...hat.com>,
 akpm@...ux-foundation.org
Cc: ryan.roberts@....com, willy@...radead.org, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, hughd@...gle.com, vishal.moola@...il.com,
 yang@...amperecomputing.com, ziy@...dia.com
Subject: Re: [PATCH] mempolicy: Optimize queue_folios_pte_range by PTE
 batching



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@....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;
		}