Re: [PATCH bpf-next v9 3/3] selftests/bpf: Add a test for arena fault-in under memory.max

[bot+bpf-ci@kernel.org]

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

> diff --git a/tools/testing/selftests/bpf/prog_tests/arena_memcg.c b/tools/testing/selftests/bpf/prog_tests/arena_memcg.c
> new file mode 100644
> index 0000000000000..c76a7eb2f01d9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/arena_memcg.c

[ ... ]

> +	/*
> +	 * Step 5: fault ARENA_FAULT of arena in, much more than
> +	 * HEADROOM. Once it hits memory.max every further page has to
> +	 * come from reclaiming the page cache. With the fix the
> +	 * fault-in reclaims and all of it succeeds; without it the
> +	 * allocation cannot reclaim and we die on a valid address.
> +	 */
> +	for (i = 0; i < npages; i++)
> +		base[(size_t)i * ps] = 1;
> +	_exit(0); /* fd deliberately kept open until here */
> +}
> +
> +	if (!ASSERT_EQ(waitpid(pid, &status, 0), pid, "waitpid"))
> +		goto out;
> +
> +	/* The working directory cannot hold a reclaimable page cache. */
> +	if (WIFEXITED(status) && WEXITSTATUS(status) == CHILD_UNSUPPORTED) {
> +		fprintf(stderr, "%s:SKIP:no disk-backed O_TMPFILE in cwd\n", __func__);
> +		test__skip();
> +		goto out;
> +	}
> +
> +	/* A non-zero exit means the child failed to set up; the code says where. */
> +	if (WIFEXITED(status) && WEXITSTATUS(status)) {
> +		ASSERT_OK(WEXITSTATUS(status), "child setup");
> +		goto out;
> +	}
> +
> +	/*
> +	 * With the fix the arena fault-in reclaims the page cache and every
> +	 * fault succeeds, so the child exits 0. Without it the allocation
> +	 * cannot reclaim, fails once the headroom is used up, and the child
> +	 * dies with SIGSEGV on a valid arena address.
> +	 */

This isn't a bug, but since the Step 5 comment 25 lines above already
explains the fixed vs. unfixed outcome (fault-in reclaims and succeeds vs.
allocation cannot reclaim and dies on valid address), would it read better
to keep just one of the two explanations?

> +	if (!ASSERT_TRUE(WIFEXITED(status) && !WEXITSTATUS(status),
> +			 "child faulted the arena in")) {
> +		if (WIFSIGNALED(status))
> +			fprintf(stderr, "child killed by signal %d\n", WTERMSIG(status));
> +		dump_memcg();
> +	}

[ ... ]


---
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/35716612692