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

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

On 2024/10/18 15:32, Kefeng Wang wrote:
> 
> 
> On 2024/10/18 13:23, Barry Song wrote:
>> On Fri, Oct 18, 2024 at 6:20 PM 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.
> 
> 
> rm -f /tmp/test && fallocate -l 20G /tmp/test && fallocate -d -l 20G / 
> tmp/test && time fallocate -l 20G /tmp/test
> 
> 1）mount always(2M folio)
>      with patch    without patch
> real    0m1.214s    0m1.111s
> user    0m0.000s    0m0.000s
> sys    0m1.210s    0m1.109s
> 
> With this patch, the performance does have regression,
> folio_zero_range() is bad than clear_highpage + flush_dcache_folio
> 
> with patch

Oh, this should without patch since it uses clear_highpage,

> 
>    99.95%     0.00%  fallocate  [kernel.vmlinux]       [k] vfs_fallocate
>     vfs_fallocate
>   - shmem_fallocate
>        98.54% __pi_clear_page
>      - 1.38% shmem_get_folio_gfp
>           filemap_get_entry
> 
and this one is with patch
> without patch
>   99.89%     0.00%  fallocate  [kernel.vmlinux]       [k] shmem_fallocate
>    shmem_fallocate
> - shmem_get_folio_gfp
>       90.12% __memset
>     - 9.42% zero_user_segments.constprop.0
>          8.16% flush_dcache_page
>          1.03% flush_dcache_folio
> 
> 
> 
> 
> 2）mount  never (4K folio)
> real    0m3.159s    0m3.176s
> user    0m0.000s    0m0.000s
> sys    0m3.150s    0m3.169s
> 
> But with this patch, the performance is improved a little,
> folio_zero_range() is better than clear_highpage + flush_dcache_folio
> 

For 4K, the result is fluctuating, so maybe no different.

> with patch
>   97.77%     3.37%  fallocate  [kernel.vmlinux]       [k] shmem_fallocate
> - 94.40% shmem_fallocate
>     - 93.70% shmem_get_folio_gfp
>          66.60% __memset
>        - 7.43% filemap_get_entry
>             3.49% xas_load
>          1.32% zero_user_segments.constprop.0
> 
> without patch
>    97.82%     3.22%  fallocate  [kernel.vmlinux]       [k] shmem_fallocate
>   - 94.61% shmem_fallocate
>        68.18% __pi_clear_page
>      - 25.60% shmem_get_folio_gfp
>         - 7.64% filemap_get_entry
>              3.51% xas_load
>>
>>>
>>>
>>>>
>>>>>       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));
>>>>> -            flush_dcache_folio(folio);
>>>>> +            folio_zero_range(folio, 0, folio_size(folio));
>>>>>               folio_mark_uptodate(folio);
>>>>>       }
>>>>
>>>
>>>
>>
>> Thanks
>> Barry
> 
>