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

[Barry Song <21cnbao@gmail.com>]

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

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