Re: [PATCH 3/3] selftests: add test for zswapin

[Yosry Ahmed <yosryahmed@google.com>]

On Tue, Jan 30, 2024 at 10:31:24AM -0800, Nhat Pham wrote:
> On Mon, Jan 29, 2024 at 5:24 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
[..]
> > > -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 = (size_t)arg;
> > >       char *mem = (char *)malloc(size);
> > > +     int ret = 0;
> > >
> > >       if (!mem)
> > >               return -1;
> > >       for (int i = 0; i < size; i += 4095)
> > >               mem[i] = 'a';
> > > +
> > > +     if (read) {
> > > +             /* cycle through the allocated memory to (z)swap in and out pages */
> > > +             for (int t = 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.

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)?

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

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 == 0. WDYT?

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

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

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 :)