Re: [PATCH v1 5/5] mm: zswap: do not check the global limit for same-filled pages

[Yosry Ahmed]

On Thu, Apr 4, 2024 at 7:54 PM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
>
> On Fri, Apr 05, 2024 at 01:35:47AM +0000, Yosry Ahmed wrote:
> > When storing same-filled pages, there is no point of checking the global
> > zswap limit as storing them does not contribute toward the limit Move
> > the limit checking after same-filled pages are handled.
> >
> > This avoids having same-filled pages skip zswap and go to disk swap if
> > the limit is hit. It also avoids queueing the shrink worker, which may
> > end up being unnecessary if the zswap usage goes down on its own before
> > another store is attempted.
> >
> > Ignoring the memcg limits as well for same-filled pages is more
> > controversial. Those limits are more a matter of per-workload policy.
> > Some workloads disable zswap completely by setting memory.zswap.max = 0,
> > and those workloads could start observing some zswap activity even after
> > disabling zswap. Although harmless, this could cause confusion to
> > userspace. Remain conservative and keep respecting those limits.
> >
> > Signed-off-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
>
> I'm not sure this buys us enough in practice to justify special-casing
> those entries even further. Especially with the quirk of checking
> cgroup limits but not the global ones; that would definitely need a
> code comment similar to what you have in the changelog; and once you
> add that, the real estate this special treatment takes up really
> doesn't seem reasonable anymore.

I was on the fence about this, and I thought it would be obvious
without a comment. But you are right that not skipping the limit check
for the cgroup limits would look weird.

>
> In most cases we'd expect a mix of pages to hit swap. Waking up the
> shrinker on a zero-filled entry is not strictly necessary of course,
> but the zswap limit has been reached and the system is swapping - a
> wakeup seems imminent anyway.

Theoretically, it is possible that we never have to wake up the
shrinker if zswap usage falls on its own before the next swapout,
especially with the shrinker in place. I thought it's a nice
optimization without much code, but the need for a large comment
replicating the commit log makes it less appealing tbh. I will just
drop this patch.