Received-SPF: pass (google.com: domain of linux-kernel+bounces-167062-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240501100446.1454264-1-usamaarif642@gmail.com>
 <CAKEwX=NKMOJQ2KuiBcs-62NzqAhRDOvEhWvAzjsAbUtg=65fvA@mail.gmail.com> <cc634240-bfa0-43c5-b34a-257411d0e6a1@gmail.com>
In-Reply-To: <cc634240-bfa0-43c5-b34a-257411d0e6a1@gmail.com>
From: Nhat Pham <nphamcs@gmail.com>
Date: Thu, 2 May 2024 16:30:49 -0700
Message-ID: <CAKEwX=P3CzkrkB8iQ5-BvgaFko03hUULtz=8z-qe_d3LZ7Ge2Q@mail.gmail.com>
Subject: Re: [PATCH] selftests: cgroup: add tests to verify the zswap
 writeback path
To: Usama Arif <usamaarif642@gmail.com>
Cc: akpm@linux-foundation.org, hannes@cmpxchg.org, yosryahmed@google.com, 
	chengming.zhou@linux.dev, linux-mm@kvack.org, linux-kernel@vger.kernel.org, 
	kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, May 2, 2024 at 12:05=E2=80=AFPM Usama Arif <usamaarif642@gmail.com>=
 wrote:
>
>
> On 01/05/2024 16:44, Nhat Pham wrote:
> > On Wed, May 1, 2024 at 3:04=E2=80=AFAM Usama Arif <usamaarif642@gmail.c=
om> 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/testi=
ng/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 =3D (size_t)arg;
> >> +       char *mem =3D (char *)malloc(size);
> >> +
> >> +       if (!mem)
> >> +               return -1;
> >> +       for (int i =3D 0; i < size; i++)
> >> +               mem[i] =3D rand() % 128;
> >> +       free(mem);
> >> +       return 0;
> >> +}
> >> +
> >>   static char *setup_test_group_1M(const char *root, const char *name)
> >>   {
> >>          char *group_name =3D 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 =3D KSFT_FAIL;
> >> +       char *test_group;
> >> +       long zswpwb_before, zswpwb_after;
> >> +
> >> +       test_group =3D 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 =3D 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 =3D cg_read_key_long(test_group, "memory.stat", "=
zswpwb ");
> >> +       if (wb) {
> >> +               if (zswpwb_after <=3D zswpwb_before) {
> >> +                       ksft_print_msg("writeback enabled and zswpwb_a=
fter <=3D zswpwb_before\n");
> >> +                       goto out;
> >> +               }
> >> +       } else {
> >> +               if (zswpwb_after !=3D zswpwb_before) {
> >> +                       ksft_print_msg("writeback disabled and zswpwb_=
after !=3D 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 :)