Received-SPF: pass (google.com: domain of linux-kernel+bounces-50811-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: <20240201032718.1968208-1-nphamcs@gmail.com> <20240201032718.1968208-4-nphamcs@gmail.com>
 <Zbtfku0wVGXBHDTD@google.com> <CAKEwX=PH5abBFCjYHL+d99v8MMwiASqP83aF2vSv1iwezX3UHA@mail.gmail.com>
In-Reply-To: <CAKEwX=PH5abBFCjYHL+d99v8MMwiASqP83aF2vSv1iwezX3UHA@mail.gmail.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Fri, 2 Feb 2024 17:35:49 -0800
Message-ID: <CAJD7tkbu+nGMuvkK3B9-Ekt9+P_wtwOM1A_9cAM0wLM7trO+CQ@mail.gmail.com>
Subject: Re: [PATCH v2 3/3] selftests: add zswapin and no zswap tests
To: Nhat Pham <nphamcs@gmail.com>
Cc: akpm@linux-foundation.org, riel@surriel.com, shuah@kernel.org, 
	hannes@cmpxchg.org, tj@kernel.org, lizefan.x@bytedance.com, 
	roman.gushchin@linux.dev, 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"

> > > +{
> > > +     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';
> >
> > cgroup_util.h defines PAGE_SIZE, see alloc_anon() for example.
> >
> > On that note, alloc_anon() is awfully close to allocate_bytes() below,
> > perhaps we should consolidate them. The only difference I see is that
> > alloc_anon() does not check for the allocation failure, but a lot of
> > functions in cgroup_helpers.c don't, so it seems intentional for
> > simplification.
>
> Hmm I didn't know about this function. I think it was Domenico who
> added allocate_bytes() for the initial zswap tests, and I've just been
> piggybacking on it ever since:
> https://github.com/torvalds/linux/commit/d9cfaf405b8ffe2c716b1ce4c82e0a19d50951da
>
> I can send a separate patch to clean this up later :) Doesn't seem that bad.

SGTM.

[..]
> >
> > > +     if (cg_write(test_group, "memory.zswap.max", "0"))
> > > +             goto out;
> > > +
> > > +     /* Allocate and read more than memory.max to trigger swapin */
> > > +     if (cg_run(test_group, allocate_bytes_and_read, (void *)MB(32)))
> > > +             goto out;
> > > +
> > > +     /* Verify that no zswap happened */
> >
> > If we want to be really meticulous, we can verify that we did swap out,
> > but not to zswap. IOW, we can check memory.swap.current or something.
>
> Hmm would memory.swap.current go back to 0 once the memory-in-swap is
> freed? It doesn't seem like we have any counters at the cgroup level
> for swapout/swapin events. Maybe such counters were not useful enough
> to justify the extra overhead of maintaining them? :)
>
> Anyway, I think checking zswpout should probably be enough here.
> That's the spirit of the test anyway - make absolutely sure that no
> zswap-out happened.

The test is making sure that even though we used real swap, we did not
use zswap. In other words, we may see a false positive if something
goes wrong and we don't swap anything at all. I know I am probably
being paranoid here :)

How about we check memory.swap.peak?

[..]
> > > +     test_group = 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", "max"))
> > > +             goto out;
> > > +
> > > +     /* Allocate and read more than memory.max to trigger (z)swap in */
> > > +     if (cg_run(test_group, allocate_bytes_and_read, (void *)MB(32)))
> > > +             goto out;
> >
> > We should probably check for a positive zswapin here, no?
>
> Oh right. I'll just do a quick check here:
>
> zswpin = cg_read_key_long(test_group, "memory.stat", "zswpin ");
> if (zswpin < 0) {
>    ksft_print_msg("Failed to get zswpin\n");
>    goto out;
> }
>
> if (zswpin == 0) {
>    ksft_print_msg("zswpin should not be 0\n");
>    goto out;
> }

SGTM.

Thanks!