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

[Yosry Ahmed <yosryahmed@google.com>]

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.