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

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

[Mike Yuan]

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

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,
-- 
2.46.0

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

[Nhat Pham]

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

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

[Nhat Pham]

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

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

[Mike Yuan]

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?

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

[Nhat Pham]

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

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

[Mike Yuan]

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

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

[Yosry Ahmed]

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

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

[Mike Yuan]

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

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

[Nhat Pham]

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>

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

[Andrew Morton]

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?

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

[Yosry Ahmed]

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?

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

[Nhat Pham]

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

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

[Yosry Ahmed]

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?

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

[Andrew Morton]

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:?

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

[Yosry Ahmed]

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.

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

[Nhat Pham]

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.

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

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Yosry Ahmed]

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.