> diff --git a/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
> b/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
> new file mode 100644
> index 0000000000000..179b64fb6de84
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
[ ... ]
> + /*
> + * Optional baseline for the benchmark numbers below: run the
> + * same workload once without the BPF program, so the log can
> + * report how much async reclaim improved the pressured cgroup.
> + */
> + if (bench) {
> + err = run_high_low_workload(dir, &base_high_time,
> + &base_low_time, READ_TIMES);
> + if (!ASSERT_OK(err, "run_high_low_workload baseline"))
> + goto out;
> +
> + /*
> + * The baseline left up to CG_LIMIT of page cache charged to the
> + * high and low cgroups, and memcg charges outlive the workload
> + * processes. Recreate the cgroups so that the instrumented run
> + * below starts from the same cold state the baseline did;
> + * otherwise the reported speedup compares a cold run against a
> + * warm one.
> + */
> + cleanup_cgroup_environment();
Does this comment accurately describe why the cgroups are recreated?
Looking at run_high_low_workload(), its cleanup block closes all four file
descriptors and then unlinks all four temp files after both children have
been reaped. With link count zero and no remaining open descriptors or
mappings, the inodes are evicted, their page cache is truncated, and the
memcg charges are released - so memory.current for the high and low
cgroups is already back to near zero when control returns here.
What actually survives the baseline is memory.stat, particularly the
WORKINGSET_REFAULT_FILE counter that the BPF program samples via
should_reclaim_cgroup(). Without recreating the cgroups, wq_prog_init()
would start elem->prev_event at 0 while the counter is already large from
the baseline, so the very first timer tick would see a huge delta and kick
off a reclaim round unconditionally. Recreating the cgroups zeroes those
counters.
Could the comment be updated to describe that the recreation clears the
WORKINGSET_REFAULT_FILE counters in memory.stat rather than page cache
charges, which are already released when the files are unlinked?
[ ... ]
> + reader_pid = spawn_keepalive_reader(data_fd);
> + if (!ASSERT_GT(reader_pid, 0, "fork keepalive reader"))
> + goto out;
> +
> + /* Wait for reclaim rounds to reach the live target cgroup. */
> + for (i = 0; i < EVENT_POLL_ROUNDS && !seen.called; i++) {
> + n = ring_buffer__poll(rb, EVENT_POLL_TIMEOUT_MS);
> + if (!ASSERT_GE(n, 0, "ring_buffer__poll"))
> + goto out;
> +
> + /*
> + * The reader loops until killed, so finding it already gone
> + * means the refaults the BPF program waits for never started.
> + * Report its exit code rather than letting the assert below
> + * blame the kernel for the full poll window.
> + */
> + if (waitpid(reader_pid, &status, WNOHANG) == reader_pid) {
> + /* Reaped, so cleanup must not signal a reused PID. */
> + reader_pid = -1;
> + if (WIFEXITED(status))
> + PRINT_FAIL("keepalive reader exited early: %s",
> + child_exit_str(WEXITSTATUS(status)));
> + else
> + PRINT_FAIL("keepalive reader died, status 0x%x",
> + status);
> + goto out;
> + }
> + }
> + if (!ASSERT_GT(seen.called, 0, "reclaim events")) {
> + report_timer_failures(skel);
> + goto out;
> + }
> +
> + remove_cgroup(CG_DYING_TARGET_DIR);
> +
> + /* Let reclaim passes that were already in flight drain. */
> + usleep(DYING_SETTLE_US);
> +
> + calls_before = skel->bss->reclaim_calls;
> +
> + /*
> + * Wait for a reclaim round to hit the removed cgroup. Only TARGET_GONE
> + * ends the wait: an attempt that lands inside the rmdir window reports
> + * SKIPPED_DYING first, and stopping there would miss the events that
> + * follow it.
> + */
> + for (i = 0; i < EVENT_POLL_ROUNDS && !seen.target_gone; i++) {
> + n = ring_buffer__poll(rb, EVENT_POLL_TIMEOUT_MS);
> + if (!ASSERT_GE(n, 0, "ring_buffer__poll"))
> + goto out;
> + }
The second poll loop here drops the keepalive-reader liveness check that
the first loop performs, although both loops depend on the reader equally.
TARGET_GONE is only emitted when a reclaim round actually starts, and a
round only starts when should_reclaim_cgroup() sees a fresh
WORKINGSET_REFAULT_FILE delta - which only happens while the reader keeps
refaulting. If the reader dies during this second window, refaults stop, no
TARGET_GONE ever arrives, and the test reports "no reclaim round hit the
removed cgroup (gone=0, dying=0)", blaming the kernel for a dead child.
That is exactly the misattribution the first loop added a waitpid(WNOHANG)
check to avoid. The reader's exit code is still available here, since a
reader that dies during the second loop is left unreaped until the out:
label. Should the second loop include the same liveness check to avoid
misattributing a dead reader as a kernel issue?
> + /*
> + * TARGET_GONE is what the test can rely on: by the time rmdir has
> + * returned, bpf_cgroup_from_id() has stopped resolving the target.
> + * SKIPPED_DYING only happens if an attempt lands inside the rmdir
> + * window, so it is reported here rather than asserted.
> + */
> + if (!seen.target_gone) {
> + report_timer_failures(skel);
> + PRINT_FAIL("no reclaim round hit the removed cgroup (gone=%d,
> dying=%d)",
> + seen.target_gone, seen.skipped_dying);
> + goto out;
> + }
> +
> + /*
> + * Reclaim must have stopped with the target. Both counters stay put for
> + * the same reason: once rmdir has returned, bpf_cgroup_from_id() fails,
> + * so every attempt takes the TARGET_GONE path before it can reach
> + * reclaim_calls++. This therefore checks that reclaim really stops, not
> + * that cgroup_is_dying() works -- see that function in the BPF program
> + * for why the dying window cannot be hit reliably from userspace.
> + *
> + * reclaimed_bytes is compared against 0 rather than against its
> + * pre-removal value: the target is empty for the whole test, so
> + * bpf_proactive_reclaim() has nothing to reclaim and must have returned
> + * 0 on every call, before and after the removal alike. An "unchanged"
> + * comparison would be 0 == 0 and could never fail.
> + */
Does the reclaimed_bytes explanation make sense? The comment argues that
comparing reclaimed_bytes against a pre-removal snapshot "would be 0 == 0
and could never fail", but by its own premise that snapshot is 0, so the
snapshot comparison is literally the assertion that was written:
ASSERT_EQ(skel->bss->reclaimed_bytes, 0ULL). Both forms read
reclaimed_bytes after the removal window and both fail exactly when a
post-removal reclaim adds bytes; neither is vacuous and neither is stronger
than the other.
As written the comment tells a reader that one of two identical checks
cannot fail, which is misleading. Could it just state that
bpf_proactive_reclaim() returns 0 on the empty target throughout the test,
without the comparison that makes them sound different?
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35318390791
