| Message ID | 20240814171800.23558-1-me@yhndnzj.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/memcontrol: respect zswap.writeback setting from parent cg too | expand |
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).
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: > > May I ask you to add/clarify this new expected behavior in > Documentation/admin-guide/cgroup-v2.rst? (and a selftest too, if it's not too much to ask :))
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). 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). >
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). > >
On 2024-08-14 at 12:53 -0700, Nhat Pham 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: > > > > May I ask you to add/clarify this new expected behavior in > > Documentation/admin-guide/cgroup-v2.rst? > > (and a selftest too, if it's not too much to ask :)) Hmm, I tried to, but the test_zswap_writeback_enabled and test_zswap_writeback_disabled tests in the current tree already fail for me... Should I still add new test cases under this circumstance?
On Thu, Aug 15, 2024 at 11:39 AM Mike Yuan <me@yhndnzj.com> wrote: > > > Hmm, I tried to, but the test_zswap_writeback_enabled and > test_zswap_writeback_disabled tests in the current tree already > fail for me... Should I still add new test cases under this > circumstance? Hmm interesting. We need to fix it - could you forward me the details of the failure mode? But yeah, for now let's skip the selftest. Could you send a fixlet or a follow up patch to update the documentation :)
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>
On Thu, 15 Aug 2024 12:12:26 -0700 Nhat Pham <nphamcs@gmail.com> wrote: > > 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> But you'd still like an update to Documentation/admin-guide/cgroup-v2.rst?
On Thu, Aug 15, 2024 at 3:08 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Thu, 15 Aug 2024 12:12:26 -0700 Nhat Pham <nphamcs@gmail.com> wrote: > > > > 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> > > But you'd still like an update to Documentation/admin-guide/cgroup-v2.rst? Yeah I'd rather see a v2 with updated docs, and hopefully a selftest if the existing tests problem is resolved. Also, do we want a Fixes tag and to backport this so that current users get the new behavior ASAP?
On Thu, Aug 15, 2024 at 3:10 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > On Thu, Aug 15, 2024 at 3:08 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Thu, 15 Aug 2024 12:12:26 -0700 Nhat Pham <nphamcs@gmail.com> wrote: > > > > > > 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> > > > > But you'd still like an update to Documentation/admin-guide/cgroup-v2.rst? > > > Yeah I'd rather see a v2 with updated docs, and hopefully a selftest > if the existing tests problem is resolved. Ah yeah, I was thinking this could be done in a follow-up patch. But yes, please - documentation. Preferably everything together as v2. > > Also, do we want a Fixes tag and to backport this so that current > users get the new behavior ASAP? Hmm, I wonder if it's more confusing for users to change the behavior in older kernels. (OTOH, if this already is what people expect, then yeah it's a good idea to backport).
On 2024-08-15 at 12:10 -0700, Nhat Pham wrote: > On Thu, Aug 15, 2024 at 11:39 AM Mike Yuan <me@yhndnzj.com> wrote: > > > > > > Hmm, I tried to, but the test_zswap_writeback_enabled and > > test_zswap_writeback_disabled tests in the current tree already > > fail for me... Should I still add new test cases under this > > circumstance? > > Hmm interesting. We need to fix it - could you forward me the details > of the failure mode? > > But yeah, for now let's skip the selftest. Could you send a fixlet or > a follow up patch to update the documentation :) Sorry for the noise. It seems to be local configuration issue from my side. I'll submit a v2 with Documentation update and selftest :)
On Thu, Aug 15, 2024 at 4:32 PM Nhat Pham <nphamcs@gmail.com> wrote: > > On Thu, Aug 15, 2024 at 3:10 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > On Thu, Aug 15, 2024 at 3:08 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > > On Thu, 15 Aug 2024 12:12:26 -0700 Nhat Pham <nphamcs@gmail.com> wrote: > > > > > > > > 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> > > > > > > But you'd still like an update to Documentation/admin-guide/cgroup-v2.rst? > > > > > > Yeah I'd rather see a v2 with updated docs, and hopefully a selftest > > if the existing tests problem is resolved. > > Ah yeah, I was thinking this could be done in a follow-up patch. > > But yes, please - documentation. Preferably everything together as v2. > > > > > Also, do we want a Fixes tag and to backport this so that current > > users get the new behavior ASAP? > > Hmm, I wonder if it's more confusing for users to change the behavior > in older kernels. > > (OTOH, if this already is what people expect, then yeah it's a good > idea to backport). My rationale is that if people will inevitably get the behavior change when they upgrade their kernel, I'd rather they get it sooner rather than later, before more users start depending on the old behavior. I am guessing there is a chance this is not what backports are meant for. Andrew, any thoughts on this?
On Mon, 19 Aug 2024 12:05:44 -0700 Yosry Ahmed <yosryahmed@google.com> wrote: > > Ah yeah, I was thinking this could be done in a follow-up patch. > > > > But yes, please - documentation. Preferably everything together as v2. > > > > > > > > Also, do we want a Fixes tag and to backport this so that current > > > users get the new behavior ASAP? > > > > Hmm, I wonder if it's more confusing for users to change the behavior > > in older kernels. > > > > (OTOH, if this already is what people expect, then yeah it's a good > > idea to backport). > > My rationale is that if people will inevitably get the behavior change > when they upgrade their kernel, I'd rather they get it sooner rather > than later, before more users start depending on the old behavior. > > I am guessing there is a chance this is not what backports are meant > for. Andrew, any thoughts on this? I agree. It does depend on how long the old behavior has been out in the field, and on our assessment of how many people are likely to inconvenienced. So... yes please, what is that Fixes:?
On Mon, Aug 19, 2024 at 6:01 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Mon, 19 Aug 2024 12:05:44 -0700 Yosry Ahmed <yosryahmed@google.com> wrote: > > > > Ah yeah, I was thinking this could be done in a follow-up patch. > > > > > > But yes, please - documentation. Preferably everything together as v2. > > > > > > > > > > > Also, do we want a Fixes tag and to backport this so that current > > > > users get the new behavior ASAP? > > > > > > Hmm, I wonder if it's more confusing for users to change the behavior > > > in older kernels. > > > > > > (OTOH, if this already is what people expect, then yeah it's a good > > > idea to backport). > > > > My rationale is that if people will inevitably get the behavior change > > when they upgrade their kernel, I'd rather they get it sooner rather > > than later, before more users start depending on the old behavior. > > > > I am guessing there is a chance this is not what backports are meant > > for. Andrew, any thoughts on this? > > I agree. It does depend on how long the old behavior has been out in > the field, and on our assessment of how many people are likely to > inconvenienced. So... yes please, what is that Fixes:? > It's commit 501a06fe8e4c ("zswap: memcontrol: implement zswap writeback disabling"). It landed in v6.8. I suspect there aren't many users that depend on the old behavior so far, so I would prefer to get this backported so that it's less likely that more (or any) users start depending on the old behavior.
On Mon, Aug 19, 2024 at 9:07 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > It's commit 501a06fe8e4c ("zswap: memcontrol: implement zswap > writeback disabling"). It landed in v6.8. > > I suspect there aren't many users that depend on the old behavior so > far, so I would prefer to get this backported so that it's less likely > that more (or any) users start depending on the old behavior. Agree - let's backport it then. In hindsight, I feel like this is already what users assume. We don't rely on the assumption internally, but at least one client of ours has asked for clarifications about this.
On Wed, Aug 14, 2024 at 01:22:01PM GMT, Yosry Ahmed <yosryahmed@google.com> 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.) Thanks, Michal
On Wed, Aug 21, 2024 at 9:14 AM Michal Koutný <mkoutny@suse.com> wrote: > > On Wed, Aug 14, 2024 at 01:22:01PM GMT, Yosry Ahmed <yosryahmed@google.com> 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.
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 8f2f1bb18c9c..2dcdaaf358ce 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -8423,7 +8423,14 @@ void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size) bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg) { /* if zswap is disabled, do not block pages going to the swapping device */ - return !is_zswap_enabled() || !memcg || READ_ONCE(memcg->zswap_writeback); + if (!is_zswap_enabled()) + return true; + + for (; memcg; memcg = parent_mem_cgroup(memcg)) + if (!READ_ONCE(memcg->zswap_writeback)) + return false; + + return true; } static u64 zswap_current_read(struct cgroup_subsys_state *css,
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. [1] https://wiki.archlinux.org/title/Power_management/Suspend_and_hibernate#Disable_zswap_writeback_to_use_the_swap_space_only_for_hibernation [2] https://github.com/systemd/systemd/pull/31734 Signed-off-by: Mike Yuan <me@yhndnzj.com> --- mm/memcontrol.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)