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

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

[Nhat Pham]

Changelog:
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 | 99 ++++++++++++++++++++-
 2 files changed, 99 insertions(+), 1 deletion(-)


base-commit: 3a92c45e4ba694381c46994f3fde0d8544a2088b
-- 
2.39.3

[PATCH v2 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 fecebfc4c0dc..5f60faaefaf2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24396,6 +24396,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 v2 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 v2 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 | 97 +++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
index 32ce975b21d1..14d1f18f1098 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_bytes_and_read(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;
@@ -133,6 +154,80 @@ 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 zswpout;
+
+	/* Set up */
+	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;
+	/* Disable zswap */
+	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_bytes_and_read, (void *)MB(32)))
+		goto out;
+
+	/* Verify that no zswap happened */
+	zswpout = get_zswpout(test_group);
+	if (zswpout < 0) {
+		ksft_print_msg("Failed to get zswpout\n");
+		goto out;
+	} else if (zswpout > 0) {
+		ksft_print_msg(
+			"Pages should not go to zswap 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_no_limit(const char *root)
+{
+	int ret = KSFT_FAIL;
+	char *test_group;
+
+	/* 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_bytes_and_read, (void *)MB(32)))
+		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 +404,8 @@ struct zswap_test {
 	const char *name;
 } tests[] = {
 	T(test_zswap_usage),
+	T(test_swapin_nozswap),
+	T(test_zswapin_no_limit),
 	T(test_no_kmem_bypass),
 	T(test_no_invasive_cgroup_shrink),
 };
-- 
2.39.3

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

[Yosry Ahmed]

Hey Nhat,

I have a few more comments, sorry for not catching everything the first
time around.

Adding Roman to CC.

On Wed, Jan 31, 2024 at 07:27:18PM -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>
> ---
>  tools/testing/selftests/cgroup/test_zswap.c | 97 +++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
> index 32ce975b21d1..14d1f18f1098 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_bytes_and_read(const char *cgroup, void *arg)

I think allocate_and_read_bytes() is easier to read, but I don't feel
strongly about it.

> +{
> +	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';

cgroup_util.h defines PAGE_SIZE, see alloc_anon() for example.

On that note, alloc_anon() is awfully close to allocate_bytes() below,
perhaps we should consolidate them. The only difference I see is that
alloc_anon() does not check for the allocation failure, but a lot of
functions in cgroup_helpers.c don't, so it seems intentional for
simplification.

> +
> +	/* 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;
> @@ -133,6 +154,80 @@ 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 zswpout;
> +
> +	/* Set up */

I think this comment is unnecessary.

> +	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;
> +	/* Disable zswap */

I think this comment is unnecessary.

> +	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_bytes_and_read, (void *)MB(32)))
> +		goto out;
> +
> +	/* Verify that no zswap happened */

If we want to be really meticulous, we can verify that we did swap out,
but not to zswap. IOW, we can check memory.swap.current or something.

> +	zswpout = get_zswpout(test_group);
> +	if (zswpout < 0) {
> +		ksft_print_msg("Failed to get zswpout\n");
> +		goto out;
> +	} else if (zswpout > 0) {

nit: This can be a separate if condition, I think it would be more
inline with the style of separate consecutive if blocks we are
following.

> +		ksft_print_msg(
> +			"Pages should not go to zswap when memory.zswap.max = 0\n");

We can probably avoid the line break with something more concise, for
example:
"zswapout > 0 when zswap is disabled"
or "zswapout > 0 when memory.zswap.max = 0"

> +		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_no_limit(const char *root)

I think test_zswapin() is enough to be distinct from
test_swapin_nozswap(). The limit is not a factor here AFAICT.

> +{
> +	int ret = KSFT_FAIL;
> +	char *test_group;
> +
> +	/* Set up */

I think this comment is unnecessary.

> +	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_bytes_and_read, (void *)MB(32)))
> +		goto out;

We should probably check for a positive zswapin here, no?

> +
> +	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 +404,8 @@ struct zswap_test {
>  	const char *name;
>  } tests[] = {
>  	T(test_zswap_usage),
> +	T(test_swapin_nozswap),
> +	T(test_zswapin_no_limit),
>  	T(test_no_kmem_bypass),
>  	T(test_no_invasive_cgroup_shrink),
>  };
> -- 
> 2.39.3

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

[Nhat Pham]

On Thu, Feb 1, 2024 at 1:08 AM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> Hey Nhat,
>
> I have a few more comments, sorry for not catching everything the first
> time around.

No worries :)

>
> Adding Roman to CC.
>
> On Wed, Jan 31, 2024 at 07:27:18PM -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>
> > ---
> >  tools/testing/selftests/cgroup/test_zswap.c | 97 +++++++++++++++++++++
> >  1 file changed, 97 insertions(+)
> >
> > diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
> > index 32ce975b21d1..14d1f18f1098 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_bytes_and_read(const char *cgroup, void *arg)
>
> I think allocate_and_read_bytes() is easier to read, but I don't feel
> strongly about it.

Ah you're right. I'll fix that in the next version.

>
> > +{
> > +     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';
>
> cgroup_util.h defines PAGE_SIZE, see alloc_anon() for example.
>
> On that note, alloc_anon() is awfully close to allocate_bytes() below,
> perhaps we should consolidate them. The only difference I see is that
> alloc_anon() does not check for the allocation failure, but a lot of
> functions in cgroup_helpers.c don't, so it seems intentional for
> simplification.

Hmm I didn't know about this function. I think it was Domenico who
added allocate_bytes() for the initial zswap tests, and I've just been
piggybacking on it ever since:
https://github.com/torvalds/linux/commit/d9cfaf405b8ffe2c716b1ce4c82e0a19d50951da

I can send a separate patch to clean this up later :) Doesn't seem that bad.

>
> > +
> > +     /* 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;
> > @@ -133,6 +154,80 @@ 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 zswpout;
> > +
> > +     /* Set up */
>
> I think this comment is unnecessary.

Fair!

>
> > +     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;
> > +     /* Disable zswap */
>
> I think this comment is unnecessary.

Fair!

>
> > +     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_bytes_and_read, (void *)MB(32)))
> > +             goto out;
> > +
> > +     /* Verify that no zswap happened */
>
> If we want to be really meticulous, we can verify that we did swap out,
> but not to zswap. IOW, we can check memory.swap.current or something.

Hmm would memory.swap.current go back to 0 once the memory-in-swap is
freed? It doesn't seem like we have any counters at the cgroup level
for swapout/swapin events. Maybe such counters were not useful enough
to justify the extra overhead of maintaining them? :)

Anyway, I think checking zswpout should probably be enough here.
That's the spirit of the test anyway - make absolutely sure that no
zswap-out happened.

>
> > +     zswpout = get_zswpout(test_group);
> > +     if (zswpout < 0) {
> > +             ksft_print_msg("Failed to get zswpout\n");
> > +             goto out;
> > +     } else if (zswpout > 0) {
>
> nit: This can be a separate if condition, I think it would be more
> inline with the style of separate consecutive if blocks we are
> following.

Yeah now that you point out the inconsistency, it starts to bug my
brain too :) I'll fix it in the next version.

>
> > +             ksft_print_msg(
> > +                     "Pages should not go to zswap when memory.zswap.max = 0\n");
>
> We can probably avoid the line break with something more concise, for
> example:
> "zswapout > 0 when zswap is disabled"
> or "zswapout > 0 when memory.zswap.max = 0"
>
> > +             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_no_limit(const char *root)
>
> I think test_zswapin() is enough to be distinct from
> test_swapin_nozswap(). The limit is not a factor here AFAICT.

Fair.

>
> > +{
> > +     int ret = KSFT_FAIL;
> > +     char *test_group;
> > +
> > +     /* Set up */
>
> I think this comment is unnecessary.

Fair.

>
> > +     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_bytes_and_read, (void *)MB(32)))
> > +             goto out;
>
> We should probably check for a positive zswapin here, no?

Oh right. I'll just do a quick check here:

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 +404,8 @@ struct zswap_test {
> >       const char *name;
> >  } tests[] = {
> >       T(test_zswap_usage),
> > +     T(test_swapin_nozswap),
> > +     T(test_zswapin_no_limit),
> >       T(test_no_kmem_bypass),
> >       T(test_no_invasive_cgroup_shrink),
> >  };
> > --
> > 2.39.3

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

[Yosry Ahmed]

> > > +{
> > > +     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';
> >
> > cgroup_util.h defines PAGE_SIZE, see alloc_anon() for example.
> >
> > On that note, alloc_anon() is awfully close to allocate_bytes() below,
> > perhaps we should consolidate them. The only difference I see is that
> > alloc_anon() does not check for the allocation failure, but a lot of
> > functions in cgroup_helpers.c don't, so it seems intentional for
> > simplification.
>
> Hmm I didn't know about this function. I think it was Domenico who
> added allocate_bytes() for the initial zswap tests, and I've just been
> piggybacking on it ever since:
> https://github.com/torvalds/linux/commit/d9cfaf405b8ffe2c716b1ce4c82e0a19d50951da
>
> I can send a separate patch to clean this up later :) Doesn't seem that bad.

SGTM.

[..]
> >
> > > +     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_bytes_and_read, (void *)MB(32)))
> > > +             goto out;
> > > +
> > > +     /* Verify that no zswap happened */
> >
> > If we want to be really meticulous, we can verify that we did swap out,
> > but not to zswap. IOW, we can check memory.swap.current or something.
>
> Hmm would memory.swap.current go back to 0 once the memory-in-swap is
> freed? It doesn't seem like we have any counters at the cgroup level
> for swapout/swapin events. Maybe such counters were not useful enough
> to justify the extra overhead of maintaining them? :)
>
> Anyway, I think checking zswpout should probably be enough here.
> That's the spirit of the test anyway - make absolutely sure that no
> zswap-out happened.

The test is making sure that even though we used real swap, we did not
use zswap. In other words, we may see a false positive if something
goes wrong and we don't swap anything at all. I know I am probably
being paranoid here :)

How about we check memory.swap.peak?

[..]
> > > +     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_bytes_and_read, (void *)MB(32)))
> > > +             goto out;
> >
> > We should probably check for a positive zswapin here, no?
>
> Oh right. I'll just do a quick check here:
>
> 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;
> }

SGTM.

Thanks!