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

[Yosry Ahmed <yosryahmed@google.com>]

On Wed, Aug 21, 2024 at 9:14 AM Michal Koutný <mkoutny@suse.com> wrote:
>
> On Wed, Aug 14, 2024 at 01:22:01PM GMT, Yosry Ahmed <yosryahmed@google.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.