linux-kernel - Re: [PATCH] mm/memcontrol: respect zswap.writeback setting from parent cg too

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]

Message-ID: <CAJD7tkZ28FSKOmA4dzVdTzR05_Qge9EcYGZzwMNgP7EHJOJHWQ@mail.gmail.com>
Date: Thu, 22 Aug 2024 10:49:00 -0700
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Michal Koutný <mkoutny@...e.com>
Cc: Nhat Pham <nphamcs@...il.com>, Mike Yuan <me@...dnzj.com>, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org, cgroups@...r.kernel.org, 
	Andrew Morton <akpm@...ux-foundation.org>, Muchun Song <muchun.song@...ux.dev>, 
	Shakeel Butt <shakeel.butt@...ux.dev>, Roman Gushchin <roman.gushchin@...ux.dev>, 
	Michal Hocko <mhocko@...nel.org>, Johannes Weiner <hannes@...xchg.org>
Subject: Re: [PATCH] mm/memcontrol: respect zswap.writeback setting from
 parent cg too

On Wed, Aug 21, 2024 at 9:14 AM Michal Koutný <mkoutny@...e.com> wrote:
>
> On Wed, Aug 14, 2024 at 01:22:01PM GMT, Yosry Ahmed <yosryahmed@...gle.com> wrote:
> > Anyway, both use cases make sense to me, disabling writeback
> > system-wide or in an entire subtree, and disabling writeback on the
> > root and then selectively enabling it. I am slightly inclined to the
> > first one (what this patch does).
> >
> > Considering the hierarchical cgroup knobs work, we usually use the
> > most restrictive limit among the ancestors. I guess it ultimately
> > depends on how we define "most restrictive". Disabling writeback is
> > restrictive in the sense that you don't have access to free some zswap
> > space to reclaim more memory. OTOH, disabling writeback also means
> > that your zswapped memory won't go to disk under memory pressure, so
> > in that sense it would be restrictive to force writeback :)
> >
> > Usually, the "default" is the non-restrictive thing, and then you can
> > set restrictions that apply to all children (e.g. no limits are set by
> > default). Since writeback is enabled by default, it seems like the
> > restriction would be disabling writeback. Hence, it would make sense
> > to inherit zswap disabling (i.e. only writeback if all ancestors allow
> > it, like this patch does).
> >
> > What we do today dismisses inheritance completely, so it seems to me
> > like it should be changed anyway.
>
> I subscribe to inheritance (at cgroup creation) not proving well (in
> general). Here's the case of expecting hierarchical semantic of the
> attribute.
>
> With this change -- is there any point in keeping the inheritance
> around? (Simply default to enabled.)

Agreed, please feel free to include a patch in your next version that
does that, and add "Fixes" tags and Cc:stable so that the changes are
backported and users get these changes ASAP.