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
