> 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..1270d73c9116e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
[ ... ]
> + wait_ret = waitpid(low_pid, &status, 0);
> + if (!ASSERT_GT(wait_ret, 0, "low waitpid"))
> + goto cleanup;
> + /*
> + * The child has been reaped and its PID can already be reused,
> + * so mark it to keep cleanup from signaling an unrelated process.
> + */
> + low_pid = -1;
> + if (!ASSERT_TRUE(WIFEXITED(status), "low exited"))
> + goto cleanup;
> + if (WEXITSTATUS(status) != CHILD_EXIT_OK) {
> + PRINT_FAIL("low child failed at: %s (exit status %d)",
> + child_exit_str(WEXITSTATUS(status)),
> + WEXITSTATUS(status));
> + goto cleanup;
> + }
> +
> + wait_ret = waitpid(high_pid, &status, 0);
> + if (!ASSERT_GT(wait_ret, 0, "high waitpid"))
> + goto cleanup;
> + /* Same as above: the reaped PID must not be signaled again. */
> + high_pid = -1;
> + if (!ASSERT_TRUE(WIFEXITED(status), "high exited"))
> + goto cleanup;
> + if (WEXITSTATUS(status) != CHILD_EXIT_OK) {
> + PRINT_FAIL("high child failed at: %s (exit status %d)",
> + child_exit_str(WEXITSTATUS(status)),
> + WEXITSTATUS(status));
> + goto cleanup;
> + }
The three PRINT_FAIL() calls in run_high_low_workload() omit the trailing
newline, so the failure messages run into whatever is logged next.
PRINT_FAIL() in test_progs.h does not append a newline:
#define PRINT_FAIL(format...) \
({ \
test__fail(); \
fprintf(stdout, "%s:FAIL:%d ", __func__, __LINE__); \
fprintf(stdout, ##format); \
})
Since test_progs hijacks stdout into an open_memstream() buffer that is
dumped verbatim, the missing newline concatenates these lines with the
following log output. The dominant convention in prog_tests/ is to
terminate the format string (33 of the ~44 PRINT_FAIL() call sites end
in "\n").
Should these three PRINT_FAIL() calls (the two shown above and the one
after the high_time/low_time comparison) end with "\n"?
> diff --git a/tools/testing/selftests/bpf/progs/memcg_async_reclaim.c
> b/tools/testing/selftests/bpf/progs/memcg_async_reclaim.c
> new file mode 100644
> index 0000000000000..b2ca5185150fe
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/memcg_async_reclaim.c
[ ... ]
> +static int reclaim_cgroup(u64 cgroup_id)
> +{
> + struct cgroup_memcg cm;
> + int i;
> +
> + if (get_cgroup_memcg_from_id(cgroup_id, &cm))
> + return 0;
> +
> + reclaim_calls++;
> + for (i = 0; i < RECLAIM_MAX_ITER; i++) {
> + u64 nr = bpf_proactive_reclaim(cm.memcg, RECLAIM_SIZE);
> +
> + if (!nr)
> + break;
> + reclaimed_bytes += nr;
> + }
> +
> + put_cgroup_memcg(&cm);
> +
> + return 0;
> +}
This isn't a bug, but reclaim_cgroup() is declared to return int but
returns 0 on every path, including the lookup-failure path, and its only
caller discards the value:
reclaim_work_fn() {
...
if (should_reclaim_cgroup(...)) {
reclaim_cgroup(wq_low_cgroup_id);
bpf_wq_start(&elem->work, 0);
}
...
}
The failure to obtain the memcg is therefore indistinguishable from
success. The file already uses void where a helper has no result
(put_cgroup_memcg), so would it make sense to make reclaim_cgroup() void,
or to return -1 on the lookup failure and react to it?
---
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/33064477499
