[PATCH v2 0/1] cgroup: add tests to verify the zswap writeback path

[PATCH v2 0/1] cgroup: add tests to verify the zswap writeback path

[Usama Arif]

Initate writeback with the steps described in the commit message and check
using memory.stat.zswpwb if zswap writeback occurred.

v1 -> v2:
- Change method of causing writeback from limit zswap to memory reclaim.
  (Further described in commit message) (Yosry Ahmed)
- Document why using random memory (Nhat Pham)

Usama Arif (1):
  selftests: cgroup: add tests to verify the zswap writeback path

 tools/testing/selftests/cgroup/test_zswap.c | 125 +++++++++++++++++++-
 1 file changed, 124 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v2 1/1] selftests: cgroup: add tests to verify the zswap writeback path

[Usama Arif]

Initate writeback with the below steps and check using
memory.stat.zswpwb if zswap writeback occurred:
1. Allocate memory.
2. Reclaim memory equal to the amount that was allocated in step 1.
   This will move it into zswap.
3. Save current zswap usage.
4. Move the memory allocated in step 1 back in from zswap.
5. Set zswap.max to half the amount that was recorded in step 3.
6. Attempt to reclaim memory equal to the amount that was allocated,
   this will either trigger writeback if its enabled, or reclamation
   will fail if writeback is disabled as there isn't enough zswap
   space.

Suggested-by: Nhat Pham <nphamcs@gmail.com>
Signed-off-by: Usama Arif <usamaarif642@gmail.com>
---
 tools/testing/selftests/cgroup/test_zswap.c | 125 +++++++++++++++++++-
 1 file changed, 124 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
index f0e488ed90d8..cd864ab825d0 100644
--- a/tools/testing/selftests/cgroup/test_zswap.c
+++ b/tools/testing/selftests/cgroup/test_zswap.c
@@ -50,7 +50,7 @@ static int get_zswap_stored_pages(size_t *value)
 	return read_int("/sys/kernel/debug/zswap/stored_pages", value);
 }
 
-static int get_cg_wb_count(const char *cg)
+static long get_cg_wb_count(const char *cg)
 {
 	return cg_read_key_long(cg, "memory.stat", "zswpwb");
 }
@@ -248,6 +248,127 @@ static int test_zswapin(const char *root)
 	return ret;
 }
 
+/*
+ * Initate writeback with the following steps:
+ * 1. Allocate memory.
+ * 2. Reclaim memory equal to the amount that was allocated in step 1.
+      This will move it into zswap.
+ * 3. Save current zswap usage.
+ * 4. Move the memory allocated in step 1 back in from zswap.
+ * 5. Set zswap.max to half the amount that was recorded in step 3.
+ * 6. Attempt to reclaim memory equal to the amount that was allocated,
+      this will either trigger writeback if its enabled, or reclamation
+      will fail if writeback is disabled as there isn't enough zswap space.
+ */
+static int initiate_writeback(const char *cgroup, void *arg)
+{
+	char *test_group = arg;
+	size_t memsize = MB(4);
+	int ret = -1;
+	bool wb_enabled = cg_read_long(test_group, "memory.zswap.writeback");
+	long zswap_usage;
+
+	if (cg_write(test_group, "memory.max", "8M"))
+		return ret;
+
+	/* Allocate random memory to enusre high zswap memory usage */
+	char *mem = (char *)malloc(memsize);
+
+	if (!mem)
+		return ret;
+	for (int i = 0; i < memsize; i++)
+		mem[i] = rand() % 128;
+
+	/* Try and reclaim allocated memory */
+	if (cg_write(test_group, "memory.reclaim", "4M")) {
+		ksft_print_msg("Failed to reclaim all of the requested memory\n");
+		goto out;
+	}
+
+	zswap_usage = cg_read_long(test_group, "memory.zswap.current");
+
+	/* zswpin */
+	for (int i = 0; i < memsize; i++) {
+		if (mem[i] < 0 || mem[i] > 127) {
+			ksft_print_msg("invalid memory\n");
+			ret = -1;
+		}
+	}
+
+	if (cg_write_numeric(test_group, "memory.zswap.max", zswap_usage/2))
+		goto out;
+
+	/*
+	 * If writeback is enabled, trying to reclaim memory now will trigger a
+	 * writeback as zswap.max is half of what was needed when reclaim ran the first time.
+	 * If writeback is disabled, memory reclaim will fail as zswap is limited and
+	 * it can't writeback to swap.
+	 */
+	ret = cg_write(test_group, "memory.reclaim", "4M");
+	if (!wb_enabled && ret)
+		ret = 0;
+out:
+	free(mem);
+	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.zswap.writeback", wb ? "1" : "0"))
+		return ret;
+
+	zswpwb_before = get_cg_wb_count(test_group);
+	if (zswpwb_before < 0) {
+		ksft_print_msg("failed to get zswpwb_before\n");
+		goto out;
+	}
+
+	if (cg_run(test_group, initiate_writeback, (void *) test_group))
+		goto out;
+
+	/* Verify that zswap writeback occurred only if writeback was enabled */
+	zswpwb_after = get_cg_wb_count(test_group);
+	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 +546,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

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

[Yosry Ahmed]

On Thu, May 2, 2024 at 12:02 PM Usama Arif <usamaarif642@gmail.com> wrote:
>
> Initate writeback with the below steps and check using
> memory.stat.zswpwb if zswap writeback occurred:
> 1. Allocate memory.
> 2. Reclaim memory equal to the amount that was allocated in step 1.
>    This will move it into zswap.
> 3. Save current zswap usage.
> 4. Move the memory allocated in step 1 back in from zswap.
> 5. Set zswap.max to half the amount that was recorded in step 3.
> 6. Attempt to reclaim memory equal to the amount that was allocated,
>    this will either trigger writeback if its enabled, or reclamation
>    will fail if writeback is disabled as there isn't enough zswap
>    space.
>
> Suggested-by: Nhat Pham <nphamcs@gmail.com>
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> ---
>  tools/testing/selftests/cgroup/test_zswap.c | 125 +++++++++++++++++++-
>  1 file changed, 124 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
> index f0e488ed90d8..cd864ab825d0 100644
> --- a/tools/testing/selftests/cgroup/test_zswap.c
> +++ b/tools/testing/selftests/cgroup/test_zswap.c
> @@ -50,7 +50,7 @@ static int get_zswap_stored_pages(size_t *value)
>         return read_int("/sys/kernel/debug/zswap/stored_pages", value);
>  }
>
> -static int get_cg_wb_count(const char *cg)
> +static long get_cg_wb_count(const char *cg)
>  {
>         return cg_read_key_long(cg, "memory.stat", "zswpwb");
>  }
> @@ -248,6 +248,127 @@ static int test_zswapin(const char *root)
>         return ret;
>  }
>
> +/*
> + * Initate writeback with the following steps:
> + * 1. Allocate memory.
> + * 2. Reclaim memory equal to the amount that was allocated in step 1.
> +      This will move it into zswap.
> + * 3. Save current zswap usage.
> + * 4. Move the memory allocated in step 1 back in from zswap.
> + * 5. Set zswap.max to half the amount that was recorded in step 3.
> + * 6. Attempt to reclaim memory equal to the amount that was allocated,
> +      this will either trigger writeback if its enabled, or reclamation
> +      will fail if writeback is disabled as there isn't enough zswap space.
> + */
> +static int initiate_writeback(const char *cgroup, void *arg)
> +{
> +       char *test_group = arg;
> +       size_t memsize = MB(4);
> +       int ret = -1;
> +       bool wb_enabled = cg_read_long(test_group, "memory.zswap.writeback");
> +       long zswap_usage;

Nit: Reverse christmas tree (i.e. is usually more aesthetically
pleasing, but it isn't consistently followed but up to you. You can
separate the declaration and initialization of wb_enabled to do that
if you choose to.

> +
> +       if (cg_write(test_group, "memory.max", "8M"))
> +               return ret;

Why do we need to set this?

> +
> +       /* Allocate random memory to enusre high zswap memory usage */

typo: ensure

> +       char *mem = (char *)malloc(memsize);
> +
> +       if (!mem)
> +               return ret;
> +       for (int i = 0; i < memsize; i++)
> +               mem[i] = rand() % 128;

I am curious, what compression ratio do you observe in practice with
this? Is there a risk of pages becoming totally incompressible and
skipping zswap? One way to guarantee the page compressibility is to
add a bunch of zeros. I usually fill half of it with zeros and half of
it with random data. Not sure if this is actually required though.

> +
> +       /* Try and reclaim allocated memory */
> +       if (cg_write(test_group, "memory.reclaim", "4M")) {
> +               ksft_print_msg("Failed to reclaim all of the requested memory\n");
> +               goto out;
> +       }
> +
> +       zswap_usage = cg_read_long(test_group, "memory.zswap.current");
> +
> +       /* zswpin */
> +       for (int i = 0; i < memsize; i++) {
> +               if (mem[i] < 0 || mem[i] > 127) {
> +                       ksft_print_msg("invalid memory\n");
> +                       ret = -1;
> +               }
> +       }

I understand this correctness check is not strictly needed, but it is
very weak right now. There is a 50% chance that corruption will be
missed because the range we are checking is half the possible values.

If we want a reliable correctness check, perhaps we should fill the
page with increasing known values instead. Then after zswpin, we can
check that the data is exactly what we filled in the first place.

Is there any value in using random data here? If there is, we can
store a second copy of the array to compare against instead.

> +
> +       if (cg_write_numeric(test_group, "memory.zswap.max", zswap_usage/2))
> +               goto out;
> +
> +       /*
> +        * If writeback is enabled, trying to reclaim memory now will trigger a
> +        * writeback as zswap.max is half of what was needed when reclaim ran the first time.
> +        * If writeback is disabled, memory reclaim will fail as zswap is limited and
> +        * it can't writeback to swap.
> +        */
> +       ret = cg_write(test_group, "memory.reclaim", "4M");
> +       if (!wb_enabled && ret)

Should we assert that ret is -EBUSY if !wb_enabled instead? Right now
any error code will pass. The test will also pass if reclaim succeeds
while writeback is disabled, which is not correct behavior.

> +               ret = 0;
> +out:
> +       free(mem);
> +       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");

Why do we need different cgroup names? The tests do not run in
parallel, do they?

> +       if (!test_group)
> +               goto out;
> +       if (cg_create(test_group))
> +               goto out;
> +       if (cg_write(test_group, "memory.zswap.writeback", wb ? "1" : "0"))
> +               return ret;
> +
> +       zswpwb_before = get_cg_wb_count(test_group);
> +       if (zswpwb_before < 0) {

Should we assert zswpwb_before == 0 instead?

> +               ksft_print_msg("failed to get zswpwb_before\n");
> +               goto out;
> +       }
> +
> +       if (cg_run(test_group, initiate_writeback, (void *) test_group))

Nit: initiate_writeback() is not a very descriptive name because it
may or may not trigger writeback.

> +               goto out;
> +
> +       /* Verify that zswap writeback occurred only if writeback was enabled */
> +       zswpwb_after = get_cg_wb_count(test_group);
> +       if (wb) {
> +               if (zswpwb_after <= zswpwb_before) {

If we assert zswpwb_before == 0 above, this can be simplified. Also, I
think a single condition is enough, the message contents can be
inferred by which test failed.

> +                       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 +546,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
>

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

[Usama Arif]

On 03/05/2024 03:36, Yosry Ahmed wrote:
> On Thu, May 2, 2024 at 12:02 PM Usama Arif <usamaarif642@gmail.com> wrote:
>> Initate writeback with the below steps and check using
>> memory.stat.zswpwb if zswap writeback occurred:
>> 1. Allocate memory.
>> 2. Reclaim memory equal to the amount that was allocated in step 1.
>>     This will move it into zswap.
>> 3. Save current zswap usage.
>> 4. Move the memory allocated in step 1 back in from zswap.
>> 5. Set zswap.max to half the amount that was recorded in step 3.
>> 6. Attempt to reclaim memory equal to the amount that was allocated,
>>     this will either trigger writeback if its enabled, or reclamation
>>     will fail if writeback is disabled as there isn't enough zswap
>>     space.
>>
>> Suggested-by: Nhat Pham <nphamcs@gmail.com>
>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
>> ---
>>   tools/testing/selftests/cgroup/test_zswap.c | 125 +++++++++++++++++++-
>>   1 file changed, 124 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testing/selftests/cgroup/test_zswap.c
>> index f0e488ed90d8..cd864ab825d0 100644
>> --- a/tools/testing/selftests/cgroup/test_zswap.c
>> +++ b/tools/testing/selftests/cgroup/test_zswap.c
>> @@ -50,7 +50,7 @@ static int get_zswap_stored_pages(size_t *value)
>>          return read_int("/sys/kernel/debug/zswap/stored_pages", value);
>>   }
>>
>> -static int get_cg_wb_count(const char *cg)
>> +static long get_cg_wb_count(const char *cg)
>>   {
>>          return cg_read_key_long(cg, "memory.stat", "zswpwb");
>>   }
>> @@ -248,6 +248,127 @@ static int test_zswapin(const char *root)
>>          return ret;
>>   }
>>
>> +/*
>> + * Initate writeback with the following steps:
>> + * 1. Allocate memory.
>> + * 2. Reclaim memory equal to the amount that was allocated in step 1.
>> +      This will move it into zswap.
>> + * 3. Save current zswap usage.
>> + * 4. Move the memory allocated in step 1 back in from zswap.
>> + * 5. Set zswap.max to half the amount that was recorded in step 3.
>> + * 6. Attempt to reclaim memory equal to the amount that was allocated,
>> +      this will either trigger writeback if its enabled, or reclamation
>> +      will fail if writeback is disabled as there isn't enough zswap space.
>> + */
>> +static int initiate_writeback(const char *cgroup, void *arg)
>> +{
>> +       char *test_group = arg;
>> +       size_t memsize = MB(4);
>> +       int ret = -1;
>> +       bool wb_enabled = cg_read_long(test_group, "memory.zswap.writeback");
>> +       long zswap_usage;
> Nit: Reverse christmas tree (i.e. is usually more aesthetically
> pleasing, but it isn't consistently followed but up to you. You can
> separate the declaration and initialization of wb_enabled to do that
> if you choose to.
>
Thanks for the review. Will change it in next revision.


>> +
>> +       if (cg_write(test_group, "memory.max", "8M"))
>> +               return ret;
> Why do we need to set this?
Not needed, will remove.
>> +
>> +       /* Allocate random memory to enusre high zswap memory usage */
> typo: ensure
>
>> +       char *mem = (char *)malloc(memsize);
>> +
>> +       if (!mem)
>> +               return ret;
>> +       for (int i = 0; i < memsize; i++)
>> +               mem[i] = rand() % 128;
> I am curious, what compression ratio do you observe in practice with
> this? Is there a risk of pages becoming totally incompressible and
> skipping zswap? One way to guarantee the page compressibility is to
> add a bunch of zeros. I usually fill half of it with zeros and half of
> it with random data. Not sure if this is actually required though.
>
With the default zswap parameters, zswap.current is 3491645, so about 
1.2:1. I had tried a few different zswap parameters and compression was 
still taking place. I initially tried the method from allocate_bytes, 
but the compression was too high with all the zeros and zswap was a 
really small value. Will switch to your method in next revision.

>> +
>> +       /* Try and reclaim allocated memory */
>> +       if (cg_write(test_group, "memory.reclaim", "4M")) {
>> +               ksft_print_msg("Failed to reclaim all of the requested memory\n");
>> +               goto out;
>> +       }
>> +
>> +       zswap_usage = cg_read_long(test_group, "memory.zswap.current");
>> +
>> +       /* zswpin */
>> +       for (int i = 0; i < memsize; i++) {
>> +               if (mem[i] < 0 || mem[i] > 127) {
>> +                       ksft_print_msg("invalid memory\n");
>> +                       ret = -1;
>> +               }
>> +       }
> I understand this correctness check is not strictly needed, but it is
> very weak right now. There is a 50% chance that corruption will be
> missed because the range we are checking is half the possible values.
>
> If we want a reliable correctness check, perhaps we should fill the
> page with increasing known values instead. Then after zswpin, we can
> check that the data is exactly what we filled in the first place.
>
> Is there any value in using random data here? If there is, we can
> store a second copy of the array to compare against instead.
So my thought over here was not to do a correctness check for zswap, 
just to access memory to do zswpin. Switching to the method in the 
comment above, we can do a correctness check as well, so will add that 
in next revision.
>> +
>> +       if (cg_write_numeric(test_group, "memory.zswap.max", zswap_usage/2))
>> +               goto out;
>> +
>> +       /*
>> +        * If writeback is enabled, trying to reclaim memory now will trigger a
>> +        * writeback as zswap.max is half of what was needed when reclaim ran the first time.
>> +        * If writeback is disabled, memory reclaim will fail as zswap is limited and
>> +        * it can't writeback to swap.
>> +        */
>> +       ret = cg_write(test_group, "memory.reclaim", "4M");
>> +       if (!wb_enabled && ret)
> Should we assert that ret is -EBUSY if !wb_enabled instead? Right now
> any error code will pass. The test will also pass if reclaim succeeds
> while writeback is disabled, which is not correct behavior.
I believe its -EAGAIN, but yes will change it.
>> +               ret = 0;
>> +out:
>> +       free(mem);
>> +       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");
> Why do we need different cgroup names? The tests do not run in
> parallel, do they?
>
It makes the tests independent of each other (for e.g. if cg_destroy for 
any magical reason fails for one of the tests, the cgroup creation of 
the other test wont be affected). Plus, I found it easier for debugging 
to examine cgroups after the test was executed. But no strong 
preference, will change it to one name.

>> +       if (!test_group)
>> +               goto out;
>> +       if (cg_create(test_group))
>> +               goto out;
>> +       if (cg_write(test_group, "memory.zswap.writeback", wb ? "1" : "0"))
>> +               return ret;
>> +
>> +       zswpwb_before = get_cg_wb_count(test_group);
>> +       if (zswpwb_before < 0) {
> Should we assert zswpwb_before == 0 instead?
Sure, will do in next revision.
>> +               ksft_print_msg("failed to get zswpwb_before\n");
>> +               goto out;
>> +       }
>> +
>> +       if (cg_run(test_group, initiate_writeback, (void *) test_group))
> Nit: initiate_writeback() is not a very descriptive name because it
> may or may not trigger writeback.
>
>> +               goto out;
>> +
>> +       /* Verify that zswap writeback occurred only if writeback was enabled */
>> +       zswpwb_after = get_cg_wb_count(test_group);
>> +       if (wb) {
>> +               if (zswpwb_after <= zswpwb_before) {
> If we assert zswpwb_before == 0 above, this can be simplified. Also, I
> think a single condition is enough, the message contents can be
> inferred by which test failed.
>
>> +                       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 +546,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
>>