linux-kernel - Re: [PATCH v2 3/3] selftests: add zswapin and no zswap tests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAJD7tkbu+nGMuvkK3B9-Ekt9+P_wtwOM1A_9cAM0wLM7trO+CQ@mail.gmail.com>
Date: Fri, 2 Feb 2024 17:35:49 -0800
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Nhat Pham <nphamcs@...il.com>
Cc: akpm@...ux-foundation.org, riel@...riel.com, shuah@...nel.org, 
	hannes@...xchg.org, tj@...nel.org, lizefan.x@...edance.com, 
	roman.gushchin@...ux.dev, linux-mm@...ck.org, kernel-team@...a.com, 
	linux-kernel@...r.kernel.org, cgroups@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v2 3/3] selftests: add zswapin and no zswap tests

> > > +{
> > > +     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!