> 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
