| 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
| ||
|
Message-ID: <c07d1917-a460-440a-b843-a6866a87d261@arm.com>
Date: Fri, 20 Sep 2024 09:34:53 +0530
From: Dev Jain <dev.jain@....com>
To: Ryan Roberts <ryan.roberts@....com>, Barry Song <baohua@...nel.org>
Cc: akpm@...ux-foundation.org, david@...hat.com, willy@...radead.org,
anshuman.khandual@....com, hughd@...gle.com, ioworker0@...il.com,
wangkefeng.wang@...wei.com, baolin.wang@...ux.alibaba.com, gshan@...hat.com,
linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v2 2/2] mm: Compute first_set_pte to eliminate evaluating
redundant ranges
On 9/19/24 22:25, Ryan Roberts wrote:
> On 19/09/2024 09:40, Dev Jain wrote:
>> On 9/19/24 07:04, Barry Song wrote:
>>> On Mon, Sep 16, 2024 at 11:08 PM Dev Jain <dev.jain@....com> wrote:
>>>> For an mTHP allocation, we need to check, for every order, whether for
>>>> that order, we have enough number of contiguous PTEs empty. Instead of
>>>> iterating the while loop for every order, use some information, which
>>>> is the first set PTE found, from the previous iteration to eliminate
>>>> some cases. The key to understanding the correctness of the patch
>>>> is that the ranges we want to examine form a strictly decreasing
>>>> sequence of nested intervals.
>>> Could we include some benchmark data here, as suggested by Ryan in this thread?
>>>
>>> https://lore.kernel.org/linux-mm/58f91a56-890a-45d0-8b1f-47c4c70c9600@arm.com/
>> Can you please verify and get some numbers for the following program,
>> because if I am doing this correctly, it would be a regression :)
>> https://www.codedump.xyz/cpp/Zuvf8FwvRPH21UO2
> Some brief comments on the test code:
>
> - You don't need to enable/disable the top-level control. Regardless, I don't
> think this breaks the benchmark.
>
> - I think you have an off-by-1 in your for loop condition:
>
> for (unsigned long i = 1; (i * border) < size; ++i) {
>
> I think this needs to be:
>
> for (unsigned long i = 1; (i * border) <= size; ++i) {
>
> It just means that the final 32K block will get a single 32K mapping.
>
> - You're measuring the whole program; including mmap/munmap and
> enabling/disabling mTHP. It would be much better to just measure the loop that
> writes to each page after mTHP is enabled.
>
>
> I modified the code to iterate for 10 seconds and on each iteration, measure
> only the time spent in the interesting loop. Running on Apple M2 VM:
>
> Before the change:
>
> ubuntu@...ntuvm:~/scan-pte$ sudo ./scan-pte
> Average: 0.070028 seconds per GB (iterations=98)
> ubuntu@...ntuvm:~/scan-pte$ sudo ./scan-pte
> Average: 0.068495 seconds per GB (iterations=96)
> ubuntu@...ntuvm:~/scan-pte$ sudo ./scan-pte
> Average: 0.070207 seconds per GB (iterations=93)
>
> After the change:
>
> ubuntu@...ntuvm:~/scan-pte$ sudo ./scan-pte
> Average: 0.076923 seconds per GB (iterations=88)
> ubuntu@...ntuvm:~/scan-pte$ sudo ./scan-pte
> Average: 0.072206 seconds per GB (iterations=96)
> ubuntu@...ntuvm:~/scan-pte$ sudo ./scan-pte
> Average: 0.072397 seconds per GB (iterations=89)
>
> So this looks pretty clearly slower to me. So suggest we shouldn't take this patch.
Thanks for testing!
>
>
>> The program does this: disable THP completely -> mmap 1G VMA -> touch the last
>> page of a 32K sized boundary. That is, 0th till 32K/4K - 2 pages are
>> empty, while the 32K/4K - 1'th page is touched, and so on -> madvise
>> the entire VMA -> enable all THPs except 2M -> touch all pages.
>>
>> Therefore, we have 0 - 6 PTEs empty, 7th is filled, and so on. Eventually,
>> kernel will fall down to finding 4 contiguous PTEs empty and allocate
>> 4K * 4 = 16K mTHP.
>>
>> The result without the patches:
>>
>> real: 8.250s
>> user: 0.941s
>> sys: 7.077s
>>
>> real: 8.175s
>> user: 0.939s
>> sys: 7.021s
>>
>> With the patches:
>>
>> real: 8.584s
>> user: 1.089s
>> sys: 7.234s
>>
>> real: 8.429s
>> user: 0.954s
>> sys: 7.220s
> What HW did you measure this on? I'm guessing this is measuring multiple
> iterations, otherwise it looks extremely slow. If you were measuring on FVP, for
> example, that would not give representative performance numbers.
I measured with qemu.
>
> Thanks,
> Ryan
>
>> You can change the #iterations in the for loop to magnify this,
>> and the current code surprisingly wins.
>>
>>
>>>> Suggested-by: Ryan Roberts <ryan.roberts@....com>
>>>> Signed-off-by: Dev Jain <dev.jain@....com>
>>>> ---
>>>> mm/memory.c | 20 ++++++++++++++++++--
>>>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>> index 8bb1236de93c..e81c6abe09ce 100644
>>>> --- a/mm/memory.c
>>>> +++ b/mm/memory.c
>>>> @@ -4633,10 +4633,11 @@ static struct folio *alloc_anon_folio(struct vm_fault
>>>> *vmf)
>>>> {
>>>> struct vm_area_struct *vma = vmf->vma;
>>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>> + pte_t *first_set_pte = NULL, *align_pte, *pte;
>>>> unsigned long orders;
>>>> struct folio *folio;
>>>> unsigned long addr;
>>>> - pte_t *pte;
>>>> + int max_empty;
>>>> gfp_t gfp;
>>>> int order;
>>>>
>>>> @@ -4671,8 +4672,23 @@ static struct folio *alloc_anon_folio(struct vm_fault
>>>> *vmf)
>>>> order = highest_order(orders);
>>>> while (orders) {
>>>> addr = ALIGN_DOWN(vmf->address, PAGE_SIZE << order);
>>>> - if (pte_range_none(pte + pte_index(addr), 1 << order) == 1 <<
>>>> order)
>>>> + align_pte = pte + pte_index(addr);
>>>> +
>>>> + /* Range to be scanned known to be empty */
>>>> + if (align_pte + (1 << order) <= first_set_pte)
>>>> + break;
>>>> +
>>>> + /* Range to be scanned contains first_set_pte */
>>>> + if (align_pte <= first_set_pte)
>>>> + goto repeat;
>>>> +
>>>> + /* align_pte > first_set_pte, so need to check properly */
>>>> + max_empty = pte_range_none(align_pte, 1 << order);
>>>> + if (max_empty == 1 << order)
>>>> break;
>>>> +
>>>> + first_set_pte = align_pte + max_empty;
>>>> +repeat:
>>>> order = next_order(&orders, order);
>>>> }
>>>>
>>>> --
>>>> 2.30.2
>>>>
>>> Thanks
>>> barry
Powered by blists - more mailing lists