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
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
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.24...
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)
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), };
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
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
+{
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.
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!
linux-kselftest-mirror@lists.linaro.org
-
Nhat Pham -
Yosry Ahmed