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/d9cfaf405b8ffe2c716b1ce4c82e0a19d50...
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