Re: [PATCH v3 1/7] selftests/cgroup: skip test_zswap if zswap is globally disabled

[Li Wang <liwang@redhat.com>]

Andrew Morton wrote:

> Meanwhile, AI review has a lot of things to say:
> 	https://sashiko.dev/#/patchset/20260313043532.103987-1-liwang@redhat.com

> [Sashiko comment in patch 2/7]
> ...
> Could this diagnostic message be updated to print the expected swap amount
> in KB or bytes to avoid confusion?

Yes, good point, will modify it in v4.

> [Sashiko comments in patch 4/7]
> ...
> Is this description accurate?
> Looking at tools/testing/selftests/cgroup/test_memcontrol.c, the macro is
> also used as a memory stride to trigger page faults:
>     for (ptr = buf; ptr < buf + size; ptr += BUF_SIZE)
>         *ptr = 0;
> Could renaming this to BUF_SIZE obscure the original intent of touching
> each memory page once to force it to be resident?

Well, maybe yes, we can use page_size for this one.

> Could this lead to truncation of /proc/self/mounts on architectures like
> arm64?
> Since BUF_SIZE is strictly 4096, this reduces the buffer size from 640KB
> (when PAGE_SIZE was 64K from system headers) to 40KB. If the mounts file
> exceeds this limit, the buffer is cleanly null-terminated but the last line
> is truncated.
> Later in this function, strtok() extracts tokens without checking for NULL:
>     type = strtok(NULL, delim);
>     options = strtok(NULL, delim);
>     ...
>     if (strcmp(type, "cgroup") == 0) {
>         if (!controller || !strstr(options, controller))
> Will passing a NULL type or options pointer directly to string comparison
> functions cause a segmentation fault?

Theoretically, you're right. But in real situation we haven't observe such
failure so we can just add NULL checks on the strtok() return values.

    options = strtok(NULL, delim);
    if (!type || !options)
        continue;

> Does this code evaluate its exit condition correctly when waiting for a
> cgroup to become empty?
> When test code wants to wait for a cgroup to become empty by passing count
> = 0 (e.g., in test_cgfreezer_forkbomb()), the loop exit condition is:
>     if (nr >= count)
>         return 0;
> Doesn't nr >= 0 evaluate to true on the very first iteration, returning
> immediately and bypassing the intended wait intervals? Could this cause
> subsequent cg_destroy() calls to fail with EBUSY because the processes have
> not yet fully terminated?

Okay, this smeems a problem, but not blong to this patchset.
I would sugguest fixing it in another series.

> Is it safe to ignore the return value of read() here?
> If the read() fails, the loop will continue incrementing the offset without
> instantiating the data into the page cache:
>     for (i = 0; i < size; i += sizeof(buf))
>         read(fd, buf, sizeof(buf));
> Can this silently fail to establish the required memory conditions for the
> tests?

This also can be improved in another series. Shouldn't resolve in
this one.

> Can this dereference a NULL pointer under memory pressure?

Theoretically, you're right. But I think NULL pointer checks can
be added in another series.

> [Sashiko comments in patch 5/7]
> ...
> If malloc() fails and returns NULL, does this loop dereference a NULL pointer
> when writing to control_allocation[i]? Other allocations in this file seem to
> check for NULL before proceeding.

Theoretically, you're right. But I think NULL pointer checks can
be added in another series.

> [Sashiko comment in patch 6/7]
> ...
> Does this larger allocation size require a NULL check on the malloc return
> value later in this function?

Theoretically, you're right. But I think NULL pointer checks can
be added in another series.

>> +	snprintf(zswap_max_buf, sizeof(zswap_max_buf), "%ld", pagesize);
> This isn't a bug, but since pagesize is a size_t, should this use the %zu
> format specifier instead of %ld to avoid compiler warnings on 32-bit
> platforms where size_t is an unsigned int?

Good point, let's use %zu in patch v4.

> Will this new limit consistently trigger zswap writeback on systems with
> efficient memory compression?

At least based on my testing, (whether on 4k or 64k systems), everything
runs smoothly; this change has indeed reduced instances of false failures
in real-world.

-- 
Regards,
Li Wang