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

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

On 2024/10/22 4:32, Barry Song wrote:
> On Tue, Oct 22, 2024 at 4:33 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>
>>
>>
>> On 2024/10/21 17:17, Barry Song wrote:
>>> On Mon, Oct 21, 2024 at 9:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>>>
>>>>
>>>>
>>>> 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.
>>>
>>> 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

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