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

Kefeng Wang
2024-10-30T03:04:57Z

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':

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.

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 questioned that just recently [1], and Ying assumed that it still
>> applies [2].
>>
>> c79b57e462b5 ("mm: hugetlb: clear target
>> sub-page last when clearing huge page”) documents the scenario where
>> this matters -- anon-w-seq which you also run below.
>>
>> If there is no performance benefit anymore, we should rip that
>> out. But likely we should check on multiple micro-architectures with
>> multiple #CPU configs that are relevant. c79b57e462b5 used a Xeon E5
>> v3 2699 with 72 processes on 2 NUMA nodes, maybe your test environment
>> cannot replicate that?>>
>>
>> [1]
>> https://lore.kernel.org/linux-mm/b8272cb4-aee8-45ad-8dff-353444b3fa74@redhat.com/
>> [2]
>> https://lore.kernel.org/linux-mm/878quv9lhf.fsf@yhuang6-desk2.ccr.corp.intel.com/
>>
>>> [1]https://lore.kernel.org/linux-mm/2524689c-08f5-446c-8cb9-924f9db0ee3a@huawei.com/
>>> case-anon-w-seq-mt (tried 2M PMD THP/ 64K mTHP)
>>> case-anon-w-seq-hugetlb (2M PMD HugeTLB)
>>
>> But these are sequential, not random. I'd have thought access +
>> zeroing would be sequentially either way. Did you run with random
>> access as well>

Will do.
> > --
> Best Regards,
> Huang, Ying
>