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

[Nhat Pham]

On Thu, May 2, 2024 at 12:05 PM Usama Arif <usamaarif642@xxxxxxxxx> wrote:
>
>
> On 01/05/2024 16:44, Nhat Pham wrote:
> > On Wed, May 1, 2024 at 3:04 AM Usama Arif <usamaarif642@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >>   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 :)