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

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

On 2024/10/21 15:55, Barry Song wrote:
> On Mon, Oct 21, 2024 at 8:47 PM Barry Song <21cnbao@gmail.com> wrote:
>>
>> On Mon, Oct 21, 2024 at 7:09 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>>
>>>
>>>
>>> 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...
>>
>> 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.