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

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

On Wed, Oct 23, 2024 at 4:10 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
>
>
> 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

this explains why the performance is much worse.

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

probably worth an optimization. calling cond_resched() for each page
seems too aggressive and useless.

diff --git a/mm/memory.c b/mm/memory.c
index 0f614523b9f4..5fc38347d782 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -6738,6 +6738,19 @@ EXPORT_SYMBOL(__might_fault);
 #endif
 
 #if defined(CONFIG_TRANSPARENT_HUGEPAGE) || defined(CONFIG_HUGETLBFS)
+/*
+ * To prevent process_huge_page() from starving other processes,
+ * we allow other processes a chance with each batch.
+ */
+static inline void batched_cond_resched(int *nr)
+{
+#define BATCHED_PROCESS_NR 64
+	if (*nr++ < BATCHED_PROCESS_NR)
+		return;
+	cond_resched();
+	*nr = 0;
+}
+
 /*
  * Process all subpages of the specified huge page with the specified
  * operation.  The target subpage will be processed last to keep its
@@ -6748,7 +6761,7 @@ static inline int process_huge_page(
 	int (*process_subpage)(unsigned long addr, int idx, void *arg),
 	void *arg)
 {
-	int i, n, base, l, ret;
+	int i, n, base, l, ret, processed_nr = 0;
 	unsigned long addr = addr_hint &
 		~(((unsigned long)nr_pages << PAGE_SHIFT) - 1);
 
@@ -6761,7 +6774,7 @@ static inline int process_huge_page(
 		l = n;
 		/* Process subpages at the end of huge page */
 		for (i = nr_pages - 1; i >= 2 * n; i--) {
-			cond_resched();
+			batched_cond_resched(&processed_nr);
 			ret = process_subpage(addr + i * PAGE_SIZE, i, arg);
 			if (ret)
 				return ret;
@@ -6772,7 +6785,7 @@ static inline int process_huge_page(
 		l = nr_pages - n;
 		/* Process subpages at the begin of huge page */
 		for (i = 0; i < base; i++) {
-			cond_resched();
+			batched_cond_resched(&processed_nr);
 			ret = process_subpage(addr + i * PAGE_SIZE, i, arg);
 			if (ret)
 				return ret;
@@ -6786,11 +6799,11 @@ static inline int process_huge_page(
 		int left_idx = base + i;
 		int right_idx = base + 2 * l - 1 - i;
 
-		cond_resched();
+		batched_cond_resched(&processed_nr);
 		ret = process_subpage(addr + left_idx * PAGE_SIZE, left_idx, arg);
 		if (ret)
 			return ret;
-		cond_resched();
+		batched_cond_resched(&processed_nr);
 		ret = process_subpage(addr + right_idx * PAGE_SIZE, right_idx, arg);
 		if (ret)
 			return ret;