Received-SPF: pass (google.com: domain of linux-kernel+bounces-24353-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20231024142706.195517-1-hezhongkun.hzk@bytedance.com>
 <CAKEwX=OiNB+pPhb-3Tf7O=F7psKE3EOpwmbPSeLSOyuHpj3i+Q@mail.gmail.com>
 <CACSyD1P6HmH9tSvONnNxYv8P+am_hH2dK3UJQd9_+o6EWkPsXA@mail.gmail.com>
 <CAKEwX=PC3C-PrWAH3XiYGyR4ujqBJQBBX6uRa2jXKCy9VMyRCQ@mail.gmail.com>
 <CACSyD1O7t0+BXUujJ81RAdEys3MUnmpu0sRADLazoyvayx5DLA@mail.gmail.com>
 <CAKEwX=P5AC+ubnunnZr5vMiC6fFU+E_E7jg_FZztWwZRYSxTWQ@mail.gmail.com>
 <CACSyD1Nnc_w3epbt6+EMt7a-4pAzgW1hbE=G5Fy5Tc5R5+uxKw@mail.gmail.com>
 <CAKEwX=NuXR9Ot1eRFsp9n-3Tq9yhjD9up+jyvXeOzQ4xK9kEPA@mail.gmail.com>
 <CAKEwX=Oj2dR6a4-DeccvcVdJ-J7b=83uCWQAf5u7U0sySudnkw@mail.gmail.com>
 <CAJD7tkb2oda=4f0s8w8xn+t_TM1b2Q_otbb86VPQ9R1m2uqDTA@mail.gmail.com>
 <CACSyD1ODCikYLDzO4LkQeDzB4sqDWCULwCdehw9inP-qyw3_Jg@mail.gmail.com>
 <CAJD7tkY=zmGiPoWNjVaVeU+NPxV2t48J5-CxEP9=nBK8nAh0XA@mail.gmail.com>
 <CAKEwX=Na3dg+KZwvtQi-Nj79Am-1tttDw50_qStkobmYGUC6NA@mail.gmail.com>
 <CACSyD1Pp8gkxwTXZuinm6wiZs0e5U2B5oND4rj29dzmRApFjhQ@mail.gmail.com> <CAKEwX=OsTQCJd12S3NajRMRy_s3q3yGFpS7S=_3-yOYK6+ezzA@mail.gmail.com>
In-Reply-To: <CAKEwX=OsTQCJd12S3NajRMRy_s3q3yGFpS7S=_3-yOYK6+ezzA@mail.gmail.com>
From: Zhongkun He <hezhongkun.hzk@bytedance.com>
Date: Fri, 12 Jan 2024 15:08:54 +0800
Message-ID: <CACSyD1NgqoFKuHSgdw_bzgK_StsLrNQ+7wHVBqsnHcB-2rD2ow@mail.gmail.com>
Subject: Re: [External] Re: [PATCH] mm: zswap: fix the lack of page lru flag
 in zswap_writeback_entry
To: Nhat Pham <nphamcs@gmail.com>
Cc: Yosry Ahmed <yosryahmed@google.com>, akpm@linux-foundation.org, hannes@cmpxchg.org, 
	sjenning@redhat.com, ddstreet@ieee.org, vitaly.wool@konsulko.com, 
	linux-mm@kvack.org, linux-kernel@vger.kernel.org, 
	Chris Li <chrisl@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jan 12, 2024 at 3:25=E2=80=AFAM Nhat Pham <nphamcs@gmail.com> wrote=
:
>
> On Wed, Jan 10, 2024 at 7:49=E2=80=AFPM Zhongkun He
> <hezhongkun.hzk@bytedance.com> wrote:
> >
> > >
> > > This sounds dangerous. This is going to introduce a rather large
> > > unexpected side effect - we're changing the readahead behavior in a
> > > seemingly small zswap optimization. In fact, I'd argue that if we do
> > > this, the readahead behavior change will be the "main effect", and th=
e
> > > zswap-side change would be a "happy consequence". We should run a lot
> > > of benchmarking and document the change extensively if we pursue this
> > > route.
> > >
> >
> > I agree with the unexpected side effect,  and here I need
> > to clarify the original intention of this patch.Please see the memory
> > offloading steps below.
> >
> >
> > memory      zswap(reclaim)          memory+swap (writeback)
> > 1G                 0.5G                        1G(tmp memory) + 1G=EF=
=BC=88swap=EF=BC=89
> >
> > If the decompressed memory cannot be released in time,
> > zswap's writeback has great side effects(mostly clod pages).
> > On the one hand, the memory space has not been reduced,
> > but has increased (from 0.5G->1G).
> > At the same time, it is not put the pages to the tail of the lru.
> > When the memory is insufficient, other pages will be squeezed out
> > and released early.
> > With this patch=EF=BC=8C we can put the tmp pages to the tail and recla=
im it
> > in time when the memory is insufficient or actively reclaimed.
> > So I think this patch makes sense and hope it can be fixed with a
> > suitable approaches.
>
> Makes sense to me. IIUC, that's the original intention behind calling
> SetPageReclaim() - unfortunately that doesn't work :) And IIRC, your
> original attempt shows reduction in swap usage (albeit at the cost of
> performance regression), which means we're onto something. I believe
> that the folio_lru_add_tail() approach will work :)
>
> Please include a version of the clarification paragraph above in your
> later version to explain the goal of the optimization, along with
> suitable benchmark numbers to show the effect (such as minimal change
> in performance, and reduction in some metrics). Maybe include the link
> to the original patch that introduces SetPageReclaim() too, to show
> the motivation behind all of this :) It'd be nice to have all the
> contexts readily available, in case we need to revisit this in the
> future (as was the case with the SetPageReclaim() here).
>

OK.

> >
> > >
> > > Unless some page flag/readahead expert can confirm that the first
> > > option is safe, my vote is on this option. I mean, it's fairly minima=
l
> > > codewise, no? Just a bunch of plumbing. We can also keep the other
> > > call sites intact if we just rename the old versions - something alon=
g
> > > the line of:
> > >
> > > __read_swap_cache_async_head(..., bool add_to_lru_head)
> > > {
> > > ...
> > > if (add_to_lru_head)
> > >   folio_add_lru(folio)
> > > else
> > >   folio_add_lru_tail(folio);
> > > }
> > >
> > > __read_swap_cache_async(...)
> > > {
> > >    return __read_swap_cache_async_tail(..., true);
> > > }
> > >
> > > A bit boilerplate? Sure. But this seems safer, and I doubt it's *that=
*
> > > much more work.
> > >
> >
> > Yes=EF=BC=8C agree. I will try it again.
>
> Look forward to seeing it! Thanks for your patience and for working on th=
is.

Thanks for your time.