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

[Nhat Pham <nphamcs@gmail.com>]

On Wed, Mar 11, 2026 at 9:06 PM Li Wang <liwang@redhat.com> wrote:
>
> Currently, test_zswap only checks for the presence of zswap via
> /sys/module/zswap, but it does not account for the global runtime
> state in /sys/module/zswap/parameters/enabled.
>
> If zswap is configured in the kernel but globally disabled at runtime,
> the zswap cgroup tests will run in an invalid environment and fail
> spuriously.
>
> Fix this by reading the 'enabled' parameter and gracefully skipping
> the test when zswap is disabled. A helpful hint is also added to the
> skip message to show users how to enable it.
>
> Signed-off-by: Li Wang <liwang@redhat.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Michal Koutný <mkoutny@suse.com>
> Cc: Muchun Song <muchun.song@linux.dev>
> Cc: Nhat Pham <nphamcs@gmail.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Roman Gushchin <roman.gushchin@linux.dev>
> Cc: Shakeel Butt <shakeel.butt@linux.dev>
> Cc: Yosry Ahmed <yosryahmed@google.com>
> ---
>
> Notes:
>     v1 --> v2:
>         * remove enable/disable_zswap functions
>         * skip the test if zswap is not enabled
>         * reporting fail when zswap_enabled return -1
>
>  tools/testing/selftests/cgroup/test_zswap.c | 28 +++++++++++++++++----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> 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?