Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> writes: > On 2024/10/30 11:21, Huang, Ying wrote: >> Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> writes: >> >>> On 2024/10/30 9:04, Huang, Ying wrote: >>>> David Hildenbrand <david@xxxxxxxxxx> 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@xxxxxxxxxxx/ 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
