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

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2024/11/1 14:18, Huang, Ying wrote:
> 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.
> 

Thanks for the tips, will check it.
> --
> Best Regards,
> Huang, Ying
>