[PATCH v3 0/3] fix and extend zswap kselftests

[PATCH v3 0/3] fix and extend zswap kselftests

[Nhat Pham]

Changelog:
v3:
	* More cleanup (patch 3) (suggested by Yosry Ahmed).
	* Check swap peak in swapin test
v2:
	* Make the swapin test also checks for zswap usage (patch 3)
	  (suggested by Yosry Ahmed)
	* Some test simplifications/cleanups (patch 3)
	  (suggested by Yosry Ahmed).

Fix a broken zswap kselftest due to cgroup zswap writeback counter
renaming, and add 2 zswap kselftests, one to cover the (z)swapin case,
and another to check that no zswapping happens when the cgroup limit is
0.

Also, add the zswap kselftest file to zswap maintainer entry so that
get_maintainers script can find zswap maintainers.

Nhat Pham (3):
  selftests: zswap: add zswap selftest file to zswap maintainer entry
  selftests: fix the zswap invasive shrink test
  selftests: add zswapin and no zswap tests

 MAINTAINERS                                 |   1 +
 tools/testing/selftests/cgroup/test_zswap.c | 122 +++++++++++++++++++-
 2 files changed, 121 insertions(+), 2 deletions(-)


base-commit: 91f3daa1765ee4e0c89987dc25f72c40f07af34d
-- 
2.39.3

[PATCH v3 1/3] selftests: zswap: add zswap selftest file to zswap maintainer entry

[Nhat Pham]

Make it easier for contributors to find the zswap maintainers when they
update the zswap tests.

Signed-off-by: Nhat Pham <nphamcs@gmail.com>
Acked-by: Yosry Ahmed <yosryahmed@google.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6527850f24d1..126090f0e418 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24398,6 +24398,7 @@ F:	include/linux/zpool.h
 F:	include/linux/zswap.h
 F:	mm/zpool.c
 F:	mm/zswap.c
+F:	tools/testing/selftests/cgroup/test_zswap.c
 
 THE REST
 M:	Linus Torvalds <torvalds@linux-foundation.org>
-- 
2.39.3

[PATCH v3 2/3] selftests: fix the zswap invasive shrink test

[Nhat Pham]

The zswap no invasive shrink selftest breaks because we rename the zswap
writeback counter (see [1]). Fix the test.

[1]: https://patchwork.kernel.org/project/linux-kselftest/patch/20231205193307.2432803-1-nphamcs@gmail.com/

Fixes: a697dc2be925 ("selftests: cgroup: update per-memcg zswap writeback selftest")
Signed-off-by: Nhat Pham <nphamcs@gmail.com>
Acked-by: Yosry Ahmed <yosryahmed@google.com>
---
 tools/testing/selftests/cgroup/test_zswap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
index 47fdaa146443..32ce975b21d1 100644
--- a/tools/testing/selftests/cgroup/test_zswap.c
+++ b/tools/testing/selftests/cgroup/test_zswap.c
@@ -52,7 +52,7 @@ static int get_zswap_stored_pages(size_t *value)
 
 static int get_cg_wb_count(const char *cg)
 {
-	return cg_read_key_long(cg, "memory.stat", "zswp_wb");
+	return cg_read_key_long(cg, "memory.stat", "zswpwb");
 }
 
 static long get_zswpout(const char *cgroup)
-- 
2.39.3

[PATCH v3 3/3] selftests: add zswapin and no zswap tests

[Nhat Pham]

Add a selftest to cover the zswapin code path, allocating more memory
than the cgroup limit to trigger swapout/zswapout, then reading the
pages back in memory several times. This is inspired by a recently
encountered kernel crash on the zswapin path in our internal kernel,
which went undetected because of a lack of test coverage for this path.

Add a selftest to verify that when memory.zswap.max = 0, no pages can go
to the zswap pool for the cgroup.

Suggested-by: Rik van Riel <riel@surriel.com>
Suggested-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Nhat Pham <nphamcs@gmail.com>
---
 tools/testing/selftests/cgroup/test_zswap.c | 120 +++++++++++++++++++-
 1 file changed, 119 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
index 32ce975b21d1..c263610a4a60 100644
--- a/tools/testing/selftests/cgroup/test_zswap.c
+++ b/tools/testing/selftests/cgroup/test_zswap.c
@@ -60,6 +60,27 @@ static long get_zswpout(const char *cgroup)
 	return cg_read_key_long(cgroup, "memory.stat", "zswpout ");
 }
 
+static int allocate_and_read_bytes(const char *cgroup, void *arg)
+{
+	size_t size = (size_t)arg;
+	char *mem = (char *)malloc(size);
+	int ret = 0;
+
+	if (!mem)
+		return -1;
+	for (int i = 0; i < size; i += 4095)
+		mem[i] = 'a';
+
+	/* go through the allocated memory to (z)swap in and out pages */
+	for (int i = 0; i < size; i += 4095) {
+		if (mem[i] != 'a')
+			ret = -1;
+	}
+
+	free(mem);
+	return ret;
+}
+
 static int allocate_bytes(const char *cgroup, void *arg)
 {
 	size_t size = (size_t)arg;
@@ -100,7 +121,6 @@ static int test_zswap_usage(const char *root)
 	int ret = KSFT_FAIL;
 	char *test_group;
 
-	/* Set up */
 	test_group = cg_name(root, "no_shrink_test");
 	if (!test_group)
 		goto out;
@@ -133,6 +153,102 @@ static int test_zswap_usage(const char *root)
 	return ret;
 }
 
+/*
+ * Check that when memory.zswap.max = 0, no pages can go to the zswap pool for
+ * the cgroup.
+ */
+static int test_swapin_nozswap(const char *root)
+{
+	int ret = KSFT_FAIL;
+	char *test_group;
+	long swap_peak, zswpout;
+
+	test_group = cg_name(root, "no_zswap_test");
+	if (!test_group)
+		goto out;
+	if (cg_create(test_group))
+		goto out;
+	if (cg_write(test_group, "memory.max", "8M"))
+		goto out;
+	if (cg_write(test_group, "memory.zswap.max", "0"))
+		goto out;
+
+	/* Allocate and read more than memory.max to trigger swapin */
+	if (cg_run(test_group, allocate_and_read_bytes, (void *)MB(32)))
+		goto out;
+
+	/* Verify that pages are swapped out, but no zswap happened */
+	swap_peak = cg_read_long(test_group, "memory.swap.peak");
+	if (swap_peak < 0) {
+		ksft_print_msg("failed to get cgroup's swap_peak\n");
+		goto out;
+	}
+
+	if (swap_peak == 0) {
+		ksft_print_msg("pages should be swapped out\n");
+		goto out;
+	}
+
+	zswpout = get_zswpout(test_group);
+	if (zswpout < 0) {
+		ksft_print_msg("failed to get zswpout\n");
+		goto out;
+	}
+
+	if (zswpout > 0) {
+		ksft_print_msg("zswapout > 0 when memory.zswap.max = 0\n");
+		goto out;
+	}
+
+	ret = KSFT_PASS;
+
+out:
+	cg_destroy(test_group);
+	free(test_group);
+	return ret;
+}
+
+/* Simple test to verify the (z)swapin code paths */
+static int test_zswapin(const char *root)
+{
+	int ret = KSFT_FAIL;
+	char *test_group;
+	long zswpin;
+
+	/* Set up */
+	test_group = cg_name(root, "zswapin_test");
+	if (!test_group)
+		goto out;
+	if (cg_create(test_group))
+		goto out;
+	if (cg_write(test_group, "memory.max", "8M"))
+		goto out;
+	if (cg_write(test_group, "memory.zswap.max", "max"))
+		goto out;
+
+	/* Allocate and read more than memory.max to trigger (z)swap in */
+	if (cg_run(test_group, allocate_and_read_bytes, (void *)MB(32)))
+		goto out;
+
+	zswpin = cg_read_key_long(test_group, "memory.stat", "zswpin ");
+	if (zswpin < 0) {
+		ksft_print_msg("failed to get zswpin\n");
+		goto out;
+	}
+
+	if (zswpin == 0) {
+		ksft_print_msg("zswpin should not be 0\n");
+		goto out;
+	}
+
+	ret = KSFT_PASS;
+
+out:
+	cg_destroy(test_group);
+	free(test_group);
+	return ret;
+}
+
 /*
  * When trying to store a memcg page in zswap, if the memcg hits its memory
  * limit in zswap, writeback should affect only the zswapped pages of that
@@ -309,6 +425,8 @@ struct zswap_test {
 	const char *name;
 } tests[] = {
 	T(test_zswap_usage),
+	T(test_swapin_nozswap),
+	T(test_zswapin),
 	T(test_no_kmem_bypass),
 	T(test_no_invasive_cgroup_shrink),
 };
-- 
2.39.3

Re: [PATCH v3 3/3] selftests: add zswapin and no zswap tests

[Yosry Ahmed]

On Mon, Feb 05, 2024 at 02:56:08PM -0800, Nhat Pham wrote:
> Add a selftest to cover the zswapin code path, allocating more memory
> than the cgroup limit to trigger swapout/zswapout, then reading the
> pages back in memory several times. This is inspired by a recently
> encountered kernel crash on the zswapin path in our internal kernel,
> which went undetected because of a lack of test coverage for this path.
> 
> Add a selftest to verify that when memory.zswap.max = 0, no pages can go
> to the zswap pool for the cgroup.
> 
> Suggested-by: Rik van Riel <riel@surriel.com>
> Suggested-by: Yosry Ahmed <yosryahmed@google.com>
> Signed-off-by: Nhat Pham <nphamcs@gmail.com>

LGTM with a few nits below:
Acked-by: Yosry Ahmed <yosryahmed@google.com>

Thanks!

> ---
>  tools/testing/selftests/cgroup/test_zswap.c | 120 +++++++++++++++++++-
>  1 file changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
> index 32ce975b21d1..c263610a4a60 100644
> --- a/tools/testing/selftests/cgroup/test_zswap.c
> +++ b/tools/testing/selftests/cgroup/test_zswap.c
> @@ -60,6 +60,27 @@ static long get_zswpout(const char *cgroup)
>  	return cg_read_key_long(cgroup, "memory.stat", "zswpout ");
>  }
>  
> +static int allocate_and_read_bytes(const char *cgroup, void *arg)
> +{
> +	size_t size = (size_t)arg;
> +	char *mem = (char *)malloc(size);
> +	int ret = 0;
> +
> +	if (!mem)
> +		return -1;
> +	for (int i = 0; i < size; i += 4095)
> +		mem[i] = 'a';
> +
> +	/* go through the allocated memory to (z)swap in and out pages */

nit: s/go/Go

> +	for (int i = 0; i < size; i += 4095) {
> +		if (mem[i] != 'a')
> +			ret = -1;
> +	}
> +
> +	free(mem);
> +	return ret;
> +}
> +
>  static int allocate_bytes(const char *cgroup, void *arg)
>  {
>  	size_t size = (size_t)arg;
> @@ -100,7 +121,6 @@ static int test_zswap_usage(const char *root)
>  	int ret = KSFT_FAIL;
>  	char *test_group;
>  
> -	/* Set up */

We removed this comment here.

>  	test_group = cg_name(root, "no_shrink_test");
>  	if (!test_group)
>  		goto out;
> @@ -133,6 +153,102 @@ static int test_zswap_usage(const char *root)
>  	return ret;
>  }
>  
> +/*
> + * Check that when memory.zswap.max = 0, no pages can go to the zswap pool for
> + * the cgroup.
> + */
> +static int test_swapin_nozswap(const char *root)
> +{
> +	int ret = KSFT_FAIL;
> +	char *test_group;
> +	long swap_peak, zswpout;
> +
> +	test_group = cg_name(root, "no_zswap_test");
> +	if (!test_group)
> +		goto out;
> +	if (cg_create(test_group))
> +		goto out;
> +	if (cg_write(test_group, "memory.max", "8M"))
> +		goto out;
> +	if (cg_write(test_group, "memory.zswap.max", "0"))
> +		goto out;
> +
> +	/* Allocate and read more than memory.max to trigger swapin */
> +	if (cg_run(test_group, allocate_and_read_bytes, (void *)MB(32)))
> +		goto out;
> +
> +	/* Verify that pages are swapped out, but no zswap happened */
> +	swap_peak = cg_read_long(test_group, "memory.swap.peak");
> +	if (swap_peak < 0) {
> +		ksft_print_msg("failed to get cgroup's swap_peak\n");
> +		goto out;
> +	}
> +
> +	if (swap_peak == 0) {
> +		ksft_print_msg("pages should be swapped out\n");
> +		goto out;
> +	}

We can actually check that this number is >= 24M instead. Not a big
deal, but might as well.

> +
> +	zswpout = get_zswpout(test_group);
> +	if (zswpout < 0) {
> +		ksft_print_msg("failed to get zswpout\n");
> +		goto out;
> +	}
> +
> +	if (zswpout > 0) {
> +		ksft_print_msg("zswapout > 0 when memory.zswap.max = 0\n");
> +		goto out;
> +	}
> +
> +	ret = KSFT_PASS;
> +
> +out:
> +	cg_destroy(test_group);
> +	free(test_group);
> +	return ret;
> +}
> +
> +/* Simple test to verify the (z)swapin code paths */
> +static int test_zswapin(const char *root)
> +{
> +	int ret = KSFT_FAIL;
> +	char *test_group;
> +	long zswpin;
> +
> +	/* Set up */

Yet we added a similar one here :)

> +	test_group = cg_name(root, "zswapin_test");
> +	if (!test_group)
> +		goto out;
> +	if (cg_create(test_group))
> +		goto out;
> +	if (cg_write(test_group, "memory.max", "8M"))
> +		goto out;
> +	if (cg_write(test_group, "memory.zswap.max", "max"))
> +		goto out;
> +
> +	/* Allocate and read more than memory.max to trigger (z)swap in */
> +	if (cg_run(test_group, allocate_and_read_bytes, (void *)MB(32)))
> +		goto out;
> +
> +	zswpin = cg_read_key_long(test_group, "memory.stat", "zswpin ");
> +	if (zswpin < 0) {
> +		ksft_print_msg("failed to get zswpin\n");
> +		goto out;
> +	}
> +
> +	if (zswpin == 0) {
> +		ksft_print_msg("zswpin should not be 0\n");
> +		goto out;
> +	}

Same here, we can check that zswpin is at least 24M worth of events.
Again, not a big deal, but might as well.

Re: [PATCH v3 3/3] selftests: add zswapin and no zswap tests

[Nhat Pham]

On Mon, Feb 5, 2024 at 3:05 PM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Mon, Feb 05, 2024 at 02:56:08PM -0800, Nhat Pham wrote:
> > Add a selftest to cover the zswapin code path, allocating more memory
> > than the cgroup limit to trigger swapout/zswapout, then reading the
> > pages back in memory several times. This is inspired by a recently
> > encountered kernel crash on the zswapin path in our internal kernel,
> > which went undetected because of a lack of test coverage for this path.
> >
> > Add a selftest to verify that when memory.zswap.max = 0, no pages can go
> > to the zswap pool for the cgroup.
> >
> > Suggested-by: Rik van Riel <riel@surriel.com>
> > Suggested-by: Yosry Ahmed <yosryahmed@google.com>
> > Signed-off-by: Nhat Pham <nphamcs@gmail.com>
>
> LGTM with a few nits below:
> Acked-by: Yosry Ahmed <yosryahmed@google.com>
>
> Thanks!
>
> > ---
> >  tools/testing/selftests/cgroup/test_zswap.c | 120 +++++++++++++++++++-
> >  1 file changed, 119 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
> > index 32ce975b21d1..c263610a4a60 100644
> > --- a/tools/testing/selftests/cgroup/test_zswap.c
> > +++ b/tools/testing/selftests/cgroup/test_zswap.c
> > @@ -60,6 +60,27 @@ static long get_zswpout(const char *cgroup)
> >       return cg_read_key_long(cgroup, "memory.stat", "zswpout ");
> >  }
> >
> > +static int allocate_and_read_bytes(const char *cgroup, void *arg)
> > +{
> > +     size_t size = (size_t)arg;
> > +     char *mem = (char *)malloc(size);
> > +     int ret = 0;
> > +
> > +     if (!mem)
> > +             return -1;
> > +     for (int i = 0; i < size; i += 4095)
> > +             mem[i] = 'a';
> > +
> > +     /* go through the allocated memory to (z)swap in and out pages */
>
> nit: s/go/Go
>
> > +     for (int i = 0; i < size; i += 4095) {
> > +             if (mem[i] != 'a')
> > +                     ret = -1;
> > +     }
> > +
> > +     free(mem);
> > +     return ret;
> > +}
> > +
> >  static int allocate_bytes(const char *cgroup, void *arg)
> >  {
> >       size_t size = (size_t)arg;
> > @@ -100,7 +121,6 @@ static int test_zswap_usage(const char *root)
> >       int ret = KSFT_FAIL;
> >       char *test_group;
> >
> > -     /* Set up */
>
> We removed this comment here.
>
> >       test_group = cg_name(root, "no_shrink_test");
> >       if (!test_group)
> >               goto out;
> > @@ -133,6 +153,102 @@ static int test_zswap_usage(const char *root)
> >       return ret;
> >  }
> >
> > +/*
> > + * Check that when memory.zswap.max = 0, no pages can go to the zswap pool for
> > + * the cgroup.
> > + */
> > +static int test_swapin_nozswap(const char *root)
> > +{
> > +     int ret = KSFT_FAIL;
> > +     char *test_group;
> > +     long swap_peak, zswpout;
> > +
> > +     test_group = cg_name(root, "no_zswap_test");
> > +     if (!test_group)
> > +             goto out;
> > +     if (cg_create(test_group))
> > +             goto out;
> > +     if (cg_write(test_group, "memory.max", "8M"))
> > +             goto out;
> > +     if (cg_write(test_group, "memory.zswap.max", "0"))
> > +             goto out;
> > +
> > +     /* Allocate and read more than memory.max to trigger swapin */
> > +     if (cg_run(test_group, allocate_and_read_bytes, (void *)MB(32)))
> > +             goto out;
> > +
> > +     /* Verify that pages are swapped out, but no zswap happened */
> > +     swap_peak = cg_read_long(test_group, "memory.swap.peak");
> > +     if (swap_peak < 0) {
> > +             ksft_print_msg("failed to get cgroup's swap_peak\n");
> > +             goto out;
> > +     }
> > +
> > +     if (swap_peak == 0) {
> > +             ksft_print_msg("pages should be swapped out\n");
> > +             goto out;
> > +     }
>
> We can actually check that this number is >= 24M instead. Not a big
> deal, but might as well.
>
> > +
> > +     zswpout = get_zswpout(test_group);
> > +     if (zswpout < 0) {
> > +             ksft_print_msg("failed to get zswpout\n");
> > +             goto out;
> > +     }
> > +
> > +     if (zswpout > 0) {
> > +             ksft_print_msg("zswapout > 0 when memory.zswap.max = 0\n");
> > +             goto out;
> > +     }
> > +
> > +     ret = KSFT_PASS;
> > +
> > +out:
> > +     cg_destroy(test_group);
> > +     free(test_group);
> > +     return ret;
> > +}
> > +
> > +/* Simple test to verify the (z)swapin code paths */
> > +static int test_zswapin(const char *root)
> > +{
> > +     int ret = KSFT_FAIL;
> > +     char *test_group;
> > +     long zswpin;
> > +
> > +     /* Set up */
>
> Yet we added a similar one here :)

Urgh I am stupid :) I meant to remove this, but accidentally removed
it from the old test instead :)


>
> > +     test_group = cg_name(root, "zswapin_test");
> > +     if (!test_group)
> > +             goto out;
> > +     if (cg_create(test_group))
> > +             goto out;
> > +     if (cg_write(test_group, "memory.max", "8M"))
> > +             goto out;
> > +     if (cg_write(test_group, "memory.zswap.max", "max"))
> > +             goto out;
> > +
> > +     /* Allocate and read more than memory.max to trigger (z)swap in */
> > +     if (cg_run(test_group, allocate_and_read_bytes, (void *)MB(32)))
> > +             goto out;
> > +
> > +     zswpin = cg_read_key_long(test_group, "memory.stat", "zswpin ");
> > +     if (zswpin < 0) {
> > +             ksft_print_msg("failed to get zswpin\n");
> > +             goto out;
> > +     }
> > +
> > +     if (zswpin == 0) {
> > +             ksft_print_msg("zswpin should not be 0\n");
> > +             goto out;
> > +     }
>
> Same here, we can check that zswpin is at least 24M worth of events.
> Again, not a big deal, but might as well.

[PATCH v3 3/3] selftests: add zswapin and no zswap tests (fix)

[Nhat Pham]

Remove redundant "set up" comment and add check to ensure enough data is
swapped out (in swapin test) and zswapped-in.

Suggested-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Nhat Pham <nphamcs@gmail.com>
---
 tools/testing/selftests/cgroup/test_zswap.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
index c263610a4a60..f0e488ed90d8 100644
--- a/tools/testing/selftests/cgroup/test_zswap.c
+++ b/tools/testing/selftests/cgroup/test_zswap.c
@@ -71,7 +71,7 @@ static int allocate_and_read_bytes(const char *cgroup, void *arg)
 	for (int i = 0; i < size; i += 4095)
 		mem[i] = 'a';

-	/* go through the allocated memory to (z)swap in and out pages */
+	/* Go through the allocated memory to (z)swap in and out pages */
 	for (int i = 0; i < size; i += 4095) {
 		if (mem[i] != 'a')
 			ret = -1;
@@ -184,8 +184,8 @@ static int test_swapin_nozswap(const char *root)
 		goto out;
 	}

-	if (swap_peak == 0) {
-		ksft_print_msg("pages should be swapped out\n");
+	if (swap_peak < MB(24)) {
+		ksft_print_msg("at least 24MB of memory should be swapped out\n");
 		goto out;
 	}

@@ -215,7 +215,6 @@ static int test_zswapin(const char *root)
 	char *test_group;
 	long zswpin;

-	/* Set up */
 	test_group = cg_name(root, "zswapin_test");
 	if (!test_group)
 		goto out;
@@ -236,8 +235,8 @@ static int test_zswapin(const char *root)
 		goto out;
 	}

-	if (zswpin == 0) {
-		ksft_print_msg("zswpin should not be 0\n");
+	if (zswpin < MB(24) / PAGE_SIZE) {
+		ksft_print_msg("at least 24MB should be brought back from zswap\n");
 		goto out;
 	}

@@ -260,7 +259,6 @@ static int test_no_invasive_cgroup_shrink(const char *root)
 	size_t control_allocation_size = MB(10);
 	char *control_allocation, *wb_group = NULL, *control_group = NULL;

-	/* Set up */
 	wb_group = setup_test_group_1M(root, "per_memcg_wb_test1");
 	if (!wb_group)
 		return KSFT_FAIL;

base-commit: 9d193b36872d153e02e80c26203de4ee15127b58
--
2.40.1