Received-SPF: pass (google.com: domain of linux-kernel+bounces-45144-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 30 Jan 2024 18:54:31 +0000
In-Reply-To: <CAKEwX=Pj+ncE=wZTOBVzT8E-=YVbqr-1CtsMNuZWLAhuaf7wAg@mail.gmail.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240129224542.162599-1-nphamcs@gmail.com> <20240129224542.162599-4-nphamcs@gmail.com>
 <ZbhP0JkEe39g3yqk@google.com> <CAKEwX=Pj+ncE=wZTOBVzT8E-=YVbqr-1CtsMNuZWLAhuaf7wAg@mail.gmail.com>
Message-ID: <ZblF54ZpiUzzsbbf@google.com>
Subject: Re: [PATCH 3/3] selftests: add test for zswapin
From: Yosry Ahmed <yosryahmed@google.com>
To: Nhat Pham <nphamcs@gmail.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 Tue, Jan 30, 2024 at 10:31:24AM -0800, Nhat Pham wrote:
> On Mon, Jan 29, 2024 at 5:24=E2=80=AFPM Yosry Ahmed <yosryahmed@google.co=
m> wrote:
> >
[..]
> > > -static int allocate_bytes(const char *cgroup, void *arg)
> > > +static int allocate_bytes_and_read(const char *cgroup, void *arg, bo=
ol 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=
 out 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?
>=20
> There might be data races etc. that might not appear in one iteration.
> Running multiple iterations increases the probability of these bugs
> cropping up.

Hmm this is a test running in a single process, and I assume the rest of
the system would be idle (at least from a zswap perspective). Did the
iterations actually catch problems in this scenario (not specifically in
this test, but generally in similar testing)?

>=20
> 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.
>=20
[..]
> > > +
> > > +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.
>=20
> 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.

We can check that no data is actually in zswap after
test_zswapin_size(root, "0"), in which case it becomes more of a zswap
test and we get a sanity check for memory.zswap.max =3D=3D 0. WDYT?

Perhaps we can rename it to test_swpain_nozswap() or so.

>=20
> >
> > 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.
>=20
> 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).

I am not fluent in kselftest so I can't claim to know the answer here.
There are a lot of things to do testing-wise for zswap, but I am not
asking anyone to do it because I don't have the time to do it myself. It
would be nice though :)