Re: [PATCH v4 0/7] selftests/cgroup: improve zswap tests robustness and support large page sizes

[Li Wang <liwang@redhat.com>]

On Sun, Mar 22, 2026 at 09:18:51AM -0700, Andrew Morton wrote:
> On Sun, 22 Mar 2026 14:10:31 +0800 Li Wang <liwang@redhat.com> wrote:
> 
> > This patchset aims to fix various spurious failures and improve the overall
> > robustness of the cgroup zswap selftests.
> 
> AI review has questions:
> 	https://sashiko.dev/#/patchset/20260322061038.156146-1-liwang@redhat.com

> [Sashiko comments in patch 4/7]
> ...
> Could we update this loop, along with the identical loops in
> alloc_anon_noexit() and alloc_anon_50M_check_swap() shown below, to use
> sysconf(_SC_PAGESIZE) instead?

I found that Waiman submit another patch that do same thing like this
suggestion. I'd consider to merge that one into my patch 4/7.

So, let me talk to Waiman first.


> [Sashiko comments in patch 5/7]
> ..
> if (zswpin < MB(24) / sysconf(_SC_PAGESIZE)) {
> Should these also be updated to use the new global pagesize variable for
> consistency? Subsequent patches in the series do not seem to correct this
> omission.

Good catch, that remainds should be corrected too.

> If control_allocation is NULL, wouldn't the loop immediately dereference it
> and cause an unhandled segmentation fault rather than a graceful test
> failure?

That's right, but better to be resolved in another series, not this one.

> However, there does not appear to be a corresponding munmap() call in the
> test's cleanup path. Although the OS reclaims this memory when the test
> process exits, should this explicit unmap be added for a balanced resource
> lifecycle within the test?

That's right, but better to be resolved in another series, not this one.


> [Sashiko comments in patch 6/7]
> ...
> If malloc returns a null pointer in a memory-constrained environment, the
> loop will unconditionally dereference it. Should there be a null check
> before the loop?

That's right, but better to be resolved in another series, not this one.

> The test data is generated by writing a single 'a' character per page, leaving
> the rest zero-filled:

> 	for (int i = 0; i < control_allocation_size; i += pagesize)
> 		control_allocation[i] = 'a';

> This makes the data highly compressible. Because memory.max is set to half of
> control_allocation_size, 512 pages are pushed into zswap.

> 512 pages of mostly zeros can compress down to roughly 11 to 15 kilobytes
> using compressors like zstd, which is well below the 65536 byte (64k)
> zswap.max limit on a 64k page system.

> Since the limit might not be reached, writeback might never trigger,
> causing the test to falsely fail. Should the test use incompressible data
> or a lower fixed limit?

If Sashiko suggests reducing compressibility, we'd need to fill a significant
fraction of each page with varied data, but that would work against the test:

zswap would reject poorly compressing pages and send them straight to swap,
and memory.stat:zswapped might never reach the threshold the test checks
with cg_read_key_long(..., "zswapped") < 1.

So, at most I'd keep the data highly compressible and just ensure non-zero,
unique-per-page markers.

i.e.

  control_allocation[i] = (char)((i / pagesize) + 1);

-- 
Regards,
Li Wang