Re: [PATCH v2 1/2] mm: use aligned address in clear_gigantic_page()

Huang, Ying
2024-11-01T06:22:35Z

Kefeng Wang <wangkefeng.wang@huawei.com> writes:

> On 2024/10/30 11:21, Huang, Ying wrote:
>> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>>
>>> On 2024/10/30 9:04, Huang, Ying wrote:
>>>> David Hildenbrand <david@redhat.com> writes:
>>>>
>>>>> On 29.10.24 14:04, Kefeng Wang wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> That should all be cleaned up ... process_huge_page() likely
>>>>>>>>>>>>> shouldn't
>>>>>>>>>>>>
>>>>>>>>>>>> Yes, let's fix the bug firstly,
>>>>>>>>>>>>
>>>>>>>>>>>>> be even consuming "nr_pages".
>>>>>>>>>>>>
>>>>>>>>>>>> No sure about this part, it uses nr_pages as the end and calculate
>>>>>>>>>>>> the
>>>>>>>>>>>> 'base'.
>>>>>>>>>>>
>>>>>>>>>>> It should be using folio_nr_pages().
>>>>>>>>>>
>>>>>>>>>> But process_huge_page() without an explicit folio argument, I'd like to
>>>>>>>>>> move the aligned address calculate into the folio_zero_user and
>>>>>>>>>> copy_user_large_folio(will rename it to folio_copy_user()) in the
>>>>>>>>>> following cleanup patches, or do it in the fix patches?
>>>>>>>>>
>>>>>>>>> First, why does folio_zero_user() call process_huge_page() for *a small
>>>>>>>>> folio*? Because we like or code to be extra complicated to understand?
>>>>>>>>> Or am I missing something important?
>>>>>>>>
>>>>>>>> The folio_zero_user() used for PMD-sized THP and HugeTLB before, and
>>>>>>>> after anon mTHP supported, it is used for order-2~order-PMD-order THP
>>>>>>>> and HugeTLB, so it won't process a small folio if I understand correctly.
>>>>>>>
>>>>>>> And unfortunately neither the documentation nor the function name
>>>>>>> expresses that :(
>>>>>>>
>>>>>>> I'm happy to review any patches that improve the situation here :)
>>>>>>>
>>>>>> Actually, could we drop the process_huge_page() totally, from my
>>>>>> testcase[1], process_huge_page() is not better than clear/copy page
>>>>>> from start to last, and sequential clearing/copying maybe more
>>>>>> beneficial to the hardware prefetching, and is there a way to let lkp
>>>>>> to test to check the performance, since the process_huge_page()
>>>>>> was submitted by Ying, what's your opinion?
>>>> I don't think that it's a good idea to revert the commit without
>>>> studying and root causing the issues. I can work together with you on
>>>> that. If we have solid and well explained data to prove
>>>> process_huge_page() isn't benefitial, we can revert the commit.
>>>
>>>
>>> Take 'fallocate 20G' as an example， before
>>>
>>> Performance counter stats for 'taskset -c 10 fallocate -l 20G
>>> /mnt/hugetlbfs/test':
>> IIUC, fallocate will zero pages, but will not touch them at all,
>> right?
>> If so, no cache benefit from clearing referenced page last.
>
>
> Yes, for this case, only clear page.
>>
>>> 3,118.94 msec task-clock # 0.999 CPUs
>>> utilized
>>> 30 context-switches # 0.010 K/sec
>>> 1 cpu-migrations # 0.000 K/sec
>>> 136 page-faults # 0.044 K/sec
>>> 8,092,075,873 cycles #
>>> 2.594 GHz (92.82%)
>>> 1,624,587,663 instructions # 0.20 insn per
>>> cycle (92.83%)
>>> 395,341,850 branches # 126.755 M/sec
>>> (92.82%)
>>> 3,872,302 branch-misses # 0.98% of all
>>> branches (92.83%)
>>> 1,398,066,701 L1-dcache-loads # 448.251 M/sec
>>> (92.82%)
>>> 58,124,626 L1-dcache-load-misses # 4.16% of all
>>> L1-dcache accesses (92.82%)
>>> 1,032,527 LLC-loads # 0.331 M/sec
>>> (92.82%)
>>> 498,684 LLC-load-misses # 48.30% of all
>>> LL-cache accesses (92.84%)
>>> 473,689,004 L1-icache-loads # 151.875 M/sec
>>> (92.82%)
>>> 356,721 L1-icache-load-misses # 0.08% of all
>>> L1-icache accesses (92.85%)
>>> 1,947,644,987 dTLB-loads # 624.458 M/sec
>>> (92.95%)
>>> 10,185 dTLB-load-misses # 0.00% of all
>>> dTLB cache accesses (92.96%)
>>> 474,622,896 iTLB-loads # 152.174 M/sec
>>> (92.95%)
>>> 94 iTLB-load-misses # 0.00% of all
>>> iTLB cache accesses (85.69%)
>>>
>>> 3.122844830 seconds time elapsed
>>>
>>> 0.000000000 seconds user
>>> 3.107259000 seconds sys
>>>
>>> and after（clear from start to end）
>>>
>>> Performance counter stats for 'taskset -c 10 fallocate -l 20G
>>> /mnt/hugetlbfs/test':
>>>
>>> 1,135.53 msec task-clock # 0.999 CPUs
>>> utilized
>>> 10 context-switches # 0.009 K/sec
>>> 1 cpu-migrations # 0.001 K/sec
>>> 137 page-faults # 0.121 K/sec
>>> 2,946,673,587 cycles #
>>> 2.595 GHz (92.67%)
>>> 1,620,704,205 instructions # 0.55 insn per
>>> cycle (92.61%)
>>> 394,595,772 branches # 347.499 M/sec
>>> (92.60%)
>>> 130,756 branch-misses # 0.03% of all
>>> branches (92.84%)
>>> 1,396,726,689 L1-dcache-loads # 1230.022 M/sec
>>> (92.96%)
>>> 338,344 L1-dcache-load-misses # 0.02% of all
>>> L1-dcache accesses (92.95%)
>>> 111,737 LLC-loads # 0.098 M/sec
>>> (92.96%)
>>> 67,486 LLC-load-misses # 60.40% of all
>>> LL-cache accesses (92.96%)
>>> 418,198,663 L1-icache-loads # 368.285 M/sec
>>> (92.96%)
>>> 173,764 L1-icache-load-misses # 0.04% of all
>>> L1-icache accesses (92.96%)
>>> 2,203,364,632 dTLB-loads # 1940.385 M/sec
>>> (92.96%)
>>> 17,195 dTLB-load-misses # 0.00% of all
>>> dTLB cache accesses (92.95%)
>>> 418,198,365 iTLB-loads # 368.285 M/sec
>>> (92.96%)
>>> 79 iTLB-load-misses # 0.00% of all
>>> iTLB cache accesses (85.34%)
>>>
>>> 1.137015760 seconds time elapsed
>>>
>>> 0.000000000 seconds user
>>> 1.131266000 seconds sys
>>>
>>> The IPC improved a lot，less LLC-loads and more L1-dcache-loads， but
>>> this depends on the implementation of the microarchitecture.
>> Anyway, we need to avoid (or reduce at least) the pure memory
>> clearing
>> performance. Have you double checked whether process_huge_page() is
>> inlined? Perf-profile result can be used to check this too.
>>
>
> Yes, I'm sure the process_huge_page() is inlined.
>
>> When you say from start to end, you mean to use clear_gigantic_page()
>> directly, or change process_huge_page() to clear page from start to end?
>>
>
> Using clear_gigantic_page() and changing process_huge_page() to clear
> page from start to end are both good for performance when sequential
> clearing, but no random test so far.
>
>>> 1) Will test some rand test to check the different of performance as
>>> David suggested.
>>>
>>> 2) Hope the LKP to run more tests since it is very useful(more test
>>> set and different machines)
>> I'm starting to use LKP to test.

https://lore.kernel.org/linux-mm/20200419155856.dtwxomdkyujljdfi@oneplus.com/

Just remembered that we have discussed a similar issue for arm64 before.
Can you take a look at it? There's more discussion and tests/results in
the thread, I think that may be helpful.

--
Best Regards,
Huang, Ying