Re: [PATCH 1/5] selftests/cgroup: detect and handle global zswap state in test_zswap

[Li Wang <liwang@redhat.com>]

On Wed, Mar 11, 2026 at 11:47:22AM -0700, Yosry Ahmed wrote:
...
> > --- a/tools/testing/selftests/cgroup/test_zswap.c
> > +++ b/tools/testing/selftests/cgroup/test_zswap.c
> > @@ -594,18 +594,88 @@ static bool zswap_configured(void)
> >         return access("/sys/module/zswap", F_OK) == 0;
> >  }
> >
> > +static int zswap_enabled_state(void)
> 
> Just zswap_enabled() is good.

+1

> 
> > +{
> > +       char buf[16];
> > +       ssize_t n;
> > +
> > +       if (!zswap_configured())
> > +               return -1;
> 
> Return 0 here, zswap is disabled for all intents and purposes.
> 
> > +
> > +       n = read_text("/sys/module/zswap/parameters/enabled", buf, sizeof(buf));
> > +       if (n < 0 || n == 0)
> > +               return -1;
> 
> if (read_text(..) <= 0)
> 
> > +
> > +       switch (buf[0]) {
> > +       case 'Y':
> > +       case 'y':
> > +       case '1':
> > +               return 1;
> > +       case 'N':
> > +       case 'n':
> > +       case '0':
> 
> Can a read really return any of these values? Or just Y/N?

On my side it just returns 'Y/N', but I use more chars only for
safe/compatiable consideration. Maybe we don't need others.

> >  int main(int argc, char **argv)
> >  {
> >         char root[PATH_MAX];
> > -       int i;
> > +       int i, orig_zswap_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())
> > +       orig_zswap_state = zswap_enabled_state();
> > +
> > +       if (orig_zswap_state == -1)
> >                 ksft_exit_skip("zswap isn't configured\n");
> > +       else if (orig_zswap_state == 0 && !enable_zswap())
> > +               ksft_exit_skip("zswap is disabled and cannot be enabled\n");
> 
> As Michal mentioned, skip the test if zswap is not enabled.

Sure, that would be simpler and we can remove the enable/disable_zswap() functions.

> Assuming zswap_enabled() only returns -1 if it fails to read the
> module param (and zswap_configured() is true), then we should probably
> fail instead of skip, as it means something is wrong with the test or
> the module param.

Absoultely right. Thanks for reviewing.

-- 
Regards,
Li Wang