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

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