Re: [PATCH bpf-next v4 2/2] selftests/bpf: add memcg async reclaim test

[bot+bpf-ci]

> diff --git tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
> new file mode 100644
> index 000000000000..ece7bceaab3c
> --- /dev/null
> +++ tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
> @@ -0,0 +1,480 @@

[ ... ]

> +/*
> + * The workload files must sit on a regular filesystem: with swap
> + * disabled for the cgroup, tmpfs/ramfs pages are unevictable and would
> + * OOM the cgroup instead of exercising reclaim; they are also charged
> + * as anonymous memory, so they never raise the WORKINGSET_REFAULT_FILE
> + * events the BPF program monitors. Fall back to the current directory
> + * when /tmp is backed by such a filesystem.
> + */

The comment states that tmpfs/ramfs pages "are also charged as anonymous
memory." Is this accurate?

Looking at the page cache charge path (mm/filemap.c __filemap_add_folio()),
tmpfs/shmem folios go through folio_memcg_charge() and land in
NR_FILE_PAGES/NR_SHMEM, which shows up in memory.stat's file/shmem
counters, not as anonymous memory.

The reason they don't trigger WORKINGSET_REFAULT_FILE is different:
shmem folios are swap-backed, so folio_is_file_lru() returns false
(include/linux/mm_inline.h), and workingset_refault() accounts them at
WORKINGSET_REFAULT_ANON instead (mm/workingset.c).

The conclusion the comment draws is correct, but the stated reason is
not. For a patch aimed at the mm/memcg audience, could the wording be
adjusted to reflect that they sit on the anon LRU because they are
swap-backed?

> +static const char *workload_files_dir(void)
> +{
> +	struct statfs st;
> +
> +	if (!statfs("/tmp", &st) &&
> +	    (st.f_type == TMPFS_MAGIC || st.f_type == RAMFS_MAGIC))
> +		return ".";
> +	return "/tmp";
> +}

[ ... ]

> +static int setup_high_low_cgroups(u64 *high_cgroup_id, u64 *low_cgroup_id)
> +{
> +	int ret;
> +	char limit_buf[20];
> +
> +	ret = setup_cgroup_environment();
> +	if (!ASSERT_OK(ret, "setup_cgroup_environment"))
> +		goto cleanup;
> +
> +	ret = create_and_get_cgroup(CG_DIR);
> +	if (!ASSERT_GE(ret, 0, "create_and_get_cgroup " CG_DIR))
> +		goto cleanup;
> +	close(ret);
> +
> +	ret = enable_controllers(CG_DIR, "memory");
> +	if (!ASSERT_OK(ret, "enable_controllers"))
> +		goto cleanup;

A subsystem pattern flags this as potentially concerning: the test
treats an absent memory controller as a test failure rather than a skip.

enable_controllers() ends up in __enable_controllers() which writes to
cgroup.subtree_control and returns 1 when that write fails. On a kernel
built with CONFIG_MEMCG=n or booted with cgroup_disable=memory, the
memory controller does not exist, the write is rejected, and the test
reports FAIL.

The feature being tested is CONFIG_MEMCG-gated: mm/Makefile builds
bpf_memcontrol.o as obj-$(CONFIG_MEMCG), so on such a kernel there is
nothing to test. Note that 123 other prog_tests use test__skip() with
a reason string for missing capabilities.

However, CONFIG_MEMCG=y is present in every per-arch BPF selftest config
(config.x86_64, config.aarch64, config.s390x, etc.), so BPF CI will
never hit this path. It is only reachable with a hand-rolled kernel
config that omits it. Given this context, is the ASSERT_OK() treatment
acceptable?

[ ... ]

> +void test_memcg_wq_async_reclaim(void)
> +{
> +	u64 high_cgroup_id, low_cgroup_id;
> +	int err;
> +	double high_time = 0.0, low_time = 0.0;
> +	struct memcg_async_reclaim *skel = NULL;
> +
> +	err = setup_high_low_cgroups(&high_cgroup_id, &low_cgroup_id);
> +	if (!ASSERT_OK(err, "setup_high_low_cgroups reclaim"))
> +		return;
> +
> +	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;
> +
> +	if (high_time >= low_time)
> +		PRINT_FAIL("high cgroup not improved with async reclaim: high_time=%f low_time=%f",
> +			   high_time, low_time);

Does the zero-margin wall-clock comparison make the test brittle?

The test's final pass/fail criterion is a single-run timing comparison
between two symmetric workloads with no baseline. PRINT_FAIL() expands to
test__fail(), so this marks the whole test FAILED whenever high_time >=
low_time, including on a tie or a 1-microsecond inversion.

The two children are symmetric by construction: run_high_low_workload()
forks them within microseconds of each other and both run the identical
real_test_child_work() body (write a 32 MB file, then read-fault it 50
times) inside sibling cgroups under one shared memory.max of 32 MB.

Which one finishes first is decided by page-reclaim ordering, disk service
order, CPU scheduling, and host load. The test pins only memory.max and
memory.swap.max but does not pin the reclaim implementation (classic LRU
vs MGLRU, selected at runtime), the dirty-ratio sysctls, the CPU count, or
the backing device.

On a machine where the BPF-driven reclaim of the low cgroup does not
translate into a measurable ordering advantage, a fully correct kernel
reports FAIL.

The comment just above acknowledges that the timing signal is weak. The two
ASSERT_GT checks on reclaim_calls and reclaimed_bytes are deterministic and
verify that bpf_proactive_reclaim() actually ran. The timing comparison
adds no deterministic coverage on top of them, only a nondeterministic
failure mode.

For comparison, prog_tests/test_overhead.c is the only other prog_test that
uses clock_gettime(CLOCK_MONOTONIC, ...), and it only prints its
measurements. Could the comparison either require a substantial margin, be
bounded with retry logic, or report the ratio informationally with
fprintf(stdout, ...) instead of PRINT_FAIL()?


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