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