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

[Mike Yuan <me@yhndnzj.com>]

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.

> What we do today dismisses inheritance completely, so it seems to me
> like it should be changed anyway.
> 
> I am thinking out loud here, let me know if my reasoning makes sense
> to you.
> 
> > 
> > Code looks solid to me - I think the upward tree traversal should
> > be
> > safe, as long as memcg is valid (since memcg holds reference to its
> > parent IIRC).
> >