> 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 000000000000..7c7d299b3b28
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
> @@ -0,0 +1,779 @@
[ ... ]
> +static int real_test_child_work(const char *cgroup_path, char *data_filename,
> + char *time_filename, int read_times)
> +{
> + struct timespec start, end;
> + double elapsed;
> + FILE *fp;
> +
> + if (join_parent_cgroup(cgroup_path))
> + return CHILD_EXIT_JOIN_CGROUP;
> +
> + clock_gettime(CLOCK_MONOTONIC, &start);
> +
> + if (write_file(data_filename))
> + return CHILD_EXIT_WRITE_FILE;
> +
> + if (read_file(data_filename, read_times))
> + return CHILD_EXIT_READ_FILE;
> +
> + clock_gettime(CLOCK_MONOTONIC, &end);
> +
> + if (!time_filename)
> + return CHILD_EXIT_OK;
> +
> + elapsed = (end.tv_sec - start.tv_sec) +
> + (end.tv_nsec - start.tv_nsec) / 1000000000.0;
> +
> + fp = fopen(time_filename, "w");
> + if (!fp)
> + return CHILD_EXIT_TIME_FILE;
> + fprintf(fp, "%.6f", elapsed);
> + fclose(fp);
> +
> + return CHILD_EXIT_OK;
> +}
> +
> +static int get_time(char *time_filename, double *time)
> +{
> + int ret = -1;
> + FILE *fp;
> + char buf[64];
> +
> + fp = fopen(time_filename, "r");
> + if (!ASSERT_OK_PTR(fp, "fopen"))
> + goto out;
> +
> + if (!ASSERT_OK_PTR(fgets(buf, sizeof(buf), fp), "fgets"))
> + goto cleanup;
Is ASSERT_OK_PTR valid for checking fgets() return values? fgets()
returns NULL at end-of-file without setting errno, so ASSERT_OK_PTR (which
expands to libbpf_get_error()) would read from the ambient errno rather
than detecting the EOF. If the timing file is empty, fgets() returns NULL
with errno still zero from the previous successful fopen(), causing
ASSERT_OK_PTR to pass silently and execution to fall through to
sscanf(buf, "%lf", time) with buf uninitialized.
The empty-file case is reachable: real_test_child_work() does not check
the result of fprintf()/fclose() on the timing file, so a child whose
write failed (ENOSPC on /tmp after writing two 32 MB workload files) still
exits CHILD_EXIT_OK, and run_high_low_workload() then calls get_time() on
a zero-length file.
> +
> + if (sscanf(buf, "%lf", time) != 1) {
> + PRINT_FAIL("sscanf %s", buf);
> + goto cleanup;
> + }
[ ... ]
> +/*
> + * test_progs hijacks stdout and only prints the captured log when the
> + * test fails, so benchmark numbers go straight to the real stdout to
> + * keep them from drowning in ASSERT output.
> + */
> +static void bench_printf(const char *fmt, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, fmt);
> + vfprintf(env.stdout_saved ?: stdout, fmt, ap);
> + va_end(ap);
> +}
A subsystem pattern flags this as potentially concerning: bench_printf()
writes benchmark text straight to env.stdout_saved, bypassing the test
framework's stdout hijacking. Outside test_progs.c itself this is the only
writer of env.stdout_saved in the whole selftests/bpf tree - every other
test writes through the hijacked stdout so the framework owns when and
whether the text appears.
The effect is free-form lines interleaved directly into the stream from
which runners and CI parse the per-test pass/fail records, on a path
enabled only by an environment variable, so the same binary produces two
different output shapes. The benchmark timings emitted are informational
(not pass/fail verdicts), the path is gated behind
TEST_MEMCG_ASYNC_RECLAIM_BENCH=1, and prog_tests/ files run under
test_progs rather than kselftest.h. Would dumping benchmark numbers via
the framework's normal stdout keep the output under framework control?
[ ... ]
> +void test_memcg_async_reclaim(void)
> +{
> + u64 high_cgroup_id, low_cgroup_id;
> + double high_time = 0.0, low_time = 0.0;
> + double base_high_time = 0.0, base_low_time = 0.0;
> + struct memcg_async_reclaim *skel = NULL;
> + int err, bench;
> +
> + bench = !!getenv("TEST_MEMCG_ASYNC_RECLAIM_BENCH");
> +
> + err = setup_high_low_cgroups(&high_cgroup_id, &low_cgroup_id);
> + if (!ASSERT_OK(err, "setup_high_low_cgroups reclaim"))
> + return;
> +
> + /*
> + * 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(&base_high_time, &base_low_time,
> + READ_TIMES);
> + if (!ASSERT_OK(err, "run_high_low_workload baseline"))
> + goto out;
> + }
> +
> + err = setup_bpf(high_cgroup_id, low_cgroup_id, &skel);
> + if (!ASSERT_OK(err, "setup_bpf"))
> + goto out;
> +
> + err = run_high_low_workload(&high_time, &low_time, READ_TIMES);
> + if (!ASSERT_OK(err, "run_high_low_workload reclaim"))
> + goto out;
> +
> + /*
> + * The timing comparison below alone cannot distinguish a working
> + * reclaim from a no-op one, so require that the BPF program
> + * actually reclaimed memory from the low cgroup.
> + */
> + if (!ASSERT_GT(skel->bss->reclaim_calls, 0, "reclaim_calls"))
> + goto out;
> + if (!ASSERT_GT(skel->bss->reclaimed_bytes, 0, "reclaimed_bytes"))
> + goto out;
> +
> + /*
> + * The timing comparison is a benchmark and too flaky to fail CI
> + * runs, so it only logs the numbers; the counters above already
> + * proved that the BPF program reclaimed memory.
> + */
> + if (bench && base_high_time > 0.0) {
> + double speedup = base_high_time / high_time;
> +
> + bench_printf("memcg_async_reclaim: baseline high=%f low=%f, "
> + "reclaim high=%f low=%f, speedup=%.2fx\n",
> + base_high_time, base_low_time,
> + high_time, low_time, speedup);
> + }
> + else if (high_time >= low_time)
> + printf("memcg_async_reclaim: high cgroup not improved: high=%f
> low=%f\n",
> + high_time, low_time);
Can this printf() output ever be read? test_progs redirects stdout and
stderr to an in-process memstream, and dump_test_log() only prints that
buffer when the test fails. Both printf() calls added by the patch are
emitted on paths where the test then passes, so their output is discarded.
The same file documents this behaviour and works around it for the
benchmark numbers through bench_printf() and env.stdout_saved. Should
these two informational printf() calls go through bench_printf() as well,
or be dropped?
[ ... ]
> +void test_memcg_async_reclaim_dying(void)
> +{
> + u64 trigger_cgroup_id, target_cgroup_id;
> + u64 calls_before, bytes_before;
> + char data_file[PATH_MAX] = "";
> + struct reclaim_events_seen seen = {};
> + struct memcg_async_reclaim *skel = NULL;
> + struct ring_buffer *rb = NULL;
> + pid_t reader_pid = -1;
> + int err, fd, i;
> +
> + err = setup_dying_cgroups(&trigger_cgroup_id, &target_cgroup_id);
> + if (!ASSERT_OK(err, "setup_dying_cgroups"))
> + return;
> +
> + err = setup_bpf(trigger_cgroup_id, target_cgroup_id, &skel);
> + if (!ASSERT_OK(err, "setup_bpf"))
> + goto out;
> +
> + rb = ring_buffer__new(bpf_map__fd(skel->maps.reclaim_events),
> + on_reclaim_event, &seen, NULL);
> + if (!ASSERT_OK_PTR(rb, "ring_buffer__new"))
> + goto out;
> +
> + snprintf(data_file, sizeof(data_file),
> + "%s/memcg_async_dying_XXXXXX", workload_files_dir());
> + fd = mkstemp(data_file);
> + if (!ASSERT_GE(fd, 0, "mkstemp"))
> + goto out;
> + close(fd);
> +
> + reader_pid = spawn_keepalive_reader(data_file);
> + if (!ASSERT_GT(reader_pid, 0, "fork keepalive reader"))
> + goto out;
Could the keepalive reader child's failure modes be diagnosed?
spawn_keepalive_reader() reports its failures through distinct exit codes
(CHILD_EXIT_JOIN_CGROUP, CHILD_EXIT_WRITE_FILE, CHILD_EXIT_READ_FILE),
but the caller only checks that fork() succeeded. At cleanup the status is
discarded:
kill(reader_pid, SIGKILL);
(void)waitpid(reader_pid, NULL, 0);
If the child dies immediately - for example join_parent_cgroup() fails, or
write_file() cannot create the 32 MB file - the parent still spends 50 *
100ms in the first poll loop and then fails with ASSERT_GT(seen.called, 0,
"reclaim events"), pointing at the kernel rather than at the child.
The child's own diagnostics do not survive either: test_progs sets stdout
and stderr to a per-process memstream, so log_err() output from
join_parent_cgroup() and the fprintf() in read_file() land in the forked
child's private copy of that buffer and are lost at _exit().
The file already has the machinery to handle this - child_exit_str() and
the WIFEXITED/WEXITSTATUS handling in run_high_low_workload(). Would a
waitpid(reader_pid, &status, WNOHANG) probe before or after the poll loop,
reported through child_exit_str(), make this diagnosable?
> +
> + /* Wait for reclaim rounds to reach the live target cgroup. */
> + for (i = 0; i < EVENT_POLL_ROUNDS && !seen.called; i++) {
> + err = ring_buffer__poll(rb, EVENT_POLL_TIMEOUT_MS);
> + if (!ASSERT_GE(err, 0, "ring_buffer__poll"))
> + goto out;
> + }
> + if (!ASSERT_GT(seen.called, 0, "reclaim events"))
> + goto out;
> +
> + remove_cgroup(CG_DYING_TARGET_DIR);
> +
> + usleep(DYING_SETTLE_US);
> + seen = (struct reclaim_events_seen){};
> +
> + calls_before = skel->bss->reclaim_calls;
> + bytes_before = skel->bss->reclaimed_bytes;
> +
> + /* Wait for a reclaim round to report the removed target. */
> + for (i = 0; i < EVENT_POLL_ROUNDS &&
> + !seen.skipped_dying && !seen.target_gone; i++) {
> + err = ring_buffer__poll(rb, EVENT_POLL_TIMEOUT_MS);
> + if (!ASSERT_GE(err, 0, "ring_buffer__poll"))
> + goto out;
> + }
> +
> + if (!seen.skipped_dying && !seen.target_gone) {
> + PRINT_FAIL("no reclaim round hit the removed cgroup within "
> + "the poll window");
> + goto out;
> + }
> +
> + if (!ASSERT_EQ(skel->bss->reclaim_calls, calls_before, "reclaim_calls"))
> + goto out;
> + if (!ASSERT_EQ(skel->bss->reclaimed_bytes, bytes_before,
> "reclaimed_bytes"))
> + goto out;
[ ... ]
---
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/34970614671
