Re: [PATCH 2/2] mm: mincore: use folio_pte_batch() to batch process large folios

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2025/4/1 21:04, David Hildenbrand wrote:
> On 01.04.25 12:45, Ryan Roberts wrote:
>> On 30/03/2025 15:57, Baolin Wang wrote:
>>>
>>>
>>> On 2025/3/27 22:08, Ryan Roberts wrote:
>>>> On 25/03/2025 23:38, Baolin Wang wrote:
>>>>> When I tested the mincore() syscall, I observed that it takes 
>>>>> longer with
>>>>> 64K mTHP enabled on my Arm64 server. The reason is the 
>>>>> mincore_pte_range()
>>>>> still checks each PTE individually, even when the PTEs are contiguous,
>>>>> which is not efficient.
>>>>>
>>>>> Thus we can use folio_pte_batch() to get the batch number of the 
>>>>> present
>>>>> contiguous PTEs, which can improve the performance. I tested the 
>>>>> mincore()
>>>>> syscall with 1G anonymous memory populated with 64K mTHP, and 
>>>>> observed an
>>>>> obvious performance improvement:
>>>>>
>>>>> w/o patch        w/ patch        changes
>>>>> 6022us            1115us            +81%
>>>>>
>>>>> Moreover, I also tested mincore() with disabling mTHP/THP, and did not
>>>>> see any obvious regression.
>>>>>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>>>> ---
>>>>>    mm/mincore.c | 27 ++++++++++++++++++++++-----
>>>>>    1 file changed, 22 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/mm/mincore.c b/mm/mincore.c
>>>>> index 832f29f46767..88be180b5550 100644
>>>>> --- a/mm/mincore.c
>>>>> +++ b/mm/mincore.c
>>>>> @@ -21,6 +21,7 @@
>>>>>      #include <linux/uaccess.h>
>>>>>    #include "swap.h"
>>>>> +#include "internal.h"
>>>>>      static int mincore_hugetlb(pte_t *pte, unsigned long hmask, 
>>>>> unsigned long
>>>>> addr,
>>>>>                unsigned long end, struct mm_walk *walk)
>>>>> @@ -105,6 +106,7 @@ static int mincore_pte_range(pmd_t *pmd, 
>>>>> unsigned long
>>>>> addr, unsigned long end,
>>>>>        pte_t *ptep;
>>>>>        unsigned char *vec = walk->private;
>>>>>        int nr = (end - addr) >> PAGE_SHIFT;
>>>>> +    int step, i;
>>>>>          ptl = pmd_trans_huge_lock(pmd, vma);
>>>>>        if (ptl) {
>>>>> @@ -118,16 +120,31 @@ static int mincore_pte_range(pmd_t *pmd, 
>>>>> unsigned long
>>>>> addr, unsigned long end,
>>>>>            walk->action = ACTION_AGAIN;
>>>>>            return 0;
>>>>>        }
>>>>> -    for (; addr != end; ptep++, addr += PAGE_SIZE) {
>>>>> +    for (; addr != end; ptep += step, addr += step * PAGE_SIZE) {
>>>>>            pte_t pte = ptep_get(ptep);
>>>>>    +        step = 1;
>>>>>            /* We need to do cache lookup too for pte markers */
>>>>>            if (pte_none_mostly(pte))
>>>>>                __mincore_unmapped_range(addr, addr + PAGE_SIZE,
>>>>>                             vma, vec);
>>>>> -        else if (pte_present(pte))
>>>>> -            *vec = 1;
>>>>> -        else { /* pte is a swap entry */
>>>>> +        else if (pte_present(pte)) {
>>>>> +            if (pte_batch_hint(ptep, pte) > 1) {
>>>>> +                struct folio *folio = vm_normal_folio(vma, addr, 
>>>>> pte);
>>>>> +
>>>>> +                if (folio && folio_test_large(folio)) {
>>>>> +                    const fpb_t fpb_flags = FPB_IGNORE_DIRTY |
>>>>> +                                FPB_IGNORE_SOFT_DIRTY;
>>>>> +                    int max_nr = (end - addr) / PAGE_SIZE;
>>>>> +
>>>>> +                    step = folio_pte_batch(folio, addr, ptep, pte,
>>>>> +                            max_nr, fpb_flags, NULL, NULL, NULL);
>>>>> +                }
>>>>> +            }
>>>>
>>>> You could simplify to the following, I think, to avoid needing to 
>>>> grab the folio
>>>> and call folio_pte_batch():
>>>>
>>>>              else if (pte_present(pte)) {
>>>>                  int max_nr = (end - addr) / PAGE_SIZE;
>>>>                  step = min(pte_batch_hint(ptep, pte), max_nr);
>>>>              } ...
>>>>
>>>> I expect the regression you are seeing here is all due to calling 
>>>> ptep_get() for
>>>> every pte in the contpte batch, which will cause 16 memory reads per 
>>>> pte (to
>>>> gather the access/dirty bits). For small folios its just 1 read per 
>>>> pte.
>>>
>>> Right.
>>>
>>>> pte_batch_hint() will skip forward in blocks of 16 so you now end up 
>>>> with the
>>>> same number as for the small folio case. You don't need all the 
>>>> fancy extras
>>>> that folio_pte_batch() gives you here.
>>>
>>> Sounds reasonable. Your suggestion looks simple, but my method can 
>>> batch the
>>> whole large folio (such as large folios containing more than 16 
>>> contiguous PTEs)
>>> at once.
>>
>> Sure but folio_pte_batch() just implements that with another loop that 
>> calls
>> pte_batch_hint(), so it all amounts to the same thing. In fact there 
>> are some
>> extra checks in folio_pte_batch() that you don't need so it might be a 
>> bit slower.

Right. I tested your suggestion, yes, much better.

> I don't enjoy open-coding the batching, especially if only cont-pte 
> users will benefit from it. But I also don't enjoy the open-coded 
> pte_batch_hint() :)
> 
> But we really don't need the folio here, so I assume the short variant 
> you (Ryan) suggest is alright to just avoid the ptep_get().
> 
> As Oscar says, these details might soon be hidden inside a new page 
> table walker API (even though it will likely end up using 
> folio_pte_batch() internally, TBD).

OK. I can drop this patch if it will be addressed in the following patches.