On Fri, Aug 16, 2024 at 7:44 AM Mike Yuan <me@xxxxxxxxxxx> wrote:
>
> Ensure that zswap.writeback check goes up the cgroup tree.
Too concise :) Perhaps a little bit of description of what you are
doing would be helpful.
>
> Signed-off-by: Mike Yuan <me@xxxxxxxxxxx>
> ---
> tools/testing/selftests/cgroup/test_zswap.c | 69 ++++++++++++++-------
> 1 file changed, 48 insertions(+), 21 deletions(-)
>
> diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
> index 190096017f80..7da6f9dc1066 100644
> --- a/tools/testing/selftests/cgroup/test_zswap.c
> +++ b/tools/testing/selftests/cgroup/test_zswap.c
> @@ -263,15 +263,13 @@ static int test_zswapin(const char *root)
> static int attempt_writeback(const char *cgroup, void *arg)
> {
> long pagesize = sysconf(_SC_PAGESIZE);
> - char *test_group = arg;
> size_t memsize = MB(4);
> char buf[pagesize];
> long zswap_usage;
> - bool wb_enabled;
> + bool wb_enabled = *(bool *) arg;
> int ret = -1;
> char *mem;
>
> - wb_enabled = cg_read_long(test_group, "memory.zswap.writeback");
> mem = (char *)malloc(memsize);
> if (!mem)
> return ret;
> @@ -288,12 +286,12 @@ static int attempt_writeback(const char *cgroup, void *arg)
> memcpy(&mem[i], buf, pagesize);
>
> /* Try and reclaim allocated memory */
> - if (cg_write_numeric(test_group, "memory.reclaim", memsize)) {
> + if (cg_write_numeric(cgroup, "memory.reclaim", memsize)) {
> ksft_print_msg("Failed to reclaim all of the requested memory\n");
> goto out;
> }
>
> - zswap_usage = cg_read_long(test_group, "memory.zswap.current");
> + zswap_usage = cg_read_long(cgroup, "memory.zswap.current");
>
> /* zswpin */
> for (int i = 0; i < memsize; i += pagesize) {
> @@ -303,7 +301,7 @@ static int attempt_writeback(const char *cgroup, void *arg)
> }
> }
>
> - if (cg_write_numeric(test_group, "memory.zswap.max", zswap_usage/2))
> + if (cg_write_numeric(cgroup, "memory.zswap.max", zswap_usage/2))
> goto out;
>
> /*
> @@ -312,7 +310,7 @@ static int attempt_writeback(const char *cgroup, void *arg)
> * If writeback is disabled, memory reclaim will fail as zswap is limited and
> * it can't writeback to swap.
> */
> - ret = cg_write_numeric(test_group, "memory.reclaim", memsize);
> + ret = cg_write_numeric(cgroup, "memory.reclaim", memsize);
> if (!wb_enabled)
> ret = (ret == -EAGAIN) ? 0 : -1;
>
> @@ -321,12 +319,38 @@ static int attempt_writeback(const char *cgroup, void *arg)
> return ret;
> }
>
> +static int test_zswap_writeback_one(const char *cgroup, bool wb)
> +{
> + long zswpwb_before, zswpwb_after;
> +
> + zswpwb_before = get_cg_wb_count(cgroup);
> + if (zswpwb_before != 0) {
> + ksft_print_msg("zswpwb_before = %ld instead of 0\n", zswpwb_before);
> + return -1;
> + }
> +
> + if (cg_run(cgroup, attempt_writeback, (void *) &wb))
> + return -1;
> +
> + /* Verify that zswap writeback occurred only if writeback was enabled */
> + zswpwb_after = get_cg_wb_count(cgroup);
> + if (zswpwb_after < 0)
> + return -1;
> +
> + if (wb != !!zswpwb_after) {
> + ksft_print_msg("zswpwb_after is %ld while wb is %s",
> + zswpwb_after, wb ? "enabled" : "disabled");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> /* Test to verify the zswap writeback path */
> static int test_zswap_writeback(const char *root, bool wb)
> {
> - long zswpwb_before, zswpwb_after;
> int ret = KSFT_FAIL;
> - char *test_group;
> + char *test_group, *test_group_child = NULL;
>
> test_group = cg_name(root, "zswap_writeback_test");
> if (!test_group)
> @@ -336,29 +360,32 @@ static int test_zswap_writeback(const char *root, bool wb)
> if (cg_write(test_group, "memory.zswap.writeback", wb ? "1" : "0"))
> goto out;
>
> - zswpwb_before = get_cg_wb_count(test_group);
> - if (zswpwb_before != 0) {
> - ksft_print_msg("zswpwb_before = %ld instead of 0\n", zswpwb_before);
> + if (test_zswap_writeback_one(test_group, wb))
> goto out;
> - }
>
> - if (cg_run(test_group, attempt_writeback, (void *) test_group))
> + if (cg_write(test_group, "memory.zswap.max", "max"))
> + goto out;
Why is this needed? Isn't this the default value?
> + if (cg_write(test_group, "cgroup.subtree_control", "+memory"))
> goto out;
>
> - /* Verify that zswap writeback occurred only if writeback was enabled */
> - zswpwb_after = get_cg_wb_count(test_group);
> - if (zswpwb_after < 0)
> + test_group_child = cg_name(test_group, "zswap_writeback_test_child");
> + if (!test_group_child)
> + goto out;
> + if (cg_create(test_group_child))
> + goto out;
I'd rather have all the hierarchy setup at the beginning of the test,
before the actual test logic. I don't feel strongly about it though.
> + if (cg_write(test_group_child, "memory.zswap.writeback", "1"))
> goto out;
Is the idea here that we always hardcode the child's zswap.writeback
to 1, and the parent's zswap.writeback changes from 0 to 1, and we
check that the parent's value is what matters?
I think we need a comment here.
TBH, I expected a separate test that checks different combinations of
parent and child values (e.g. also verifies that if the parent is
enabled but child is disabled, writeback is disabled).
>
> - if (wb != !!zswpwb_after) {
> - ksft_print_msg("zswpwb_after is %ld while wb is %s",
> - zswpwb_after, wb ? "enabled" : "disabled");
> + if (test_zswap_writeback_one(test_group_child, wb))
> goto out;
> - }
>
> ret = KSFT_PASS;
>
> out:
> + if (test_group_child) {
> + cg_destroy(test_group_child);
> + free(test_group_child);
> + }
> cg_destroy(test_group);
> free(test_group);
> return ret;
> --
> 2.46.0
>
>
