Received-SPF: pass (google.com: domain of linux-kernel+bounces-45109-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240129224542.162599-1-nphamcs@gmail.com> <20240129224542.162599-4-nphamcs@gmail.com>
 <ZbhP0JkEe39g3yqk@google.com>
In-Reply-To: <ZbhP0JkEe39g3yqk@google.com>
From: Nhat Pham <nphamcs@gmail.com>
Date: Tue, 30 Jan 2024 10:31:24 -0800
Message-ID: <CAKEwX=Pj+ncE=wZTOBVzT8E-=YVbqr-1CtsMNuZWLAhuaf7wAg@mail.gmail.com>
Subject: Re: [PATCH 3/3] selftests: add test for zswapin
To: Yosry Ahmed <yosryahmed@google.com>
Cc: akpm@linux-foundation.org, shuah@kernel.org, hannes@cmpxchg.org, 
	tj@kernel.org, lizefan.x@bytedance.com, linux-mm@kvack.org, 
	kernel-team@meta.com, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, 
	linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jan 29, 2024 at 5:24=E2=80=AFPM Yosry Ahmed <yosryahmed@google.com>=
 wrote:
>
> On Mon, Jan 29, 2024 at 02:45:42PM -0800, Nhat Pham wrote:
> > We recently encountered a kernel crash on the zswapin path in our
> > internal kernel, which went undetected because of a lack of test
> > coverage for this path. Add a selftest to cover this code path,
> > allocating more memories than the cgroup limit to trigger
>
> s/memories/memory
>
> > swapout/zswapout, then reading the pages back in memories several times=
.
> >
> > Also add a variant of this test that runs with zswap disabled, to verif=
y
> > swapin correctness as well.
> >
> > Suggested-by: Rik van Riel <riel@surriel.com>
> > Signed-off-by: Nhat Pham <nphamcs@gmail.com>
> > ---
> >  tools/testing/selftests/cgroup/test_zswap.c | 67 ++++++++++++++++++++-
> >  1 file changed, 65 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/cgroup/test_zswap.c b/tools/testin=
g/selftests/cgroup/test_zswap.c
> > index 32ce975b21d1..86231c86dc89 100644
> > --- a/tools/testing/selftests/cgroup/test_zswap.c
> > +++ b/tools/testing/selftests/cgroup/test_zswap.c
> > @@ -60,17 +60,39 @@ static long get_zswpout(const char *cgroup)
> >       return cg_read_key_long(cgroup, "memory.stat", "zswpout ");
> >  }
> >
> > -static int allocate_bytes(const char *cgroup, void *arg)
> > +static int allocate_bytes_and_read(const char *cgroup, void *arg, bool=
 read)
> >  {
> >       size_t size =3D (size_t)arg;
> >       char *mem =3D (char *)malloc(size);
> > +     int ret =3D 0;
> >
> >       if (!mem)
> >               return -1;
> >       for (int i =3D 0; i < size; i +=3D 4095)
> >               mem[i] =3D 'a';
> > +
> > +     if (read) {
> > +             /* cycle through the allocated memory to (z)swap in and o=
ut pages */
> > +             for (int t =3D 0; t < 5; t++) {
>
> What benefit does the iteration serve here? I would guess one iteration
> is enough to swap everything in at least once, no?

There might be data races etc. that might not appear in one iteration.
Running multiple iterations increases the probability of these bugs
cropping up.

Admittedly, the same effect could, perhaps, also be achieved by
running the same test multiple times, so this is not a hill I will die
on :) This is just a bit more convenient - CI infra often runs these
tests once every time a new kernel is built.

>
> > +                     for (int i =3D 0; i < size; i +=3D 4095) {
> > +                             if (mem[i] !=3D 'a')
> > +                                     ret =3D -1;
> > +                     }
> > +             }
> > +     }
> > +
> >       free(mem);
> > -     return 0;
> > +     return ret;
> > +}
> > +
> > +static int allocate_bytes(const char *cgroup, void *arg)
> > +{
> > +     return allocate_bytes_and_read(cgroup, arg, false);
> > +}
> > +
> > +static int read_bytes(const char *cgroup, void *arg)
> > +{
> > +     return allocate_bytes_and_read(cgroup, arg, true);
> >  }
>
> I don't like how we reuse allocate_bytes_and_read(), we are not saving
> much. Let's keep allocate_bytes() as-is and add a separate helper. Also,
> I think allocate_and_read_bytes() is easier to read.

Fair!

>
> >
> >  static char *setup_test_group_1M(const char *root, const char *name)
> > @@ -133,6 +155,45 @@ static int test_zswap_usage(const char *root)
> >       return ret;
> >  }
> >
> > +/* Simple test to verify the (z)swapin code paths */
> > +static int test_zswapin_size(const char *root, char *zswap_size)
> > +{
> > +     int ret =3D KSFT_FAIL;
> > +     char *test_group;
> > +
> > +     /* Set up */
> > +     test_group =3D cg_name(root, "zswapin_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.zswap.max", zswap_size))
> > +             goto out;
> > +
> > +     /* Allocate and read more than memory.max to trigger (z)swap in *=
/
> > +     if (cg_run(test_group, read_bytes, (void *)MB(32)))
> > +             goto out;
> > +
> > +     ret =3D KSFT_PASS;
> > +
> > +out:
> > +     cg_destroy(test_group);
> > +     free(test_group);
> > +     return ret;
> > +}
> > +
> > +static int test_swapin(const char *root)
> > +{
> > +     return test_zswapin_size(root, "0");
> > +}
>
> Why are we testing the no zswap case? I am all for testing but it seems
> out of scope here. It would have been understandable if we are testing
> memory.zswap.max itself, but we are not doing that.

Eh it's just by convenience. We already have the workload - any test
for zswap can pretty much be turned into a test for swap by disabling
zswap (and enabling swap), so I was trying to kill two birds with one
stone and cover a bit more of the codebase.

>
> FWIW, I think the tests here should really be separated from cgroup
> tests, but I understand why they were added here. There is a lot of
> testing for memcg interface and control for zswap, and a lot of nice
> helpers present.

Yeah FWIW, I agree :) I wonder if there's an easy way to inherit
helpers from one test suite to another. Some sort of kselftest
dependency? Or maybe move these cgroup helpers up the hierarchy (so
that it can be shared by multiple selftest suites).

>