Re: [PATCH] mm: shmem: convert to use folio_zero_range()

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

On 2024/10/28 10:39, Huang, Ying wrote:
> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
> 
>> On 2024/10/25 20:21, Huang, Ying wrote:
>>> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>>>
>>>> On 2024/10/25 15:47, Huang, Ying wrote:
>>>>> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>>>>>
>>>>>> On 2024/10/25 10:59, Huang, Ying wrote:
>>>>>>> Hi, Kefeng,
>>>>>>> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>>>>>>>
>>>>>>>> +CC Huang Ying,
>>>>>>>>
>>>>>>>> On 2024/10/23 6:56, Barry Song wrote:
>>>>>>>>> On Wed, Oct 23, 2024 at 4:10 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> ...
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> On 2024/10/17 23:09, Matthew Wilcox wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> On Thu, Oct 17, 2024 at 10:25:04PM +0800, Kefeng Wang wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> Directly use folio_zero_range() to cleanup code.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Are you sure there's no performance regression introduced by this?
>>>>>>>>>>>>>>>>>>>>>>>>> clear_highpage() is often optimised in ways that we can't optimise for
>>>>>>>>>>>>>>>>>>>>>>>>> a plain memset().  On the other hand, if the folio is large, maybe a
>>>>>>>>>>>>>>>>>>>>>>>>> modern CPU will be able to do better than clear-one-page-at-a-time.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Right, I missing this, clear_page might be better than memset, I change
>>>>>>>>>>>>>>>>>>>>>>>> this one when look at the shmem_writepage(), which already convert to
>>>>>>>>>>>>>>>>>>>>>>>> use folio_zero_range() from clear_highpage(), also I grep
>>>>>>>>>>>>>>>>>>>>>>>> folio_zero_range(), there are some other to use folio_zero_range().
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> fs/bcachefs/fs-io-buffered.c:           folio_zero_range(folio, 0,
>>>>>>>>>>>>>>>>>>>>>>>> folio_size(folio));
>>>>>>>>>>>>>>>>>>>>>>>> fs/bcachefs/fs-io-buffered.c:                   folio_zero_range(f,
>>>>>>>>>>>>>>>>>>>>>>>> 0, folio_size(f));
>>>>>>>>>>>>>>>>>>>>>>>> fs/bcachefs/fs-io-buffered.c:                   folio_zero_range(f,
>>>>>>>>>>>>>>>>>>>>>>>> 0, folio_size(f));
>>>>>>>>>>>>>>>>>>>>>>>> fs/libfs.c:     folio_zero_range(folio, 0, folio_size(folio));
>>>>>>>>>>>>>>>>>>>>>>>> fs/ntfs3/frecord.c:             folio_zero_range(folio, 0,
>>>>>>>>>>>>>>>>>>>>>>>> folio_size(folio));
>>>>>>>>>>>>>>>>>>>>>>>> mm/page_io.c:   folio_zero_range(folio, 0, folio_size(folio));
>>>>>>>>>>>>>>>>>>>>>>>> mm/shmem.c:             folio_zero_range(folio, 0, folio_size(folio));
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> IOW, what performance testing have you done with this patch?
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> No performance test before, but I write a testcase,
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> 1) allocate N large folios (folio_alloc(PMD_ORDER))
>>>>>>>>>>>>>>>>>>>>>>>> 2) then calculate the diff(us) when clear all N folios
>>>>>>>>>>>>>>>>>>>>>>>>                clear_highpage/folio_zero_range/folio_zero_user
>>>>>>>>>>>>>>>>>>>>>>>> 3) release N folios
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> the result(run 5 times) shown below on my machine,
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> N=1,
>>>>>>>>>>>>>>>>>>>>>>>>                    clear_highpage  folio_zero_range    folio_zero_user
>>>>>>>>>>>>>>>>>>>>>>>>               1      69                   74                 177
>>>>>>>>>>>>>>>>>>>>>>>>               2      57                   62                 168
>>>>>>>>>>>>>>>>>>>>>>>>               3      54                   58                 234
>>>>>>>>>>>>>>>>>>>>>>>>               4      54                   58                 157
>>>>>>>>>>>>>>>>>>>>>>>>               5      56                   62                 148
>>>>>>>>>>>>>>>>>>>>>>>> avg       58                   62.8               176.8
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> N=100
>>>>>>>>>>>>>>>>>>>>>>>>                    clear_highpage  folio_zero_range    folio_zero_user
>>>>>>>>>>>>>>>>>>>>>>>>               1    11015                 11309               32833
>>>>>>>>>>>>>>>>>>>>>>>>               2    10385                 11110               49751
>>>>>>>>>>>>>>>>>>>>>>>>               3    10369                 11056               33095
>>>>>>>>>>>>>>>>>>>>>>>>               4    10332                 11017               33106
>>>>>>>>>>>>>>>>>>>>>>>>               5    10483                 11000               49032
>>>>>>>>>>>>>>>>>>>>>>>> avg     10516.8               11098.4             39563.4
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> N=512
>>>>>>>>>>>>>>>>>>>>>>>>                    clear_highpage  folio_zero_range   folio_zero_user
>>>>>>>>>>>>>>>>>>>>>>>>               1    55560                 60055              156876
>>>>>>>>>>>>>>>>>>>>>>>>               2    55485                 60024              157132
>>>>>>>>>>>>>>>>>>>>>>>>               3    55474                 60129              156658
>>>>>>>>>>>>>>>>>>>>>>>>               4    55555                 59867              157259
>>>>>>>>>>>>>>>>>>>>>>>>               5    55528                 59932              157108
>>>>>>>>>>>>>>>>>>>>>>>> avg     55520.4               60001.4            157006.6
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> folio_zero_user with many cond_resched(), so time fluctuates a lot,
>>>>>>>>>>>>>>>>>>>>>>>> clear_highpage is better folio_zero_range as you said.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Maybe add a new helper to convert all folio_zero_range(folio, 0,
>>>>>>>>>>>>>>>>>>>>>>>> folio_size(folio))
>>>>>>>>>>>>>>>>>>>>>>>> to use clear_highpage + flush_dcache_folio?
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> If this also improves performance for other existing callers of
>>>>>>>>>>>>>>>>>>>>>>> folio_zero_range(), then that's a positive outcome.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> ...
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> hi Kefeng,
>>>>>>>>>>>>>>>>>>>> what's your point? providing a helper like clear_highfolio() or similar?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Yes, from above test, using clear_highpage/flush_dcache_folio is better
>>>>>>>>>>>>>>>>>>> than using folio_zero_range() for folio zero(especially for large
>>>>>>>>>>>>>>>>>>> folio), so I'd like to add a new helper, maybe name it folio_zero()
>>>>>>>>>>>>>>>>>>> since it zero the whole folio.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> we already have a helper like folio_zero_user()?
>>>>>>>>>>>>>>>>>> it is not good enough?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Since it is with many cond_resched(), the performance is worst...
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Not exactly? It should have zero cost for a preemptible kernel.
>>>>>>>>>>>>>>>> For a non-preemptible kernel, it helps avoid clearing the folio
>>>>>>>>>>>>>>>> from occupying the CPU and starving other processes, right?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> --- a/mm/shmem.c
>>>>>>>>>>>>>>> +++ b/mm/shmem.c
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> @@ -2393,10 +2393,7 @@ static int shmem_get_folio_gfp(struct inode
>>>>>>>>>>>>>>> *inode, pgoff_t index,
>>>>>>>>>>>>>>>                  * it now, lest undo on failure cancel our earlier guarantee.
>>>>>>>>>>>>>>>                  */
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>                 if (sgp != SGP_WRITE && !folio_test_uptodate(folio)) {
>>>>>>>>>>>>>>> -               long i, n = folio_nr_pages(folio);
>>>>>>>>>>>>>>> -
>>>>>>>>>>>>>>> -               for (i = 0; i < n; i++)
>>>>>>>>>>>>>>> -                       clear_highpage(folio_page(folio, i));
>>>>>>>>>>>>>>> +               folio_zero_user(folio, vmf->address);
>>>>>>>>>>>>>>>                         flush_dcache_folio(folio);
>>>>>>>>>>>>>>>                         folio_mark_uptodate(folio);
>>>>>>>>>>>>>>>                 }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Do we perform better or worse with the following?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Here is for SGP_FALLOC, vmf = NULL, we could use folio_zero_user(folio,
>>>>>>>>>>>>>> 0), I think the performance is worse, will retest once I can access
>>>>>>>>>>>>>> hardware.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Perhaps, since the current code uses clear_hugepage(). Does using
>>>>>>>>>>>>> index << PAGE_SHIFT as the addr_hint offer any benefit?
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> when use folio_zero_user(), the performance is vary bad with above
>>>>>>>>>>>> fallocate test(mount huge=always),
>>>>>>>>>>>>
>>>>>>>>>>>>              folio_zero_range   clear_highpage         folio_zero_user
>>>>>>>>>>>> real    0m1.214s             0m1.111s              0m3.159s
>>>>>>>>>>>> user    0m0.000s             0m0.000s              0m0.000s
>>>>>>>>>>>> sys     0m1.210s             0m1.109s              0m3.152s
>>>>>>>>>>>>
>>>>>>>>>>>> I tried with addr_hint = 0/index << PAGE_SHIFT, no obvious different.
>>>>>>>>>>>
>>>>>>>>>>> Interesting. Does your kernel have preemption disabled or
>>>>>>>>>>> preemption_debug enabled?
>>>>>>>>>>
>>>>>>>>>> ARM64 server, CONFIG_PREEMPT_NONE=y
>>>>>>>>> this explains why the performance is much worse.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> If not, it makes me wonder whether folio_zero_user() in
>>>>>>>>>>> alloc_anon_folio() is actually improving performance as expected,
>>>>>>>>>>> compared to the simpler folio_zero() you plan to implement. :-)
>>>>>>>>>>
>>>>>>>>>> Yes, maybe, the folio_zero_user(was clear_huge_page) is from
>>>>>>>>>> 47ad8475c000 ("thp: clear_copy_huge_page"), so original clear_huge_page
>>>>>>>>>> is used in HugeTLB, clear PUD size maybe spend many time, but for PMD or
>>>>>>>>>> other size of large folio, cond_resched is not necessary since we
>>>>>>>>>> already have some folio_zero_range() to clear large folio, and no issue
>>>>>>>>>> was reported.
>>>>>>>>> probably worth an optimization. calling cond_resched() for each page
>>>>>>>>> seems too aggressive and useless.
>>>>>>>>
>>>>>>>> After some test, I think the cond_resched() is not the root cause,
>>>>>>>> no performance gained with batched cond_resched(), even I kill
>>>>>>>> cond_resched() from process_huge_page, no improvement.
>>>>>>>>
>>>>>>>> But when I unconditionally use clear_gigantic_page() in
>>>>>>>> folio_zero_user(patched), there is big improvement with above
>>>>>>>> fallocate on tmpfs(mount huge=always), also I test some other testcase,
>>>>>>>>
>>>>>>>>
>>>>>>>> 1) case-anon-w-seq-mt: (2M PMD THP)
>>>>>>>>
>>>>>>>> base:
>>>>>>>> real    0m2.490s    0m2.254s    0m2.272s
>>>>>>>> user    1m59.980s   2m23.431s   2m18.739s
>>>>>>>> sys     1m3.675s    1m15.462s   1m15.030s	
>>>>>>>>
>>>>>>>> patched:
>>>>>>>> real    0m2.234s    0m2.225s    0m2.159s
>>>>>>>> user    2m56.105s   2m57.117s   3m0.489s
>>>>>>>> sys     0m17.064s   0m17.564s   0m16.150s
>>>>>>>>
>>>>>>>> Patched kernel win on sys and bad in user, but real is almost same,
>>>>>>>> maybe a little better than base.
>>>>>>> We can find user time difference.  That means the original cache hot
>>>>>>> behavior still applies on your system.
>>>>>>> However, it appears that the performance to clear page from end to
>>>>>>> begin
>>>>>>> is really bad on your system.
>>>>>>> So, I suggest to revise the current implementation to use sequential
>>>>>>> clearing as much as possible.
>>>>>>>
>>>>>>
>>>>>> I test case-anon-cow-seq-hugetlb for copy_user_large_folio()
>>>>>>
>>>>>> base:
>>>>>> real    0m6.259s    0m6.197s    0m6.316s
>>>>>> user    1m31.176s   1m27.195s   1m29.594s
>>>>>> sys     7m44.199s   7m51.490s   8m21.149s
>>>>>>
>>>>>> patched(use copy_user_gigantic_page for 2M hugetlb too)
>>>>>> real    0m3.182s    0m3.002s    0m2.963s
>>>>>> user    1m19.456s   1m3.107s    1m6.447s
>>>>>> sys     2m59.222s   3m10.899s   3m1.027s
>>>>>>
>>>>>> and sequential copy is better than the current implementation,
>>>>>> so I will use sequential clear and copy.
>>>>> Sorry, it appears that you misunderstanding my suggestion.  I
>>>>> suggest to
>>>>> revise process_huge_page() to use more sequential memory clearing and
>>>>> copying to improve its performance on your platform.
>>>>> --
>>>>> Best Regards,
>>>>> Huang, Ying
>>>>>
>>>>>>>> 2) case-anon-w-seq-hugetlb:(2M PMD HugeTLB)
>>>>>>>>
>>>>>>>> base:
>>>>>>>> real    0m5.175s    0m5.117s    0m4.856s
>>>>>>>> user    5m15.943s   5m7.567s    4m29.273s
>>>>>>>> sys     2m38.503s   2m21.949s   2m21.252s
>>>>>>>>
>>>>>>>> patched:
>>>>>>>> real    0m4.966s    0m4.841s    0m4.561s
>>>>>>>> user    6m30.123s   6m9.516s    5m49.733s
>>>>>>>> sys     0m58.503s   0m47.847s   0m46.785s
>>>>>>>>
>>>>>>>>
>>>>>>>> This case is similar to the case1.
>>>>>>>>
>>>>>>>> 3) fallocate hugetlb 20G (2M PMD HugeTLB)
>>>>>>>>
>>>>>>>> base:
>>>>>>>> real    0m3.016s    0m3.019s    0m3.018s
>>>>>>>> user    0m0.000s    0m0.000s    0m0.000s
>>>>>>>> sys     0m3.009s    0m3.012s    0m3.010s
>>>>>>>>
>>>>>>>> patched:
>>>>>>>>
>>>>>>>> real    0m1.136s    0m1.136s    0m1.136s
>>>>>>>> user    0m0.000s    0m0.000s    0m0.004s
>>>>>>>> sys     0m1.133s    0m1.133s    0m1.129s
>>>>>>>>
>>>>>>>>
>>>>>>>> There is big win on patched kernel, and it is similar to above tmpfs
>>>>>>>> test, so maybe we could revert the commit c79b57e462b5 ("mm: hugetlb:
>>>>>>>> clear target sub-page last when clearing huge page").
>>>>
>>>> I tried the following changes,
>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>> index 66cf855dee3f..e5cc75adfa10 100644
>>>> --- a/mm/memory.c
>>>> +++ b/mm/memory.c
>>>> @@ -6777,7 +6777,7 @@ static inline int process_huge_page(
>>>>                   base = 0;
>>>>                   l = n;
>>>>                   /* Process subpages at the end of huge page */
>>>> -               for (i = nr_pages - 1; i >= 2 * n; i--) {
>>>> +               for (i = 2 * n; i < nr_pages; i++) {
>>>>                           cond_resched();
>>>>                           ret = process_subpage(addr + i * PAGE_SIZE, i,
>>>>                           arg);
>>>>                           if (ret)
>>>>
>>>> Since n = 0, so the copying is from start to end now, but not
>>>> improvement for case-anon-cow-seq-hugetlb,
>>>>
>>>> and if use copy_user_gigantic_pager, the time reduced from 6s to 3s
>>>>
>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>> index fe21bd3beff5..2c6532d21d84 100644
>>>> --- a/mm/memory.c
>>>> +++ b/mm/memory.c
>>>> @@ -6876,10 +6876,7 @@ int copy_user_large_folio(struct folio *dst,
>>>> struct folio *src,
>>>>                   .vma = vma,
>>>>           };
>>>>
>>>> -       if (unlikely(nr_pages > MAX_ORDER_NR_PAGES))
>>>> -               return copy_user_gigantic_page(dst, src, addr_hint,
>>>>                   vma, nr_pages);
>>>> -
>>>> -       return process_huge_page(addr_hint, nr_pages, copy_subpage, &arg);
>>>> +       return copy_user_gigantic_page(dst, src, addr_hint, vma, nr_pages);
>>>>    }
>>> It appears that we have code generation issue here.  Can you check
>>> it?
>>> Whether code is inlined in the same way?
>>>
>>
>> No different, and I checked the asm, both process_huge_page and
>> copy_user_gigantic_page are inlined, it is strange...
> 
> It's not inlined in my configuration.  And __always_inline below changes
> it for me.
> 
> If it's already inlined and the code is actually almost same, why
> there's difference?  Is it possible for you to do some profile or
> further analysis?

Yes, will continue to debug this.

> 
>>> Maybe we can start with
>>> modified   mm/memory.c
>>> @@ -6714,7 +6714,7 @@ EXPORT_SYMBOL(__might_fault);
>>>     * operation.  The target subpage will be processed last to keep its
>>>     * cache lines hot.
>>>     */
>>> -static inline int process_huge_page(
>>> +static __always_inline int process_huge_page(
>>>    	unsigned long addr_hint, unsigned int nr_pages,
>>>    	int (*process_subpage)(unsigned long addr, int idx, void *arg),
>>>    	void *arg)
> 
> --
> Best Regards,
> Huang, Ying
>