lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Thu, 4 Apr 2024 21:19:09 -0700
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Johannes Weiner <hannes@...xchg.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Nhat Pham <nphamcs@...il.com>, 
	Chengming Zhou <chengming.zhou@...ux.dev>, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 5/5] mm: zswap: do not check the global limit for
 same-filled pages

On Thu, Apr 4, 2024 at 7:54 PM Johannes Weiner <hannes@...xchg.org> 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@...gle.com>
>
> 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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ