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@xxxxxxxxxxx>
> Suggested-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
> Signed-off-by: Nhat Pham <nphamcs@xxxxxxxxx>
> ---
>  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