Re: [PATCH v2 2/2] selftests/bpf: add cgroup_iter_cpu test for cpu cgroup kfuncs

[bot+bpf-ci@kernel.org]

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

> commit 0dbd10c9717fb724b55b28346bbbbe9e5e570bc6
> Author: Ziyang Men <ziyang.meme@gmail.com>
> Date:   Mon Aug 17 17:24:50 2026 -0700
>
>     selftests/bpf: add cgroup_iter_cpu test for cpu cgroup kfuncs
>
>     Add cgroup_iter_cpu, a selftest for the CPU controller BPF kfuncs.
>
>     The userspace side runs a CPU hog in a test cgroup with cpu.max settled
>     then:
>
>       - checks the CPU-time and throttling counters are nonzero,
>       - compares whether all values the program read are same as those
>         reading from cgroup file.
>
>     Enable CONFIG_CGROUP_SCHED, CONFIG_FAIR_GROUP_SCHED and
>     CONFIG_CFS_BANDWIDTH in the selftest config.
>
>     Tested on VM with v7.2-rc5.
>
>     Suggested-by: Shakeel Butt <shakeel.butt@linux.dev>
>     Assisted-by: Claude:claude-opus-5
>     Signed-off-by: Ziyang Men <ziyang.meme@gmail.com>

This isn't a bug, but could the changelog body be split into paragraphs
so it's clearer what state the cgroup is put in and which values are
compared?

The sentence "with cpu.max settled" doesn't say what quota is written
(the test writes "10000 100000"), and "compares whether all values the
program read are same as those reading from cgroup file" is missing
words that would indicate which side is being read from where.

Rewording might help, something like: "runs a CPU hog in a test cgroup
with a cpu.max quota set, then checks that the CPU-time and throttling
counters are nonzero and that every value the BPF program reports
matches the corresponding value read from cpu.stat and cpu.stat.local."

> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_iter_cpu.c b/tools/testing/selftests/bpf/prog_tests/cgroup_iter_cpu.c
> new file mode 100644
> index 000000000000..cd7e92ababfb

[ ... ]

> +/* Read cgroup file @name into @buf. */
> +static int read_cgroup_file(int cgroup_fd, const char *name, char *buf,
> +			    size_t size)
> +{
> +	ssize_t n;
> +	int fd;
> +
> +	fd = openat(cgroup_fd, name, O_RDONLY);
> +	if (fd < 0)
> +		return -1;
> +	n = read(fd, buf, size - 1);
> +	close(fd);
> +	if (n <= 0)
> +		return -1;
> +	buf[n] = '\0';
> +	return 0;
> +}

This isn't a bug, but since cgroup_helpers.h already has
write_cgroup_file() but no reader, would read_cgroup_file() be better
placed next to it in cgroup_helpers.c so other tests can use it?

It's a generic "read any cgroup file into a buffer" helper with nothing
cpu-specific in it.

> diff --git a/tools/testing/selftests/bpf/progs/cgroup_iter_cpu.c b/tools/testing/selftests/bpf/progs/cgroup_iter_cpu.c
> new file mode 100644
> index 000000000000..22b9bb62d910

[ ... ]

> +	if (tg && bpf_core_field_exists(tg->cfs_rq) &&
> +	    bpf_core_field_exists(struct cfs_rq, throttled_clock_self_time)) {
> +		__u32 mask_bytes = bpf_core_type_size(struct cpumask);
> +		__u32 full_words = mask_bytes / sizeof(__u64);
> +		int *cpu;
> +
> +		if (full_words)
> +			bpf_for_each(bits, cpu,
> +				     (const __u64 *)&__cpu_possible_mask,
> +				     full_words)
> +				throttled_self += read_throttled_self(tg, *cpu);
> +
> +		if (mask_bytes & (sizeof(__u64) - 1)) {
> +			__u32 tail = 0;
> +			const void *src = (const char *)&__cpu_possible_mask +
> +					  full_words * sizeof(__u64);
> +			int bit;
> +
> +			if (!bpf_probe_read_kernel(&tail, sizeof(tail), src))
> +				bpf_for(bit, 0, 32)
> +					if (tail & (1U << bit)) {
> +						__u32 tail_cpu = full_words * 64 + bit;
> +
> +						throttled_self +=
> +							read_throttled_self(tg, tail_cpu);
> +					}
> +		}
> +	}

This isn't a bug, but since struct cpumask is an array of unsigned long,
sizeof() is always a multiple of 8 on 64-bit, so the `mask_bytes &
(sizeof(__u64) - 1)` tail path never fires there - is 32-bit coverage
the intent here, and if so would a comment saying so help?

The `if (full_words)` guard also looks droppable given
bpf_for_each(bits, ..., 0) is already a valid no-op
(verifier_bits_iter.c:zero_words).


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