Re: [PATCH] selftests: cgroup: add tests to verify the zswap writeback path

From: Usama Arif
Date: Fri May 03 2024 - 11:11:22 EST



On 03/05/2024 00:30, Nhat Pham wrote:
On Thu, May 2, 2024 at 12:05 PM Usama Arif <usamaarif642@xxxxxxxxx> wrote:

On 01/05/2024 16:44, Nhat Pham wrote:
On Wed, May 1, 2024 at 3:04 AM Usama Arif <usamaarif642@xxxxxxxxx> wrote:
The condition for writeback can be triggered by allocating random
memory more than memory.high to push memory into zswap, more than
zswap.max to trigger writeback if enabled, but less than memory.max
so that OOM is not triggered. Both values of memory.zswap.writeback
are tested.
Thanks for adding the test, Usama :) A couple of suggestions below.

Signed-off-by: Usama Arif <usamaarif642@xxxxxxxxx>
---
tools/testing/selftests/cgroup/test_zswap.c | 83 +++++++++++++++++++++
1 file changed, 83 insertions(+)

diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
index f0e488ed90d8..fe0e7221525c 100644
--- a/tools/testing/selftests/cgroup/test_zswap.c
+++ b/tools/testing/selftests/cgroup/test_zswap.c
@@ -94,6 +94,19 @@ static int allocate_bytes(const char *cgroup, void *arg)
return 0;
}

+static int allocate_random_bytes(const char *cgroup, void *arg)
+{
+ size_t size = (size_t)arg;
+ char *mem = (char *)malloc(size);
+
+ if (!mem)
+ return -1;
+ for (int i = 0; i < size; i++)
+ mem[i] = rand() % 128;
+ free(mem);
+ return 0;
+}
+
static char *setup_test_group_1M(const char *root, const char *name)
{
char *group_name = cg_name(root, name);
@@ -248,6 +261,74 @@ static int test_zswapin(const char *root)
return ret;
}

+/* Test to verify the zswap writeback path */
+static int test_zswap_writeback(const char *root, bool wb)
+{
+ int ret = KSFT_FAIL;
+ char *test_group;
+ long zswpwb_before, zswpwb_after;
+
+ test_group = cg_name(root,
+ wb ? "zswap_writeback_enabled_test" : "zswap_writeback_disabled_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.high", "2M"))
+ goto out;
+ if (cg_write(test_group, "memory.zswap.max", "2M"))
+ goto out;
+ if (cg_write(test_group, "memory.zswap.writeback", wb ? "1" : "0"))
+ goto out;
+
+ zswpwb_before = cg_read_key_long(test_group, "memory.stat", "zswpwb ");
+ if (zswpwb_before < 0) {
+ ksft_print_msg("failed to get zswpwb_before\n");
+ goto out;
+ }
+
+ /*
+ * Allocate more than memory.high to push memory into zswap,
+ * more than zswap.max to trigger writeback if enabled,
+ * but less than memory.max so that OOM is not triggered
+ */
+ if (cg_run(test_group, allocate_random_bytes, (void *)MB(3)))
+ goto out;
I think we should document better why we allocate random bytes (rather
than just using the existing allocation helper).

This random allocation pattern (rand() % 128) is probably still
compressible by zswap, albeit poorly. I assume this is so that zswap
would not be able to just absorb all the swapped out pages?
Thanks for the review! I have added doc in v2 explaining why random
memory is used.


+
+ /* Verify that zswap writeback occurred only if writeback was enabled */
+ zswpwb_after = cg_read_key_long(test_group, "memory.stat", "zswpwb ");
+ if (wb) {
+ if (zswpwb_after <= zswpwb_before) {
+ ksft_print_msg("writeback enabled and zswpwb_after <= zswpwb_before\n");
+ goto out;
+ }
+ } else {
+ if (zswpwb_after != zswpwb_before) {
+ ksft_print_msg("writeback disabled and zswpwb_after != zswpwb_before\n");
+ goto out;
+ }
It'd be nice if we can check that in this case, the number of pages
that are "swapped out" matches the cgroup's zswpout stats :)
I think with the method in v2, this might not be easily tracked as some
metrics are all time (zswpout) while others are current.
Hmm would pgsteal be a good candidate for this purpose?
Just throwing out ideas - I'll leave this up to you to decide :)

So I think what the equation would be is pgsteal = zswpout + number of pages that couldnt be compressed and directly moved to swap? We have pgsteal and zswpout, but I cant see from existing metrics if we can get the 3rd thing.

As a reference these are some of the metrics at the end of writeback enabled test from v2:

zswpin 1024
zswpout 1801
zswpwb 297

pgsteal 2057

zswap 0
zswapped 0

We need a metric that is 2057-1801 = 256, but can't find any :)