* [PATCH] selftests: cgroup: add tests to verify the zswap writeback path
@ 2024-05-01 10:04 Usama Arif
2024-05-01 12:29 ` Usama Arif
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Usama Arif @ 2024-05-01 10:04 UTC (permalink / raw)
To: akpm
Cc: hannes, yosryahmed, nphamcs, chengming.zhou, linux-mm,
linux-kernel, kernel-team, Usama Arif
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.
Signed-off-by: Usama Arif <usamaarif642@gmail.com>
---
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;
+
+ /* 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;
+ }
+ }
+
+ ret = KSFT_PASS;
+
+out:
+ cg_destroy(test_group);
+ free(test_group);
+ return ret;
+}
+
+static int test_zswap_writeback_enabled(const char *root)
+{
+ return test_zswap_writeback(root, true);
+}
+
+static int test_zswap_writeback_disabled(const char *root)
+{
+ return test_zswap_writeback(root, false);
+}
+
/*
* 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
@@ -425,6 +506,8 @@ struct zswap_test {
T(test_zswap_usage),
T(test_swapin_nozswap),
T(test_zswapin),
+ T(test_zswap_writeback_enabled),
+ T(test_zswap_writeback_disabled),
T(test_no_kmem_bypass),
T(test_no_invasive_cgroup_shrink),
};
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests: cgroup: add tests to verify the zswap writeback path
2024-05-01 10:04 [PATCH] selftests: cgroup: add tests to verify the zswap writeback path Usama Arif
@ 2024-05-01 12:29 ` Usama Arif
2024-05-01 15:44 ` Nhat Pham
2024-05-01 17:15 ` Yosry Ahmed
2 siblings, 0 replies; 8+ messages in thread
From: Usama Arif @ 2024-05-01 12:29 UTC (permalink / raw)
To: akpm
Cc: hannes, yosryahmed, nphamcs, chengming.zhou, linux-mm,
linux-kernel, kernel-team
On 01/05/2024 11:04, Usama Arif 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.
>
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
Forgot to add:
Suggested-by: Nhat Pham <nphamcs@gmail.com>
> ---
> 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;
> +
> + /* 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;
> + }
> + }
> +
> + ret = KSFT_PASS;
> +
> +out:
> + cg_destroy(test_group);
> + free(test_group);
> + return ret;
> +}
> +
> +static int test_zswap_writeback_enabled(const char *root)
> +{
> + return test_zswap_writeback(root, true);
> +}
> +
> +static int test_zswap_writeback_disabled(const char *root)
> +{
> + return test_zswap_writeback(root, false);
> +}
> +
> /*
> * 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
> @@ -425,6 +506,8 @@ struct zswap_test {
> T(test_zswap_usage),
> T(test_swapin_nozswap),
> T(test_zswapin),
> + T(test_zswap_writeback_enabled),
> + T(test_zswap_writeback_disabled),
> T(test_no_kmem_bypass),
> T(test_no_invasive_cgroup_shrink),
> };
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests: cgroup: add tests to verify the zswap writeback path
2024-05-01 10:04 [PATCH] selftests: cgroup: add tests to verify the zswap writeback path Usama Arif
2024-05-01 12:29 ` Usama Arif
@ 2024-05-01 15:44 ` Nhat Pham
2024-05-02 19:05 ` Usama Arif
2024-05-01 17:15 ` Yosry Ahmed
2 siblings, 1 reply; 8+ messages in thread
From: Nhat Pham @ 2024-05-01 15:44 UTC (permalink / raw)
To: Usama Arif
Cc: akpm, hannes, yosryahmed, chengming.zhou, linux-mm, linux-kernel,
kernel-team
On Wed, May 1, 2024 at 3:04 AM Usama Arif <usamaarif642@gmail.com> 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@gmail.com>
> ---
> 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?
> +
> + /* 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 :)
> + }
> +
> + ret = KSFT_PASS;
> +
> +out:
> + cg_destroy(test_group);
> + free(test_group);
> + return ret;
> +}
> +
> +static int test_zswap_writeback_enabled(const char *root)
> +{
> + return test_zswap_writeback(root, true);
> +}
> +
> +static int test_zswap_writeback_disabled(const char *root)
> +{
> + return test_zswap_writeback(root, false);
> +}
> +
> /*
> * 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
> @@ -425,6 +506,8 @@ struct zswap_test {
> T(test_zswap_usage),
> T(test_swapin_nozswap),
> T(test_zswapin),
> + T(test_zswap_writeback_enabled),
> + T(test_zswap_writeback_disabled),
> T(test_no_kmem_bypass),
> T(test_no_invasive_cgroup_shrink),
> };
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests: cgroup: add tests to verify the zswap writeback path
2024-05-01 10:04 [PATCH] selftests: cgroup: add tests to verify the zswap writeback path Usama Arif
2024-05-01 12:29 ` Usama Arif
2024-05-01 15:44 ` Nhat Pham
@ 2024-05-01 17:15 ` Yosry Ahmed
2024-05-02 19:13 ` Usama Arif
2 siblings, 1 reply; 8+ messages in thread
From: Yosry Ahmed @ 2024-05-01 17:15 UTC (permalink / raw)
To: Usama Arif
Cc: akpm, hannes, nphamcs, chengming.zhou, linux-mm, linux-kernel,
kernel-team
Hi Usama,
On Wed, May 1, 2024 at 3:04 AM Usama Arif <usamaarif642@gmail.com> 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 working on this :)
>
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> ---
> 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;
We set the zswap limit to 2M. So for this to work properly we need to
guarantee that the 3M of random data will compress into more than 2M.
Is this true for all possible zpool implementations and compression
algorithms? How likely for this to break and start producing false
negatives if zswap magically becomes more efficient?
One alternative approach that I used before, although more complex, is
to start by compressing the memory (i.e. through reclaim) without a
zswap limit, and check the zswap usage. Then, fault the memory back
in, set the zswap limit lower than the observed usage, and repeat.
This should guarantee writeback AFAICT.
Also, using memory.reclaim may be easier than memory.high if you
follow this approach, as you would need to raise memory.high again to
be able to decompress the memory.
> +
> + /* 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;
> + }
> + }
> +
> + ret = KSFT_PASS;
> +
> +out:
> + cg_destroy(test_group);
> + free(test_group);
> + return ret;
> +}
> +
> +static int test_zswap_writeback_enabled(const char *root)
> +{
> + return test_zswap_writeback(root, true);
> +}
> +
> +static int test_zswap_writeback_disabled(const char *root)
> +{
> + return test_zswap_writeback(root, false);
> +}
> +
> /*
> * 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
> @@ -425,6 +506,8 @@ struct zswap_test {
> T(test_zswap_usage),
> T(test_swapin_nozswap),
> T(test_zswapin),
> + T(test_zswap_writeback_enabled),
> + T(test_zswap_writeback_disabled),
> T(test_no_kmem_bypass),
> T(test_no_invasive_cgroup_shrink),
> };
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests: cgroup: add tests to verify the zswap writeback path
2024-05-01 15:44 ` Nhat Pham
@ 2024-05-02 19:05 ` Usama Arif
2024-05-02 23:30 ` Nhat Pham
0 siblings, 1 reply; 8+ messages in thread
From: Usama Arif @ 2024-05-02 19:05 UTC (permalink / raw)
To: Nhat Pham
Cc: akpm, hannes, yosryahmed, chengming.zhou, linux-mm, linux-kernel,
kernel-team
On 01/05/2024 16:44, Nhat Pham wrote:
> On Wed, May 1, 2024 at 3:04 AM Usama Arif <usamaarif642@gmail.com> 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@gmail.com>
>> ---
>> 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.
>
>> + }
>> +
>> + ret = KSFT_PASS;
>> +
>> +out:
>> + cg_destroy(test_group);
>> + free(test_group);
>> + return ret;
>> +}
>> +
>> +static int test_zswap_writeback_enabled(const char *root)
>> +{
>> + return test_zswap_writeback(root, true);
>> +}
>> +
>> +static int test_zswap_writeback_disabled(const char *root)
>> +{
>> + return test_zswap_writeback(root, false);
>> +}
>> +
>> /*
>> * 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
>> @@ -425,6 +506,8 @@ struct zswap_test {
>> T(test_zswap_usage),
>> T(test_swapin_nozswap),
>> T(test_zswapin),
>> + T(test_zswap_writeback_enabled),
>> + T(test_zswap_writeback_disabled),
>> T(test_no_kmem_bypass),
>> T(test_no_invasive_cgroup_shrink),
>> };
>> --
>> 2.43.0
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests: cgroup: add tests to verify the zswap writeback path
2024-05-01 17:15 ` Yosry Ahmed
@ 2024-05-02 19:13 ` Usama Arif
0 siblings, 0 replies; 8+ messages in thread
From: Usama Arif @ 2024-05-02 19:13 UTC (permalink / raw)
To: Yosry Ahmed
Cc: akpm, hannes, nphamcs, chengming.zhou, linux-mm, linux-kernel,
kernel-team
On 01/05/2024 18:15, Yosry Ahmed wrote:
> Hi Usama,
>
> On Wed, May 1, 2024 at 3:04 AM Usama Arif <usamaarif642@gmail.com> 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 working on this :)
>
>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
>> ---
>> 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;
> We set the zswap limit to 2M. So for this to work properly we need to
> guarantee that the 3M of random data will compress into more than 2M.
> Is this true for all possible zpool implementations and compression
> algorithms? How likely for this to break and start producing false
> negatives if zswap magically becomes more efficient?
>
> One alternative approach that I used before, although more complex, is
> to start by compressing the memory (i.e. through reclaim) without a
> zswap limit, and check the zswap usage. Then, fault the memory back
> in, set the zswap limit lower than the observed usage, and repeat.
> This should guarantee writeback AFAICT.
>
> Also, using memory.reclaim may be easier than memory.high if you
> follow this approach, as you would need to raise memory.high again to
> be able to decompress the memory.
>
Thanks for the review! I have sent a v2 with the method you described. I
did like the simplicity of the method in this v1 a lot more, and we
could have increased the random memory, which eventhough theoretically
would mean it might not trigger a writeback if there was some new magic
compression method, in practice it would always trigger it and work.
Your suggestion which is in v2 covers both theory and practice :)
>> +
>> + /* 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;
>> + }
>> + }
>> +
>> + ret = KSFT_PASS;
>> +
>> +out:
>> + cg_destroy(test_group);
>> + free(test_group);
>> + return ret;
>> +}
>> +
>> +static int test_zswap_writeback_enabled(const char *root)
>> +{
>> + return test_zswap_writeback(root, true);
>> +}
>> +
>> +static int test_zswap_writeback_disabled(const char *root)
>> +{
>> + return test_zswap_writeback(root, false);
>> +}
>> +
>> /*
>> * 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
>> @@ -425,6 +506,8 @@ struct zswap_test {
>> T(test_zswap_usage),
>> T(test_swapin_nozswap),
>> T(test_zswapin),
>> + T(test_zswap_writeback_enabled),
>> + T(test_zswap_writeback_disabled),
>> T(test_no_kmem_bypass),
>> T(test_no_invasive_cgroup_shrink),
>> };
>> --
>> 2.43.0
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests: cgroup: add tests to verify the zswap writeback path
2024-05-02 19:05 ` Usama Arif
@ 2024-05-02 23:30 ` Nhat Pham
2024-05-03 15:11 ` Usama Arif
0 siblings, 1 reply; 8+ messages in thread
From: Nhat Pham @ 2024-05-02 23:30 UTC (permalink / raw)
To: Usama Arif
Cc: akpm, hannes, yosryahmed, chengming.zhou, linux-mm, linux-kernel,
kernel-team
On Thu, May 2, 2024 at 12:05 PM Usama Arif <usamaarif642@gmail.com> wrote:
>
>
> On 01/05/2024 16:44, Nhat Pham wrote:
> > On Wed, May 1, 2024 at 3:04 AM Usama Arif <usamaarif642@gmail.com> 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@gmail.com>
> >> ---
> >> 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 :)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests: cgroup: add tests to verify the zswap writeback path
2024-05-02 23:30 ` Nhat Pham
@ 2024-05-03 15:11 ` Usama Arif
0 siblings, 0 replies; 8+ messages in thread
From: Usama Arif @ 2024-05-03 15:11 UTC (permalink / raw)
To: Nhat Pham
Cc: akpm, hannes, yosryahmed, chengming.zhou, linux-mm, linux-kernel,
kernel-team
On 03/05/2024 00:30, Nhat Pham wrote:
> On Thu, May 2, 2024 at 12:05 PM Usama Arif <usamaarif642@gmail.com> wrote:
>>
>> On 01/05/2024 16:44, Nhat Pham wrote:
>>> On Wed, May 1, 2024 at 3:04 AM Usama Arif <usamaarif642@gmail.com> 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@gmail.com>
>>>> ---
>>>> 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 :)
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-05-03 15:11 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-01 10:04 [PATCH] selftests: cgroup: add tests to verify the zswap writeback path Usama Arif
2024-05-01 12:29 ` Usama Arif
2024-05-01 15:44 ` Nhat Pham
2024-05-02 19:05 ` Usama Arif
2024-05-02 23:30 ` Nhat Pham
2024-05-03 15:11 ` Usama Arif
2024-05-01 17:15 ` Yosry Ahmed
2024-05-02 19:13 ` Usama Arif
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.