On Wed, Aug 14, 2024 at 1:43 PM Mike Yuan <me@xxxxxxxxxxx> wrote: > > On 2024-08-14 at 13:22 -0700, Yosry Ahmed wrote: > > On Wed, Aug 14, 2024 at 12:52 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote: > > > > > > On Wed, Aug 14, 2024 at 10:20 AM Mike Yuan <me@xxxxxxxxxxx> 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@xxxxxxxxxxx> > > > > --- > > > > > > 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@xxxxxxxxx>
