Re: [PATCH v3 2/4] selftests/cgroup: add memcg_stat_cross_cpu correctness test for flush

[bot+bpf-ci@kernel.org]

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

> diff --git a/tools/testing/selftests/cgroup/config b/tools/testing/selftests/cgroup/config
> index 39f979690dd3..356b6a533d88 100644
> --- a/tools/testing/selftests/cgroup/config
> +++ b/tools/testing/selftests/cgroup/config
> @@ -4,3 +4,8 @@ CONFIG_CGROUP_FREEZER=y
>  CONFIG_CGROUP_SCHED=y
>  CONFIG_MEMCG=y
>  CONFIG_PAGE_COUNTER=y
> +CONFIG_BPF=y
> +CONFIG_BPF_SYSCALL=y
> +CONFIG_DEBUG_INFO=y
> +CONFIG_DEBUG_INFO_BTF=y
> +CONFIG_DEBUG_INFO_DWARF4=y

A subsystem pattern flags this as potentially concerning: CONFIG_BPF_EVENTS=y
is missing. The new program is SEC("iter.s/cgroup"), which is
BPF_PROG_TYPE_TRACING, and that program type is only registered inside
'#ifdef CONFIG_BPF_EVENTS' in include/linux/bpf_types.h:38. Without it
find_prog_type() returns -EINVAL and the skeleton load fails at runtime
even though the build succeeded.

CONFIG_BPF_EVENTS is 'default y' but has 'depends on (KPROBE_EVENTS ||
UPROBE_EVENTS) && PERF_EVENTS' (kernel/trace/Kconfig:853), so a kselftest
kernel configured only from the merged fragments can easily end up without
it. tools/testing/selftests/bpf/config:5 sets CONFIG_BPF_EVENTS=y
explicitly for exactly this reason.

Note also that CONFIG_DEBUG_INFO is not promptable (it is selected by the
DEBUG_INFO_* choice), so that line is a no-op.

> diff --git a/tools/testing/selftests/cgroup/Makefile b/tools/testing/selftests/cgroup/Makefile
> index e01584c2189a..01927511c059 100644
> --- a/tools/testing/selftests/cgroup/Makefile
> +++ b/tools/testing/selftests/cgroup/Makefile

[ ... ]

> @@ -33,3 +75,13 @@ $(OUTPUT)/test_kmem: $(LIBCGROUP_O)
>  $(OUTPUT)/test_memcontrol: $(LIBCGROUP_O)
>  $(OUTPUT)/test_pids: $(LIBCGROUP_O)
>  $(OUTPUT)/test_zswap: $(LIBCGROUP_O)
> +
> +ifneq ($(HAVE_BPF),)
> +# Links cgroup_util and the skeleton against the libbpf lib.bpf.mk built.  The
> +# uapi include is private to this binary: the other tests here must keep
> +# resolving <linux/*.h> against the system headers.
> +$(OUTPUT)/test_memcg_stat_cross_cpu: private CFLAGS += -I$(top_srcdir)/tools/include/uapi
> +$(OUTPUT)/test_memcg_stat_cross_cpu: test_memcg_stat_cross_cpu.c \
> +					$(BPF_SKELS) $(LIBCGROUP_O) $(BPFOBJ)
> +	$(call bpf_link,$@,$< $(LIBCGROUP_O))
> +endif

A subsystem pattern flags this as potentially concerning:
memcg_stat_cross_cpu.h is not listed as a prerequisite of the userspace
binary, and BPF_EXTRA_HDRS (documented by the new
tools/testing/selftests/lib.bpf.mk as 'more headers needed by the BPF
objects') is left unset, so the BPF object does not depend on it either.

That header defines struct memcg_stat_snapshot, which is the wire format
shared between the BPF program and the test binary. Editing it therefore
rebuilds neither side, and an incremental build can silently produce a
binary whose struct layout disagrees with the loaded skeleton.

The sibling conversion commit ef5ceab9d440 (selftests/hid) does set
BPF_EXTRA_HDRS for its shared header, so the mechanism was available.
LOCAL_HDRS would cover the C side.

> diff --git a/tools/testing/selftests/cgroup/memcg_stat_cross_cpu.bpf.c b/tools/testing/selftests/cgroup/memcg_stat_cross_cpu.bpf.c
> new file mode 100644
> index 000000000000..3c2acd49c16b
> --- /dev/null
> +++ b/tools/testing/selftests/cgroup/memcg_stat_cross_cpu.bpf.c

[ ... ]

> +/*
> + * Flush once at the subtree root, then read each cgroup through the kfuncs.
> + * iter.s because the flush kfunc may sleep.
> + */
> +SEC("iter.s/cgroup")
> +int cgroup_memcg_stat_cross_cpu(struct bpf_iter__cgroup *ctx)
> +{
> +	struct cgroup *cgrp = ctx->cgroup;
> +	struct memcg_stat_snapshot snap = {};
> +	struct cgroup_subsys_state *css;
> +	struct mem_cgroup *memcg;
> +	int idx_anon, idx_file, idx_shmem, idx_fmapped, idx_pgfault;
> +	__u64 cg_id;
> +
> +	/* The walk ends with a NULL element; return 0 so it runs to the end. */
> +	if (!cgrp)
> +		return 0;

A subsystem pattern flags this as potentially concerning: the comment
misstates the mechanism and will mislead the next reader. The NULL element
is the epilogue: cgroup_iter_seq_stop() invokes
__cgroup_iter_seq_show(seq, NULL, true) only after cgroup_unlock() and
only once the walk has already finished, and it sets p->visited_all = true
immediately afterwards.

__cgroup_iter_seq_show() converts a non-zero return into p->terminate =
true, but nothing reads p->terminate after that point, so returning 0
rather than non-zero for the NULL element has no effect on how far the
walk gets. The pre-existing comparable program
tools/testing/selftests/bpf/progs/cgroup_iter_memcg.c returns 1 for the
NULL element. The code is functionally correct; only the stated rationale
is wrong.

> diff --git a/tools/testing/selftests/cgroup/test_memcg_stat_cross_cpu.c b/tools/testing/selftests/cgroup/test_memcg_stat_cross_cpu.c
> new file mode 100644
> index 000000000000..9946c41c7a90
> --- /dev/null
> +++ b/tools/testing/selftests/cgroup/test_memcg_stat_cross_cpu.c

[ ... ]

> +static size_t leaf_charge(size_t want, int k)
> +{
> +	long online = sysconf(_SC_NPROCESSORS_ONLN);
> +	size_t floor;
> +
> +	floor = (size_t)FLUSH_MARGIN * MEMCG_CHARGE_BATCH * online *
> +		page_size / n_leaves;
> +	if (want < floor)
> +		want = floor;
> +	if (want < (size_t)k * page_size)
> +		want = (size_t)k * page_size;
> +
> +	return (want + page_size - 1) / page_size * page_size;
> +}

Does this handle the sysconf(_SC_NPROCESSORS_ONLN) failure case correctly?
The return value is not checked, and the expression it feeds is unsigned.
Because of the leading (size_t) cast, online is converted to size_t, so a
-1 return makes floor ~= (2^64 - 1048576)/n_leaves instead of a small byte
count.

want is then clamped up to that value and returned, and start_chargers()
passes it to charge_leaf(), where malloc(ca->bytes) fails, the charger
exits, and the test reports the misleading "%s never reached its charge"
failure. The same file already guards the analogous call: main() does
'page_size = sysconf(_SC_PAGESIZE); if (page_size <= 0) page_size =
BUF_SIZE;' (lines 947-949), and this is the only unchecked sysconf() in
the whole cgroup selftest directory.

> +static int start_chargers(int k, size_t bytes)
> +{
> +	struct charge_args ca = { .bytes = bytes, .k = k };
> +	long prev, cur;
> +	int i, retries;
> +
> +	for (i = 0; i < n_nodes; i++) {
> +		if (!nodes[i].is_leaf)
> +			continue;
> +		if (cg_run_nowait(nodes[i].path, charge_leaf, &ca) < 0) {
> +			ksft_print_msg("cannot start a charger on %s\n",
> +				       nodes[i].path);
> +			return -1;
> +		}
> +		ca.base += k;
> +	}

Can the per-leaf charge sizing in leaf_charge() cause memory pressure? The
per-leaf charge is raised to at least k * page_size, and run_case() sets k
= n_cpu for the 'cross_cpu' cases, so the total anon the test pins is
n_leaves * n_cpu * page_size with no ceiling. For the two large-tree cases
(fanout 10, depth 3) n_leaves is 1000, which is 1000 * n_cpu * 4K: 256 MB
on a 64-CPU host, 1 GB at 256 CPUs, 4 GB at 1024 CPUs, held simultaneously
by 1000 live processes.

The file's own header comment states the requirement this violates: "The
comparison is exact, which needs the subtree quiesced ... Global reclaim
would move the numbers and the test would report a mismatch", and
check_tree() runs with STAT_TOLERANCE_PCT 0, so any reclaim during the run
turns into a hard KSFT_FAIL rather than a skip. Nothing in the test
compares the computed footprint against MemAvailable or caps it.

The same sizing also feeds the charge-completion deadline in
start_chargers(): CHARGE_WAIT_RETRIES (100) * DEFAULT_WAIT_INTERVAL_US/10
(10 ms) gives each leaf only ~1 s to both reach bytes and read the same
memory.current twice in a row, while up to 1000 chargers are concurrently
performing k sched_setaffinity() migrations each.

> +	for (i = 0; i < n_nodes; i++) {
> +		if (!nodes[i].is_leaf)
> +			continue;
> +		/*
> +		 * Wait for the charge to both cover the region and stop
> +		 * moving.
> +		 */
> +		prev = -1;
> +		for (retries = CHARGE_WAIT_RETRIES; retries; retries--) {
> +			cur = cg_read_long(nodes[i].path, "memory.current");
> +			if (cur >= (long)bytes && cur == prev)
> +				break;
> +			prev = cur;
> +			usleep(DEFAULT_WAIT_INTERVAL_US / 10);
> +		}
> +		if (!retries) {
> +			ksft_print_msg("%s never reached its charge\n",
> +				       nodes[i].path);
> +			return -1;
> +		}
> +	}
> +	return 0;
> +}

A subsystem pattern flags this as potentially concerning: start_chargers()
open-codes a retry/poll loop over a cgroup control file instead of using
(or extending) the cgroup selftest library's polling helpers.

lib/include/cgroup_util.h already exports cg_read_key_long_poll(cgroup,
control, key, expected, retries, wait_interval_us),
cg_wait_for()/cg_prepare_for_wait() and cg_wait_for_proc_count(), and this
loop also silently treats cg_read_long()'s -1 error return as "not charged
yet" and burns a retry on it, which the shared helper does not do (it
returns the error).

The existing helper is not a drop-in substitute: cg_read_key_long_poll()
polls a memory.stat key for exact equality against expected and returns on
the first read error, whereas this loop needs 'memory.current >= bytes AND
unchanged across two reads' on a different file. So the semantics genuinely
differ and a bespoke loop is defensible; the guide's underlying point still
applies in that this series already extends the shared library
(cg_get_id/cg_touch_pages/cg_alloc_anon), so a 'wait until >= X and stable'
helper could live there too and pick up the missing error handling for
free.

> +/*
> + * Collect what the kfuncs report.  The program flushes at the first cgroup it
> + * sees, the subtree root in DESCENDANTS_PRE order.
> + */
> +static int read_bpf(int root_fd)
> +{
> +	DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
> +	struct memcg_stat_cross_cpu *skel = NULL;
> +	union bpf_iter_link_info linfo = {};
> +	struct bpf_link *link = NULL;
> +	int ret = -1, i, mfd, fd;
> +	char buf[4096];
> +	ssize_t r;
> +
> +	skel = memcg_stat_cross_cpu__open();
> +	if (!skel) {
> +		ksft_print_msg("skel open failed\n");
> +		return -1;
> +	}
> +	if (bpf_map__set_max_entries(skel->maps.results, n_nodes + 8)) {
> +		ksft_print_msg("set max_entries failed\n");
> +		goto out;
> +	}
> +	if (memcg_stat_cross_cpu__load(skel)) {
> +		ksft_print_msg("skel load failed\n");
> +		goto out;
> +	}
> +
> +	linfo.cgroup.cgroup_fd = root_fd;
> +	linfo.cgroup.order = BPF_CGROUP_ITER_DESCENDANTS_PRE;
> +	opts.link_info = &linfo;
> +	opts.link_info_len = sizeof(linfo);
> +
> +	link = bpf_program__attach_iter(skel->progs.cgroup_memcg_stat_cross_cpu,
> +					&opts);
> +	if (!link) {
> +		ksft_print_msg("attach iter failed\n");
> +		goto out;
> +	}
> +
> +	fd = bpf_iter_create(bpf_link__fd(link));
> +	if (fd < 0) {
> +		ksft_print_msg("bpf_iter_create failed\n");
> +		goto out;
> +	}
> +	while ((r = read(fd, buf, sizeof(buf))) > 0)
> +		;
> +	close(fd);
> +	if (r) {
> +		ksft_print_msg("bpf walk failed\n");
> +		goto out;
> +	}

A subsystem pattern flags this as potentially concerning: every BPF
capability shortfall in read_bpf() is reported as a test FAILURE rather
than a SKIP.

The feature gate is narrow. memcg_kfuncs_available() (line 907) probes
exactly one symbol:

  ok = btf__find_by_name_kind(btf, "bpf_get_mem_cgroup", BTF_KIND_FUNC) > 0;

but memcg_stat_cross_cpu.bpf.c additionally requires bpf_put_mem_cgroup,
bpf_mem_cgroup_flush_stats, bpf_mem_cgroup_page_state and
bpf_mem_cgroup_vm_events, plus kernel support for a sleepable cgroup
iterator (SEC("iter.s/cgroup")). On a kernel that exports
bpf_get_mem_cgroup but not the rest of the set (a partial backport, or a
future kernel that renames/retires one of them),
memcg_stat_cross_cpu__load() fails with a capability-absent errno (-ENOENT
for an unresolvable __ksym kfunc, -EOPNOTSUPP/-EINVAL for an unsupported
sleepable iter). The same applies to bpf_program__attach_iter() and
bpf_iter_create(), which also fail with -EOPNOTSUPP/-EPERM when the
capability or privilege is absent.

All three paths take 'goto out' and return -1. run_case() initialises 'ret
= KSFT_FAIL' and only ever upgrades it to KSFT_PASS, so it has no
KSFT_SKIP or ksft_test_result_skip() path at all. The result is a hard
"not ok" in the TAP output for a machine that simply lacks the
prerequisite. The diagnostics also make this indistinguishable from a
genuine failure: none of the four messages ("skel load failed", "attach
iter failed", "bpf_iter_create failed", "bpf walk failed") carry errno or
strerror(errno), so a CI operator cannot tell a missing-capability
environment from a real flush regression.

> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index 0ebf796f3cff..15ba46879504 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c

[ ... ]


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