Re: [PATCH v2 1/7] selftests/cgroup: skip test_zswap if zswap is globally disabled

[Li Wang <liwang@redhat.com>]

On Thu, Mar 12, 2026 at 02:09:30PM -0700, Yosry Ahmed wrote:
> > > diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
> > > index 64ebc3f3f203..ec64daaa2f5a 100644
> > > --- a/tools/testing/selftests/cgroup/test_zswap.c
> > > +++ b/tools/testing/selftests/cgroup/test_zswap.c
> > > @@ -589,23 +589,41 @@ struct zswap_test {
> > >  };
> > >  #undef T
> > >
> > > -static bool zswap_configured(void)
> > > +static int zswap_enabled(void)
> > >  {
> > > -       return access("/sys/module/zswap", F_OK) == 0;
> > > +       char buf[16];
> > > +       ssize_t n;
> > > +
> > > +       if (access("/sys/module/zswap", F_OK))
> > > +               ksft_exit_skip("zswap isn't configured\n");
> > > +
> > > +       n = read_text("/sys/module/zswap/parameters/enabled", buf, sizeof(buf));
> > > +       if (n <= 0)
> > > +               return -1;
> > > +
> > > +       if (buf[0] == 'Y')
> > > +               return 1;
> > > +       else if (buf[0] == 'N')
> > > +               return 0;
> > > +
> > > +       return -1;
> > >  }
> > >
> > >  int main(int argc, char **argv)
> > >  {
> > >         char root[PATH_MAX];
> > > -       int i;
> > > +       int i, state;
> > >
> > >         ksft_print_header();
> > >         ksft_set_plan(ARRAY_SIZE(tests));
> > >         if (cg_find_unified_root(root, sizeof(root), NULL))
> > >                 ksft_exit_skip("cgroup v2 isn't mounted\n");
> > >
> > > -       if (!zswap_configured())
> > > -               ksft_exit_skip("zswap isn't configured\n");
> > > +       state = zswap_enabled();
> > > +       if (state == 0)
> > > +               ksft_exit_skip("zswap is disabled (hint: echo 1 > /sys/module/zswap/parameters/enabled)\n");
> > > +       else if (state < 0)
> > > +               ksft_exit_fail_msg("Failed to read zswap state\n");
> > >
> > >         /*
> > >          * Check that memory controller is available:
> > > --
> > > 2.53.0
> > >
> >
> > Seems a bit convoluted. You ksft_exit_skip() for one case in
> > zswap_enabled(), but return some value in other cases. This value is
> > checked in the caller (main()) and then used to decide whether to skip
> > or fail as well?
> >
> > Why don't you just consolidate them in one place. If zswap_enabled()
> > has no other callers, let's just open code it, yeah?
> 
> Hmm I actually like having the helper. What if we move all the
> ksft_exit_skip() and ksft_exit_fail_msg() calls inside and rename it
> to check_zswap_enabled()? We can also just read one character:

Yes, this looks more tidy than open it. I'll go this helper in v3.

> 
> static void check_zswap_enabled(void)
> {
>        char value;

read_text() requires string but not char.

> 
>        if (access("/sys/module/zswap", F_OK))
>                ksft_exit_skip("zswap isn't configured\n");
> 
>        if (read_text("/sys/module/zswap/parameters/enabled", value,
> sizeof(value)) <= 0)
>                ksft_exit_fail_msg("Failed to read
> /sys/module/zswap/parameters/enabled\n");
> 
>        if (value == 'N')
>                ksft_exit_skip("zswap is disabled (hint: echo 1 >
> /sys/module/zswap/parameters/enabled)\n");
> }
> 

-- 
Regards,
Li Wang