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

[Nhat Pham <nphamcs@gmail.com>]

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?

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).