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

[PATCH v2 1/2] 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 inconsistency 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 inheriting 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

Changes in v2:
- Actually base on latest tree (is_zswap_enabled() -> zswap_is_enabled())
- Updated Documentation/admin-guide/cgroup-v2.rst to reflect the change

Link to v1: https://lore.kernel.org/linux-kernel/20240814171800.23558-1-me@yhndnzj.com/

Cc: Nhat Pham <nphamcs@gmail.com>
Cc: Yosry Ahmed <yosryahmed@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Andrew Morton <akpm@linux-foundation.org>

Signed-off-by: Mike Yuan <me@yhndnzj.com>
Reviewed-by: Nhat Pham <nphamcs@gmail.com>
---
 Documentation/admin-guide/cgroup-v2.rst | 5 ++++-
 mm/memcontrol.c                         | 9 ++++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index 86311c2907cd..80906cea4264 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -1719,7 +1719,10 @@ The following nested keys are defined.
   memory.zswap.writeback
 	A read-write single value file. The default value is "1". The
 	initial value of the root cgroup is 1, and when a new cgroup is
-	created, it inherits the current value of its parent.
+	created, it inherits the current value of its parent. Note that
+	this setting is hierarchical, i.e. the writeback would be
+	implicitly disabled for child cgroups if the upper hierarchy
+	does so.
 
 	When this is set to 0, all swapping attempts to swapping devices
 	are disabled. This included both zswap writebacks, and swapping due
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f29157288b7d..327b2b030639 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5320,7 +5320,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 !zswap_is_enabled() || !memcg || READ_ONCE(memcg->zswap_writeback);
+	if (!zswap_is_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,

base-commit: d07b43284ab356daf7ec5ae1858a16c1c7b6adab
-- 
2.46.0

[PATCH v2 2/2] selftests: test_zswap: add test for hierarchical zswap.writeback

[Mike Yuan]

Ensure that zswap.writeback check goes up the cgroup tree.

Signed-off-by: Mike Yuan <me@yhndnzj.com>
---
 tools/testing/selftests/cgroup/test_zswap.c | 69 ++++++++++++++-------
 1 file changed, 48 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
index 190096017f80..7da6f9dc1066 100644
--- a/tools/testing/selftests/cgroup/test_zswap.c
+++ b/tools/testing/selftests/cgroup/test_zswap.c
@@ -263,15 +263,13 @@ static int test_zswapin(const char *root)
 static int attempt_writeback(const char *cgroup, void *arg)
 {
 	long pagesize = sysconf(_SC_PAGESIZE);
-	char *test_group = arg;
 	size_t memsize = MB(4);
 	char buf[pagesize];
 	long zswap_usage;
-	bool wb_enabled;
+	bool wb_enabled = *(bool *) arg;
 	int ret = -1;
 	char *mem;
 
-	wb_enabled = cg_read_long(test_group, "memory.zswap.writeback");
 	mem = (char *)malloc(memsize);
 	if (!mem)
 		return ret;
@@ -288,12 +286,12 @@ static int attempt_writeback(const char *cgroup, void *arg)
 		memcpy(&mem[i], buf, pagesize);
 
 	/* Try and reclaim allocated memory */
-	if (cg_write_numeric(test_group, "memory.reclaim", memsize)) {
+	if (cg_write_numeric(cgroup, "memory.reclaim", memsize)) {
 		ksft_print_msg("Failed to reclaim all of the requested memory\n");
 		goto out;
 	}
 
-	zswap_usage = cg_read_long(test_group, "memory.zswap.current");
+	zswap_usage = cg_read_long(cgroup, "memory.zswap.current");
 
 	/* zswpin */
 	for (int i = 0; i < memsize; i += pagesize) {
@@ -303,7 +301,7 @@ static int attempt_writeback(const char *cgroup, void *arg)
 		}
 	}
 
-	if (cg_write_numeric(test_group, "memory.zswap.max", zswap_usage/2))
+	if (cg_write_numeric(cgroup, "memory.zswap.max", zswap_usage/2))
 		goto out;
 
 	/*
@@ -312,7 +310,7 @@ static int attempt_writeback(const char *cgroup, void *arg)
 	 * If writeback is disabled, memory reclaim will fail as zswap is limited and
 	 * it can't writeback to swap.
 	 */
-	ret = cg_write_numeric(test_group, "memory.reclaim", memsize);
+	ret = cg_write_numeric(cgroup, "memory.reclaim", memsize);
 	if (!wb_enabled)
 		ret = (ret == -EAGAIN) ? 0 : -1;
 
@@ -321,12 +319,38 @@ static int attempt_writeback(const char *cgroup, void *arg)
 	return ret;
 }
 
+static int test_zswap_writeback_one(const char *cgroup, bool wb)
+{
+	long zswpwb_before, zswpwb_after;
+
+	zswpwb_before = get_cg_wb_count(cgroup);
+	if (zswpwb_before != 0) {
+		ksft_print_msg("zswpwb_before = %ld instead of 0\n", zswpwb_before);
+		return -1;
+	}
+
+	if (cg_run(cgroup, attempt_writeback, (void *) &wb))
+		return -1;
+
+	/* Verify that zswap writeback occurred only if writeback was enabled */
+	zswpwb_after = get_cg_wb_count(cgroup);
+	if (zswpwb_after < 0)
+		return -1;
+
+	if (wb != !!zswpwb_after) {
+		ksft_print_msg("zswpwb_after is %ld while wb is %s",
+				zswpwb_after, wb ? "enabled" : "disabled");
+		return -1;
+	}
+
+	return 0;
+}
+
 /* Test to verify the zswap writeback path */
 static int test_zswap_writeback(const char *root, bool wb)
 {
-	long zswpwb_before, zswpwb_after;
 	int ret = KSFT_FAIL;
-	char *test_group;
+	char *test_group, *test_group_child = NULL;
 
 	test_group = cg_name(root, "zswap_writeback_test");
 	if (!test_group)
@@ -336,29 +360,32 @@ static int test_zswap_writeback(const char *root, bool wb)
 	if (cg_write(test_group, "memory.zswap.writeback", wb ? "1" : "0"))
 		goto out;
 
-	zswpwb_before = get_cg_wb_count(test_group);
-	if (zswpwb_before != 0) {
-		ksft_print_msg("zswpwb_before = %ld instead of 0\n", zswpwb_before);
+	if (test_zswap_writeback_one(test_group, wb))
 		goto out;
-	}
 
-	if (cg_run(test_group, attempt_writeback, (void *) test_group))
+	if (cg_write(test_group, "memory.zswap.max", "max"))
+		goto out;
+	if (cg_write(test_group, "cgroup.subtree_control", "+memory"))
 		goto out;
 
-	/* Verify that zswap writeback occurred only if writeback was enabled */
-	zswpwb_after = get_cg_wb_count(test_group);
-	if (zswpwb_after < 0)
+	test_group_child = cg_name(test_group, "zswap_writeback_test_child");
+	if (!test_group_child)
+		goto out;
+	if (cg_create(test_group_child))
+		goto out;
+	if (cg_write(test_group_child, "memory.zswap.writeback", "1"))
 		goto out;
 
-	if (wb != !!zswpwb_after) {
-		ksft_print_msg("zswpwb_after is %ld while wb is %s",
-				zswpwb_after, wb ? "enabled" : "disabled");
+	if (test_zswap_writeback_one(test_group_child, wb))
 		goto out;
-	}
 
 	ret = KSFT_PASS;
 
 out:
+	if (test_group_child) {
+		cg_destroy(test_group_child);
+		free(test_group_child);
+	}
 	cg_destroy(test_group);
 	free(test_group);
 	return ret;
-- 
2.46.0

Re: [PATCH v2 2/2] selftests: test_zswap: add test for hierarchical zswap.writeback

[Yosry Ahmed]

On Fri, Aug 16, 2024 at 7:44 AM Mike Yuan <me@yhndnzj.com> wrote:
>
> Ensure that zswap.writeback check goes up the cgroup tree.

Too concise :) Perhaps a little bit of description of what you are
doing would be helpful.

>
> Signed-off-by: Mike Yuan <me@yhndnzj.com>
> ---
>  tools/testing/selftests/cgroup/test_zswap.c | 69 ++++++++++++++-------
>  1 file changed, 48 insertions(+), 21 deletions(-)
>
> diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
> index 190096017f80..7da6f9dc1066 100644
> --- a/tools/testing/selftests/cgroup/test_zswap.c
> +++ b/tools/testing/selftests/cgroup/test_zswap.c
> @@ -263,15 +263,13 @@ static int test_zswapin(const char *root)
>  static int attempt_writeback(const char *cgroup, void *arg)
>  {
>         long pagesize = sysconf(_SC_PAGESIZE);
> -       char *test_group = arg;
>         size_t memsize = MB(4);
>         char buf[pagesize];
>         long zswap_usage;
> -       bool wb_enabled;
> +       bool wb_enabled = *(bool *) arg;
>         int ret = -1;
>         char *mem;
>
> -       wb_enabled = cg_read_long(test_group, "memory.zswap.writeback");
>         mem = (char *)malloc(memsize);
>         if (!mem)
>                 return ret;
> @@ -288,12 +286,12 @@ static int attempt_writeback(const char *cgroup, void *arg)
>                 memcpy(&mem[i], buf, pagesize);
>
>         /* Try and reclaim allocated memory */
> -       if (cg_write_numeric(test_group, "memory.reclaim", memsize)) {
> +       if (cg_write_numeric(cgroup, "memory.reclaim", memsize)) {
>                 ksft_print_msg("Failed to reclaim all of the requested memory\n");
>                 goto out;
>         }
>
> -       zswap_usage = cg_read_long(test_group, "memory.zswap.current");
> +       zswap_usage = cg_read_long(cgroup, "memory.zswap.current");
>
>         /* zswpin */
>         for (int i = 0; i < memsize; i += pagesize) {
> @@ -303,7 +301,7 @@ static int attempt_writeback(const char *cgroup, void *arg)
>                 }
>         }
>
> -       if (cg_write_numeric(test_group, "memory.zswap.max", zswap_usage/2))
> +       if (cg_write_numeric(cgroup, "memory.zswap.max", zswap_usage/2))
>                 goto out;
>
>         /*
> @@ -312,7 +310,7 @@ static int attempt_writeback(const char *cgroup, void *arg)
>          * If writeback is disabled, memory reclaim will fail as zswap is limited and
>          * it can't writeback to swap.
>          */
> -       ret = cg_write_numeric(test_group, "memory.reclaim", memsize);
> +       ret = cg_write_numeric(cgroup, "memory.reclaim", memsize);
>         if (!wb_enabled)
>                 ret = (ret == -EAGAIN) ? 0 : -1;
>
> @@ -321,12 +319,38 @@ static int attempt_writeback(const char *cgroup, void *arg)
>         return ret;
>  }
>
> +static int test_zswap_writeback_one(const char *cgroup, bool wb)
> +{
> +       long zswpwb_before, zswpwb_after;
> +
> +       zswpwb_before = get_cg_wb_count(cgroup);
> +       if (zswpwb_before != 0) {
> +               ksft_print_msg("zswpwb_before = %ld instead of 0\n", zswpwb_before);
> +               return -1;
> +       }
> +
> +       if (cg_run(cgroup, attempt_writeback, (void *) &wb))
> +               return -1;
> +
> +       /* Verify that zswap writeback occurred only if writeback was enabled */
> +       zswpwb_after = get_cg_wb_count(cgroup);
> +       if (zswpwb_after < 0)
> +               return -1;
> +
> +       if (wb != !!zswpwb_after) {
> +               ksft_print_msg("zswpwb_after is %ld while wb is %s",
> +                               zswpwb_after, wb ? "enabled" : "disabled");
> +               return -1;
> +       }
> +
> +       return 0;
> +}
> +
>  /* Test to verify the zswap writeback path */
>  static int test_zswap_writeback(const char *root, bool wb)
>  {
> -       long zswpwb_before, zswpwb_after;
>         int ret = KSFT_FAIL;
> -       char *test_group;
> +       char *test_group, *test_group_child = NULL;
>
>         test_group = cg_name(root, "zswap_writeback_test");
>         if (!test_group)
> @@ -336,29 +360,32 @@ static int test_zswap_writeback(const char *root, bool wb)
>         if (cg_write(test_group, "memory.zswap.writeback", wb ? "1" : "0"))
>                 goto out;
>
> -       zswpwb_before = get_cg_wb_count(test_group);
> -       if (zswpwb_before != 0) {
> -               ksft_print_msg("zswpwb_before = %ld instead of 0\n", zswpwb_before);
> +       if (test_zswap_writeback_one(test_group, wb))
>                 goto out;
> -       }
>
> -       if (cg_run(test_group, attempt_writeback, (void *) test_group))
> +       if (cg_write(test_group, "memory.zswap.max", "max"))
> +               goto out;

Why is this needed? Isn't this the default value?

> +       if (cg_write(test_group, "cgroup.subtree_control", "+memory"))
>                 goto out;
>
> -       /* Verify that zswap writeback occurred only if writeback was enabled */
> -       zswpwb_after = get_cg_wb_count(test_group);
> -       if (zswpwb_after < 0)
> +       test_group_child = cg_name(test_group, "zswap_writeback_test_child");
> +       if (!test_group_child)
> +               goto out;
> +       if (cg_create(test_group_child))
> +               goto out;

I'd rather have all the hierarchy setup at the beginning of the test,
before the actual test logic. I don't feel strongly about it though.

> +       if (cg_write(test_group_child, "memory.zswap.writeback", "1"))
>                 goto out;

Is the idea here that we always hardcode the child's zswap.writeback
to 1, and the parent's zswap.writeback changes from 0 to 1, and we
check that the parent's value is what matters?
I think we need a comment here.

TBH, I expected a separate test that checks different combinations of
parent and child values (e.g. also verifies that if the parent is
enabled but child is disabled, writeback is disabled).

>
> -       if (wb != !!zswpwb_after) {
> -               ksft_print_msg("zswpwb_after is %ld while wb is %s",
> -                               zswpwb_after, wb ? "enabled" : "disabled");
> +       if (test_zswap_writeback_one(test_group_child, wb))
>                 goto out;
> -       }
>
>         ret = KSFT_PASS;
>
>  out:
> +       if (test_group_child) {
> +               cg_destroy(test_group_child);
> +               free(test_group_child);
> +       }
>         cg_destroy(test_group);
>         free(test_group);
>         return ret;
> --
> 2.46.0
>
>

Re: [PATCH v2 2/2] selftests: test_zswap: add test for hierarchical zswap.writeback

[Mike Yuan]

On 2024-08-19 at 12:19 -0700, Yosry Ahmed wrote:
> On Fri, Aug 16, 2024 at 7:44 AM Mike Yuan <me@yhndnzj.com> wrote:
> > 
> > Ensure that zswap.writeback check goes up the cgroup tree.
> 
> Too concise :) Perhaps a little bit of description of what you are
> doing would be helpful.

The patch has been merged into mm-unstable tree. Do I need to
send a v3 to resolve the comments?

> > 
> > Signed-off-by: Mike Yuan <me@yhndnzj.com>
> > ---
> >  tools/testing/selftests/cgroup/test_zswap.c | 69 ++++++++++++++---
> > ----
> >  1 file changed, 48 insertions(+), 21 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/cgroup/test_zswap.c
> > b/tools/testing/selftests/cgroup/test_zswap.c
> > index 190096017f80..7da6f9dc1066 100644
> > --- a/tools/testing/selftests/cgroup/test_zswap.c
> > +++ b/tools/testing/selftests/cgroup/test_zswap.c
> > @@ -263,15 +263,13 @@ static int test_zswapin(const char *root)
> >  static int attempt_writeback(const char *cgroup, void *arg)
> >  {
> >         long pagesize = sysconf(_SC_PAGESIZE);
> > -       char *test_group = arg;
> >         size_t memsize = MB(4);
> >         char buf[pagesize];
> >         long zswap_usage;
> > -       bool wb_enabled;
> > +       bool wb_enabled = *(bool *) arg;
> >         int ret = -1;
> >         char *mem;
> > 
> > -       wb_enabled = cg_read_long(test_group,
> > "memory.zswap.writeback");
> >         mem = (char *)malloc(memsize);
> >         if (!mem)
> >                 return ret;
> > @@ -288,12 +286,12 @@ static int attempt_writeback(const char
> > *cgroup, void *arg)
> >                 memcpy(&mem[i], buf, pagesize);
> > 
> >         /* Try and reclaim allocated memory */
> > -       if (cg_write_numeric(test_group, "memory.reclaim",
> > memsize)) {
> > +       if (cg_write_numeric(cgroup, "memory.reclaim", memsize)) {
> >                 ksft_print_msg("Failed to reclaim all of the
> > requested memory\n");
> >                 goto out;
> >         }
> > 
> > -       zswap_usage = cg_read_long(test_group,
> > "memory.zswap.current");
> > +       zswap_usage = cg_read_long(cgroup, "memory.zswap.current");
> > 
> >         /* zswpin */
> >         for (int i = 0; i < memsize; i += pagesize) {
> > @@ -303,7 +301,7 @@ static int attempt_writeback(const char
> > *cgroup, void *arg)
> >                 }
> >         }
> > 
> > -       if (cg_write_numeric(test_group, "memory.zswap.max",
> > zswap_usage/2))
> > +       if (cg_write_numeric(cgroup, "memory.zswap.max",
> > zswap_usage/2))
> >                 goto out;
> > 
> >         /*
> > @@ -312,7 +310,7 @@ static int attempt_writeback(const char
> > *cgroup, void *arg)
> >          * If writeback is disabled, memory reclaim will fail as
> > zswap is limited and
> >          * it can't writeback to swap.
> >          */
> > -       ret = cg_write_numeric(test_group, "memory.reclaim",
> > memsize);
> > +       ret = cg_write_numeric(cgroup, "memory.reclaim", memsize);
> >         if (!wb_enabled)
> >                 ret = (ret == -EAGAIN) ? 0 : -1;
> > 
> > @@ -321,12 +319,38 @@ static int attempt_writeback(const char
> > *cgroup, void *arg)
> >         return ret;
> >  }
> > 
> > +static int test_zswap_writeback_one(const char *cgroup, bool wb)
> > +{
> > +       long zswpwb_before, zswpwb_after;
> > +
> > +       zswpwb_before = get_cg_wb_count(cgroup);
> > +       if (zswpwb_before != 0) {
> > +               ksft_print_msg("zswpwb_before = %ld instead of
> > 0\n", zswpwb_before);
> > +               return -1;
> > +       }
> > +
> > +       if (cg_run(cgroup, attempt_writeback, (void *) &wb))
> > +               return -1;
> > +
> > +       /* Verify that zswap writeback occurred only if writeback
> > was enabled */
> > +       zswpwb_after = get_cg_wb_count(cgroup);
> > +       if (zswpwb_after < 0)
> > +               return -1;
> > +
> > +       if (wb != !!zswpwb_after) {
> > +               ksft_print_msg("zswpwb_after is %ld while wb is
> > %s",
> > +                               zswpwb_after, wb ? "enabled" :
> > "disabled");
> > +               return -1;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  /* Test to verify the zswap writeback path */
> >  static int test_zswap_writeback(const char *root, bool wb)
> >  {
> > -       long zswpwb_before, zswpwb_after;
> >         int ret = KSFT_FAIL;
> > -       char *test_group;
> > +       char *test_group, *test_group_child = NULL;
> > 
> >         test_group = cg_name(root, "zswap_writeback_test");
> >         if (!test_group)
> > @@ -336,29 +360,32 @@ static int test_zswap_writeback(const char
> > *root, bool wb)
> >         if (cg_write(test_group, "memory.zswap.writeback", wb ? "1"
> > : "0"))
> >                 goto out;
> > 
> > -       zswpwb_before = get_cg_wb_count(test_group);
> > -       if (zswpwb_before != 0) {
> > -               ksft_print_msg("zswpwb_before = %ld instead of
> > 0\n", zswpwb_before);
> > +       if (test_zswap_writeback_one(test_group, wb))
> >                 goto out;
> > -       }
> > 
> > -       if (cg_run(test_group, attempt_writeback, (void *)
> > test_group))
> > +       if (cg_write(test_group, "memory.zswap.max", "max"))
> > +               goto out;
> 
> Why is this needed? Isn't this the default value?

attempt_writeback() would modify it.

> > +       if (cg_write(test_group, "cgroup.subtree_control",
> > "+memory"))
> >                 goto out;
> > 
> > -       /* Verify that zswap writeback occurred only if writeback
> > was enabled */
> > -       zswpwb_after = get_cg_wb_count(test_group);
> > -       if (zswpwb_after < 0)
> > +       test_group_child = cg_name(test_group,
> > "zswap_writeback_test_child");
> > +       if (!test_group_child)
> > +               goto out;
> > +       if (cg_create(test_group_child))
> > +               goto out;
> 
> I'd rather have all the hierarchy setup at the beginning of the test,
> before the actual test logic. I don't feel strongly about it though.
> 
> > +       if (cg_write(test_group_child, "memory.zswap.writeback",
> > "1"))
> >                 goto out;
> 
> Is the idea here that we always hardcode the child's zswap.writeback
> to 1, and the parent's zswap.writeback changes from 0 to 1, and we
> check that the parent's value is what matters?
> I think we need a comment here.

Yes, indeed.

> TBH, I expected a separate test that checks different combinations of
> parent and child values (e.g. also verifies that if the parent is
> enabled but child is disabled, writeback is disabled).

That's (implicitly) covered by the test itself IIUC? The parent cgroup
here is in turn the child of root cgroup.

> > 
> > -       if (wb != !!zswpwb_after) {
> > -               ksft_print_msg("zswpwb_after is %ld while wb is
> > %s",
> > -                               zswpwb_after, wb ? "enabled" :
> > "disabled");
> > +       if (test_zswap_writeback_one(test_group_child, wb))
> >                 goto out;
> > -       }
> > 
> >         ret = KSFT_PASS;
> > 
> >  out:
> > +       if (test_group_child) {
> > +               cg_destroy(test_group_child);
> > +               free(test_group_child);
> > +       }
> >         cg_destroy(test_group);
> >         free(test_group);
> >         return ret;
> > --
> > 2.46.0
> > 
> > 

Re: [PATCH v2 2/2] selftests: test_zswap: add test for hierarchical zswap.writeback

[Yosry Ahmed]

On Tue, Aug 20, 2024 at 2:44 AM Mike Yuan <me@yhndnzj.com> wrote:
>
> On 2024-08-19 at 12:19 -0700, Yosry Ahmed wrote:
> > On Fri, Aug 16, 2024 at 7:44 AM Mike Yuan <me@yhndnzj.com> wrote:
> > >
> > > Ensure that zswap.writeback check goes up the cgroup tree.
> >
> > Too concise :) Perhaps a little bit of description of what you are
> > doing would be helpful.
>
> The patch has been merged into mm-unstable tree. Do I need to
> send a v3 to resolve the comments?

You can send a new version and Andrew usually replaces them. If the
changes are too trivial sometimes Andrew is nice enough to make
amendments directly :)

>
> > >
> > > Signed-off-by: Mike Yuan <me@yhndnzj.com>
> > > ---
> > >  tools/testing/selftests/cgroup/test_zswap.c | 69 ++++++++++++++---
> > > ----
> > >  1 file changed, 48 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/cgroup/test_zswap.c
> > > b/tools/testing/selftests/cgroup/test_zswap.c
> > > index 190096017f80..7da6f9dc1066 100644
> > > --- a/tools/testing/selftests/cgroup/test_zswap.c
> > > +++ b/tools/testing/selftests/cgroup/test_zswap.c
> > > @@ -263,15 +263,13 @@ static int test_zswapin(const char *root)
> > >  static int attempt_writeback(const char *cgroup, void *arg)
> > >  {
> > >         long pagesize = sysconf(_SC_PAGESIZE);
> > > -       char *test_group = arg;
> > >         size_t memsize = MB(4);
> > >         char buf[pagesize];
> > >         long zswap_usage;
> > > -       bool wb_enabled;
> > > +       bool wb_enabled = *(bool *) arg;
> > >         int ret = -1;
> > >         char *mem;
> > >
> > > -       wb_enabled = cg_read_long(test_group,
> > > "memory.zswap.writeback");
> > >         mem = (char *)malloc(memsize);
> > >         if (!mem)
> > >                 return ret;
> > > @@ -288,12 +286,12 @@ static int attempt_writeback(const char
> > > *cgroup, void *arg)
> > >                 memcpy(&mem[i], buf, pagesize);
> > >
> > >         /* Try and reclaim allocated memory */
> > > -       if (cg_write_numeric(test_group, "memory.reclaim",
> > > memsize)) {
> > > +       if (cg_write_numeric(cgroup, "memory.reclaim", memsize)) {
> > >                 ksft_print_msg("Failed to reclaim all of the
> > > requested memory\n");
> > >                 goto out;
> > >         }
> > >
> > > -       zswap_usage = cg_read_long(test_group,
> > > "memory.zswap.current");
> > > +       zswap_usage = cg_read_long(cgroup, "memory.zswap.current");
> > >
> > >         /* zswpin */
> > >         for (int i = 0; i < memsize; i += pagesize) {
> > > @@ -303,7 +301,7 @@ static int attempt_writeback(const char
> > > *cgroup, void *arg)
> > >                 }
> > >         }
> > >
> > > -       if (cg_write_numeric(test_group, "memory.zswap.max",
> > > zswap_usage/2))
> > > +       if (cg_write_numeric(cgroup, "memory.zswap.max",
> > > zswap_usage/2))
> > >                 goto out;
> > >
> > >         /*
> > > @@ -312,7 +310,7 @@ static int attempt_writeback(const char
> > > *cgroup, void *arg)
> > >          * If writeback is disabled, memory reclaim will fail as
> > > zswap is limited and
> > >          * it can't writeback to swap.
> > >          */
> > > -       ret = cg_write_numeric(test_group, "memory.reclaim",
> > > memsize);
> > > +       ret = cg_write_numeric(cgroup, "memory.reclaim", memsize);
> > >         if (!wb_enabled)
> > >                 ret = (ret == -EAGAIN) ? 0 : -1;
> > >
> > > @@ -321,12 +319,38 @@ static int attempt_writeback(const char
> > > *cgroup, void *arg)
> > >         return ret;
> > >  }
> > >
> > > +static int test_zswap_writeback_one(const char *cgroup, bool wb)
> > > +{
> > > +       long zswpwb_before, zswpwb_after;
> > > +
> > > +       zswpwb_before = get_cg_wb_count(cgroup);
> > > +       if (zswpwb_before != 0) {
> > > +               ksft_print_msg("zswpwb_before = %ld instead of
> > > 0\n", zswpwb_before);
> > > +               return -1;
> > > +       }
> > > +
> > > +       if (cg_run(cgroup, attempt_writeback, (void *) &wb))
> > > +               return -1;
> > > +
> > > +       /* Verify that zswap writeback occurred only if writeback
> > > was enabled */
> > > +       zswpwb_after = get_cg_wb_count(cgroup);
> > > +       if (zswpwb_after < 0)
> > > +               return -1;
> > > +
> > > +       if (wb != !!zswpwb_after) {
> > > +               ksft_print_msg("zswpwb_after is %ld while wb is
> > > %s",
> > > +                               zswpwb_after, wb ? "enabled" :
> > > "disabled");
> > > +               return -1;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  /* Test to verify the zswap writeback path */
> > >  static int test_zswap_writeback(const char *root, bool wb)
> > >  {
> > > -       long zswpwb_before, zswpwb_after;
> > >         int ret = KSFT_FAIL;
> > > -       char *test_group;
> > > +       char *test_group, *test_group_child = NULL;
> > >
> > >         test_group = cg_name(root, "zswap_writeback_test");
> > >         if (!test_group)
> > > @@ -336,29 +360,32 @@ static int test_zswap_writeback(const char
> > > *root, bool wb)
> > >         if (cg_write(test_group, "memory.zswap.writeback", wb ? "1"
> > > : "0"))
> > >                 goto out;
> > >
> > > -       zswpwb_before = get_cg_wb_count(test_group);
> > > -       if (zswpwb_before != 0) {
> > > -               ksft_print_msg("zswpwb_before = %ld instead of
> > > 0\n", zswpwb_before);
> > > +       if (test_zswap_writeback_one(test_group, wb))
> > >                 goto out;
> > > -       }
> > >
> > > -       if (cg_run(test_group, attempt_writeback, (void *)
> > > test_group))
> > > +       if (cg_write(test_group, "memory.zswap.max", "max"))
> > > +               goto out;
> >
> > Why is this needed? Isn't this the default value?
>
> attempt_writeback() would modify it.

Oh yes, missed that.

>
> > > +       if (cg_write(test_group, "cgroup.subtree_control",
> > > "+memory"))
> > >                 goto out;
> > >
> > > -       /* Verify that zswap writeback occurred only if writeback
> > > was enabled */
> > > -       zswpwb_after = get_cg_wb_count(test_group);
> > > -       if (zswpwb_after < 0)
> > > +       test_group_child = cg_name(test_group,
> > > "zswap_writeback_test_child");
> > > +       if (!test_group_child)
> > > +               goto out;
> > > +       if (cg_create(test_group_child))
> > > +               goto out;
> >
> > I'd rather have all the hierarchy setup at the beginning of the test,
> > before the actual test logic. I don't feel strongly about it though.
> >
> > > +       if (cg_write(test_group_child, "memory.zswap.writeback",
> > > "1"))
> > >                 goto out;
> >
> > Is the idea here that we always hardcode the child's zswap.writeback
> > to 1, and the parent's zswap.writeback changes from 0 to 1, and we
> > check that the parent's value is what matters?
> > I think we need a comment here.
>
> Yes, indeed.
>
> > TBH, I expected a separate test that checks different combinations of
> > parent and child values (e.g. also verifies that if the parent is
> > enabled but child is disabled, writeback is disabled).
>
> That's (implicitly) covered by the test itself IIUC? The parent cgroup
> here is in turn the child of root cgroup.

This assumes that the root has zswap writeback enabled, but that's a
fair assumption as otherwise all the writeback tests will fail.

TBH I'd prefer a standalone test rather than these implicitly tested scenarios.

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

[Yosry Ahmed]

On Fri, Aug 16, 2024 at 7:44 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 inconsistency 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 inheriting 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
>
> Changes in v2:
> - Actually base on latest tree (is_zswap_enabled() -> zswap_is_enabled())
> - Updated Documentation/admin-guide/cgroup-v2.rst to reflect the change
>
> Link to v1: https://lore.kernel.org/linux-kernel/20240814171800.23558-1-me@yhndnzj.com/
>
> Cc: Nhat Pham <nphamcs@gmail.com>
> Cc: Yosry Ahmed <yosryahmed@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
>
> Signed-off-by: Mike Yuan <me@yhndnzj.com>
> Reviewed-by: Nhat Pham <nphamcs@gmail.com>

LGTM,
Acked-by: Yosry Ahmed <yosryahmed@google.com>

> ---
>  Documentation/admin-guide/cgroup-v2.rst | 5 ++++-
>  mm/memcontrol.c                         | 9 ++++++++-
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index 86311c2907cd..80906cea4264 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -1719,7 +1719,10 @@ The following nested keys are defined.
>    memory.zswap.writeback
>         A read-write single value file. The default value is "1". The
>         initial value of the root cgroup is 1, and when a new cgroup is
> -       created, it inherits the current value of its parent.
> +       created, it inherits the current value of its parent. Note that
> +       this setting is hierarchical, i.e. the writeback would be
> +       implicitly disabled for child cgroups if the upper hierarchy
> +       does so.
>
>         When this is set to 0, all swapping attempts to swapping devices
>         are disabled. This included both zswap writebacks, and swapping due
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index f29157288b7d..327b2b030639 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5320,7 +5320,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 !zswap_is_enabled() || !memcg || READ_ONCE(memcg->zswap_writeback);
> +       if (!zswap_is_enabled())
> +               return true;

This is orthogonal to this patch, but I just realized that we
completely ignore memory.zswap_writeback if zswap is disabled. This
means that if a cgroup has disabled writeback, then zswap is globally
disabled for some reason, we stop respecting the cgroup knob. I guess
the rationale could be that we want to help get pages out of zswap as
much as possible to honor zswap's disablement? Nhat, did I get that
right?

I feel like it's a little bit odd to be honest, but I don't have a
strong opinion on it. Maybe we should document this behavior better.


> +
> +       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,
>
> base-commit: d07b43284ab356daf7ec5ae1858a16c1c7b6adab
> --
> 2.46.0
>
>

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

[Mike Yuan]

On 2024-08-19 at 12:09 -0700, Yosry Ahmed wrote:
> On Fri, Aug 16, 2024 at 7:44 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 inconsistency 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 inheriting 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
> > 
> > Changes in v2:
> > - Actually base on latest tree (is_zswap_enabled() ->
> > zswap_is_enabled())
> > - Updated Documentation/admin-guide/cgroup-v2.rst to reflect the
> > change
> > 
> > Link to v1:
> > https://lore.kernel.org/linux-kernel/20240814171800.23558-1-me@yhndnzj.com/
> > 
> > Cc: Nhat Pham <nphamcs@gmail.com>
> > Cc: Yosry Ahmed <yosryahmed@google.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > 
> > Signed-off-by: Mike Yuan <me@yhndnzj.com>
> > Reviewed-by: Nhat Pham <nphamcs@gmail.com>
> 
> LGTM,
> Acked-by: Yosry Ahmed <yosryahmed@google.com>
> 
> > ---
> >  Documentation/admin-guide/cgroup-v2.rst | 5 ++++-
> >  mm/memcontrol.c                         | 9 ++++++++-
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/cgroup-v2.rst
> > b/Documentation/admin-guide/cgroup-v2.rst
> > index 86311c2907cd..80906cea4264 100644
> > --- a/Documentation/admin-guide/cgroup-v2.rst
> > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > @@ -1719,7 +1719,10 @@ The following nested keys are defined.
> >    memory.zswap.writeback
> >         A read-write single value file. The default value is "1".
> > The
> >         initial value of the root cgroup is 1, and when a new
> > cgroup is
> > -       created, it inherits the current value of its parent.
> > +       created, it inherits the current value of its parent. Note
> > that
> > +       this setting is hierarchical, i.e. the writeback would be
> > +       implicitly disabled for child cgroups if the upper
> > hierarchy
> > +       does so.
> > 
> >         When this is set to 0, all swapping attempts to swapping
> > devices
> >         are disabled. This included both zswap writebacks, and
> > swapping due
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index f29157288b7d..327b2b030639 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -5320,7 +5320,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 !zswap_is_enabled() || !memcg || READ_ONCE(memcg-
> > >zswap_writeback);
> > +       if (!zswap_is_enabled())
> > +               return true;
> 
> This is orthogonal to this patch, but I just realized that we
> completely ignore memory.zswap_writeback if zswap is disabled. This
> means that if a cgroup has disabled writeback, then zswap is globally
> disabled for some reason, we stop respecting the cgroup knob. I guess
> the rationale could be that we want to help get pages out of zswap as
> much as possible to honor zswap's disablement? Nhat, did I get that
> right?

Hmm, I think the current behavior makes more sense. If zswap is
completely
disabled, it seems intuitive that zswap-related knobs lose their
effect.

> I feel like it's a little bit odd to be honest, but I don't have a
> strong opinion on it. Maybe we should document this behavior better.

But clarify this in the documentation certainly sounds good :)

> 
> > +
> > +       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,
> > 
> > base-commit: d07b43284ab356daf7ec5ae1858a16c1c7b6adab
> > --
> > 2.46.0
> > 
> > 

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

[Nhat Pham]

On Tue, Aug 20, 2024 at 5:38 AM Mike Yuan <me@yhndnzj.com> wrote:
>
> On 2024-08-19 at 12:09 -0700, Yosry Ahmed wrote:
> > On Fri, Aug 16, 2024 at 7:44 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 inconsistency 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 inheriting 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
> > >
> > > Changes in v2:
> > > - Actually base on latest tree (is_zswap_enabled() ->
> > > zswap_is_enabled())
> > > - Updated Documentation/admin-guide/cgroup-v2.rst to reflect the
> > > change
> > >
> > > Link to v1:
> > > https://lore.kernel.org/linux-kernel/20240814171800.23558-1-me@yhndnzj.com/
> > >
> > > Cc: Nhat Pham <nphamcs@gmail.com>
> > > Cc: Yosry Ahmed <yosryahmed@google.com>
> > > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > >
> > > Signed-off-by: Mike Yuan <me@yhndnzj.com>
> > > Reviewed-by: Nhat Pham <nphamcs@gmail.com>
> >
> > LGTM,
> > Acked-by: Yosry Ahmed <yosryahmed@google.com>
> >
> > > ---
> > >  Documentation/admin-guide/cgroup-v2.rst | 5 ++++-
> > >  mm/memcontrol.c                         | 9 ++++++++-
> > >  2 files changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/admin-guide/cgroup-v2.rst
> > > b/Documentation/admin-guide/cgroup-v2.rst
> > > index 86311c2907cd..80906cea4264 100644
> > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > @@ -1719,7 +1719,10 @@ The following nested keys are defined.
> > >    memory.zswap.writeback
> > >         A read-write single value file. The default value is "1".
> > > The
> > >         initial value of the root cgroup is 1, and when a new
> > > cgroup is
> > > -       created, it inherits the current value of its parent.
> > > +       created, it inherits the current value of its parent. Note
> > > that
> > > +       this setting is hierarchical, i.e. the writeback would be
> > > +       implicitly disabled for child cgroups if the upper
> > > hierarchy
> > > +       does so.
> > >
> > >         When this is set to 0, all swapping attempts to swapping
> > > devices
> > >         are disabled. This included both zswap writebacks, and
> > > swapping due
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index f29157288b7d..327b2b030639 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -5320,7 +5320,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 !zswap_is_enabled() || !memcg || READ_ONCE(memcg-
> > > >zswap_writeback);
> > > +       if (!zswap_is_enabled())
> > > +               return true;
> >
> > This is orthogonal to this patch, but I just realized that we
> > completely ignore memory.zswap_writeback if zswap is disabled. This
> > means that if a cgroup has disabled writeback, then zswap is globally
> > disabled for some reason, we stop respecting the cgroup knob. I guess
> > the rationale could be that we want to help get pages out of zswap as
> > much as possible to honor zswap's disablement? Nhat, did I get that
> > right?
>
> Hmm, I think the current behavior makes more sense. If zswap is
> completely
> disabled, it seems intuitive that zswap-related knobs lose their
> effect.

Mike is right here. It's less of a behavioral decision, but more of a
this-can-confuse-users kind of thing :) At least that's my rationale
when I wrote this.

If users want to disable swap still, they can still do that with
memory.swap.max = 0, which is probably better because it would fail
earlier at the swap slot allocation step.

>
> > I feel like it's a little bit odd to be honest, but I don't have a
> > strong opinion on it. Maybe we should document this behavior better.
>
> But clarify this in the documentation certainly sounds good :)

But yes, better documentation == happy Nhat :)