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

[Nhat Pham <nphamcs@gmail.com>]

On Wed, Aug 14, 2024 at 1:43 PM Mike Yuan <me@yhndnzj.com> wrote:
>
> On 2024-08-14 at 13:22 -0700, Yosry Ahmed wrote:
> > On Wed, Aug 14, 2024 at 12:52 PM Nhat Pham <nphamcs@gmail.com> wrote:
> > >
> > > On Wed, Aug 14, 2024 at 10:20 AM Mike Yuan <me@yhndnzj.com> wrote:
> > > >
> > > > Currently, the behavior of zswap.writeback wrt.
> > > > the cgroup hierarchy seems a bit odd. Unlike zswap.max,
> > > > it doesn't honor the value from parent cgroups. This
> > > > surfaced when people tried to globally disable zswap writeback,
> > > > i.e. reserve physical swap space only for hibernation [1] -
> > > > disabling zswap.writeback only for the root cgroup results
> > > > in subcgroups with zswap.writeback=1 still performing writeback.
> > > >
> > > > The consistency became more noticeable after I introduced
> > > > the MemoryZSwapWriteback= systemd unit setting [2] for
> > > > controlling the knob. The patch assumed that the kernel would
> > > > enforce the value of parent cgroups. It could probably be
> > > > workarounded from systemd's side, by going up the slice unit
> > > > tree and inherit the value. Yet I think it's more sensible
> > > > to make it behave consistently with zswap.max and friends.
> > >
> > > May I ask you to add/clarify this new expected behavior in
> > > Documentation/admin-guide/cgroup-v2.rst?
> > >
> > > >
> > > > [1]
> > > > https://wiki.archlinux.org/title/Power_management/Suspend_and_hibernate#Disable_zswap_writeback_to_use_the_swap_space_only_for_hibernation
> > >
> > > This is an interesting use case. Never envisioned this when I
> > > developed this feature :)
> > >
> > > > [2] https://github.com/systemd/systemd/pull/31734
> > > >
> > > > Signed-off-by: Mike Yuan <me@yhndnzj.com>
> > > > ---
> > >
> > > Personally, I don't feel too strongly about this one way or
> > > another. I
> > > guess you can make a case that people want to disable zswap
> > > writeback
> > > by default, and only selectively enable it for certain descendant
> > > workloads - for convenience, they would set memory.zswap.writeback
> > > ==
> > > 0 at root, then enable it on selected descendants?
> > >
> > > It's not super expensive IMHO - we already perform upward traversal
> > > on
> > > every zswap store. This wouldn't be the end of the world.
> > >
> > > Yosry, Johannes - how do you two feel about this?
> >
> > I wasn't CC'd on this, but found it by chance :) I think there is a
> > way for the zswap maintainers entry to match any patch that mentions
> > "zswap", not just based on files, right?
> >
> > 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).
> >
>
> Yeah, I thought about the other way around and reached the same
> conclusion.
> And there's permission boundary in the mix too - if root disables zswap
> writeback for its cgroup, the subcgroups, which could possibly be owned
> by other users, should not be able to reenable this.

Hmm yeah, I think I agree with your and Yosry's reasonings :) It
doesn't affect our use case AFAICS, and the code looks solid to me,
so:

Reviewed-by: Nhat Pham <nphamcs@gmail.com>