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

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

On 2024/10/21 13:38, Barry Song wrote:
> On Mon, Oct 21, 2024 at 6:16 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>
>>
>>
>> On 2024/10/21 12:15, Barry Song wrote:
>>> On Fri, Oct 18, 2024 at 8:48 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>>>
>>>>
>>>>
>>>> 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.
>>>>>
...

>>> 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...