Received-SPF: pass (google.com: domain of linux-kernel+bounces-21640-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>
In-Reply-To: <CAJD7tkY=zmGiPoWNjVaVeU+NPxV2t48J5-CxEP9=nBK8nAh0XA@mail.gmail.com>
From: Nhat Pham <nphamcs@gmail.com>
Date: Tue, 9 Jan 2024 17:32:37 -0800
Message-ID: <CAKEwX=Na3dg+KZwvtQi-Nj79Am-1tttDw50_qStkobmYGUC6NA@mail.gmail.com>
Subject: Re: [External] Re: [PATCH] mm: zswap: fix the lack of page lru flag
 in zswap_writeback_entry
To: Yosry Ahmed <yosryahmed@google.com>
Cc: Zhongkun He <hezhongkun.hzk@bytedance.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 Tue, Jan 9, 2024 at 8:30=E2=80=AFAM Yosry Ahmed <yosryahmed@google.com> =
wrote:
>
> On Mon, Jan 8, 2024 at 7:13=E2=80=AFPM Zhongkun He <hezhongkun.hzk@byteda=
nce.com> wrote:
> >
> > Hi Yosry, glad to hear from you and happy new year!
> >
> > > Sorry for being late to the party. It seems to me that all of this
> > > hassle can be avoided if lru_add_fn() did the right thing in this cas=
e
> > > and added the folio to the tail of the lru directly. I am no expert i=
n
> > > how the page flags work here, but it seems like we can do something
> > > like this in lru_add_fn():
> > >
> > > if (folio_test_reclaim(folio))
> > >     lruvec_add_folio_tail(lruvec, folio);
> > > else
> > >     lruvec_add_folio(lruvec, folio);
> > >
> > > I think the main problem with this is that PG_reclaim is an alias to
> > > PG_readahead, so readahead pages will also go to the tail of the lru,
> > > which is probably not good.

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

> >
> > Agree with you=EF=BC=8C I will try it.
>
> +Matthew Wilcox
>
> I think we need to figure out if it's okay to do this first, because
> it will affect pages with PG_readahead as well.
>
> >
> > >
> > > A more intrusive alternative is to introduce a folio_lru_add_tail()
> > > variant that always adds pages to the tail, and optionally call that
> > > from __read_swap_cache_async() instead of folio_lru_add() based on a
> > > new boolean argument. The zswap code can set that boolean argument
> > > during writeback to make sure newly allocated folios are always added
> > > to the tail of the lru.

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 minimal
codewise, no? Just a bunch of plumbing. We can also keep the other
call sites intact if we just rename the old versions - something along
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.

> >
> > I have the same idea and also find it intrusive. I think the first solu=
tion
> > is very good and I will try it. If it works, I will send the next versi=
on.
>
> One way to avoid introducing folio_lru_add_tail() and blumping a
> boolean from zswap is to have a per-task context (similar to
> memalloc_nofs_save()), that makes folio_add_lru() automatically add
> folios to the tail of the LRU. I am not sure if this is an acceptable
> approach though in terms of per-task flags and such.

This seems a bit hacky and obscure, but maybe it could work.