Andrew Morton wrote:
> Meanwhile, AI review has a lot of things to say:
> https://sashiko.dev/#/patchset/[email protected]
> [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
