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

[Yosry Ahmed <yosryahmed@google.com>]

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