Re: [PATCH bpf-next v5 2/2] selftests/bpf: Add memcg async reclaim test

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3832 bytes --]

> 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