Re: [PATCH v6] zswap: memcontrol: implement zswap writeback disabling

[Chris Li]

Hi Nhat,

On Thu, Dec 7, 2023 at 5:03 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
>
> On Thu, Dec 7, 2023 at 4:19 PM Chris Li <chrisl@xxxxxxxxxx> wrote:
> >
> > Hi Nhat,
> >
> >
> > On Thu, Dec 7, 2023 at 11:24 AM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
> > >
> > > During our experiment with zswap, we sometimes observe swap IOs due to
> > > occasional zswap store failures and writebacks-to-swap. These swapping
> > > IOs prevent many users who cannot tolerate swapping from adopting zswap
> > > to save memory and improve performance where possible.
> > >
> > > This patch adds the option to disable this behavior entirely: do not
> > > writeback to backing swapping device when a zswap store attempt fail,
> > > and do not write pages in the zswap pool back to the backing swap
> > > device (both when the pool is full, and when the new zswap shrinker is
> > > called).
> > >
> > > This new behavior can be opted-in/out on a per-cgroup basis via a new
> > > cgroup file. By default, writebacks to swap device is enabled, which is
> > > the previous behavior. Initially, writeback is enabled for the root
> > > cgroup, and a newly created cgroup will inherit the current setting of
> > > its parent.
> > >
> > > Note that this is subtly different from setting memory.swap.max to 0, as
> > > it still allows for pages to be stored in the zswap pool (which itself
> > > consumes swap space in its current form).
> > >
> > > This patch should be applied on top of the zswap shrinker series:
> > >
> > > https://lore.kernel.org/linux-mm/20231130194023.4102148-1-nphamcs@xxxxxxxxx/
> > >
> > > as it also disables the zswap shrinker, a major source of zswap
> > > writebacks.
> >
> > I am wondering about the status of "memory.swap.tiers" proof of concept patch?
> > Are we still on board to have this two patch merge together somehow so
> > we can have
> > "memory.swap.tiers" == "all" and "memory.swap.tiers" == "zswap" cover the
> > memory.zswap.writeback == 1 and memory.zswap.writeback == 0 case?
> >
> > Thanks
> >
> > Chris
> >
>
> Hi Chris,
>
> I briefly summarized my recent discussion with Johannes here:
>
> https://lore.kernel.org/all/CAKEwX=NwGGRAtXoNPfq63YnNLBCF0ZDOdLVRsvzUmYhK4jxzHA@xxxxxxxxxxxxxx/

Sorry I am traveling in a different time zone so not able to get to
that email sooner. That email is only sent out less than one day
before the V6 patch right?

>
> TL;DR is we acknowledge the potential usefulness of swap.tiers
> interface, but the use case is not quite there yet, so it does not

I disagree about no use case. No use case for Meta != no usage case
for the rest of the linux kernel community. That mindset really needs
to shift to do Linux kernel development. Respect other's usage cases.
It is not just Meta's Linux kernel. It is everybody's Linux kernel.

I can give you three usage cases right now:
1) Google producting kernel uses SSD only swap, it is currently on
pilot. This is not expressible by the memory.zswap.writeback. You can
set the memory.zswap.max = 0 and memory.zswap.writeback = 1, then SSD
backed swapfile. But the whole thing feels very clunky, especially
what you really want is SSD only swap, you need to do all this zswap
config dance. Google has an internal memory.swapfile feature
implemented per cgroup swap file type by "zswap only", "real swap file
only", "both", "none" (the exact keyword might be different). running
in the production for almost 10 years. The need for more than zswap
type of per cgroup control is really there.

2) As indicated by this discussion, Tencent has a usage case for SSD
and hard disk swap as overflow.
https://lore.kernel.org/linux-mm/20231119194740.94101-9-ryncsn@xxxxxxxxx/
+Kairui

3) Android has some fancy swap ideas led by those patches.
https://lore.kernel.org/linux-mm/20230710221659.2473460-1-minchan@xxxxxxxxxx/
It got shot down due to removal of frontswap. But the usage case and
product requirement is there.
+Minchan

> make too much sense to build up that heavy machinery now.

Does my minimal memory.swap.tiers patch to support "zswap" and "all"
sound heavy machinery to you?
It is the same implementation under the hood. I am only trying to
avoid introducing something that will be foreseeable obsolete.

> zswap.writeback is a more urgent need, and does not prevent swap.tiers
> if we do decide to implement it.

I respect that urgent need, that is why I Ack on the V5 path, under
the understanding that this zswap.writeback is not carved into stones.
When a better interface comes alone, that interface can be obsolete.
Frankly speaking I would much prefer not introducing the cgroup API
which will be obsolete soon.

If you think zswap.writeback is not removable when another better
alternative is available, please voice it now.

If you squash my minimal memory.swap.tiers patch, it will also address
your urgent need for merging the "zswap.writeback", no?


Chris