Re: [PATCH v2 1/2] cgroup: add BPF kfuncs to read a cpu cgroup's stats

>     cgroup: add BPF kfuncs to read a cpu cgroup's stats
>
>     Collecting cgroup statistics is expensive: the existing
>     method is to open and parse a cgroup file. memcg already has an
>     efficient alternative through BPF; this series extends that idea to cpu.
>
>     Expose the CPU controller's per-cgroup statistics to BPF, following
>     the memory controller kfuncs in mm/bpf_memcontrol.c.
>
>     Design:
>     - Add bpf_css_flush_rstat() and bpf_cgroup_base_stat() to cgroup rstat.
>       The second kfunc returns the raw cgroup_base_stat after the
>       same cputime adjustment used by cpu.stat.
>     - Leave reading the CFS bandwidth counters to the BPF program. They are
>       plain fields of tg->cfs_bandwidth, so they need no kernel code.
>     - Add bpf_css_to_task_group(), which returns a checked RCU pointer of
>       the task_group*. This is usefule to compute the throttled time in bpf
>       side since the bpf_per_cpu_ptr() requires a verifier-known struct
>       task_group * and a pointer carrying the MEM_PERCPU property.
>       This use the convention that task_group embeds its css at offset zero,
>       so no scheduler helper or scheduler source change is needed.

This isn't a bug, but there are language errors in the third Design
bullet: "usefule" should be "useful", and "This use" should be "This
uses". Also "in bpf side" is typically written as "on the BPF side" in
kernel documentation.

> diff --git a/kernel/cgroup/Makefile b/kernel/cgroup/Makefile
> index ede31601a363..0ba59b7eef48 100644
> --- a/kernel/cgroup/Makefile
> +++ b/kernel/cgroup/Makefile
> @@ -1,6 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-y := cgroup.o rstat.o namespace.o cgroup-v1.o freezer.o
>
> +obj-$(CONFIG_BPF_SYSCALL) += bpf_cpu.o
> +
>  obj-$(CONFIG_CGROUP_FREEZER) += legacy_freezer.o
>  obj-$(CONFIG_CGROUP_PIDS) += pids.o
>  obj-$(CONFIG_CGROUP_RDMA) += rdma.o

[ ... ]

> diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
> index de816a43db9f..46c322c4858b 100644
> --- a/kernel/cgroup/rstat.c
> +++ b/kernel/cgroup/rstat.c
> @@ -752,6 +752,54 @@ void cgroup_base_stat_cputime_show(struct seq_file *seq)
>  	cgroup_force_idle_show(seq, &bstat);
>  }
>
> +#ifdef CONFIG_BPF_SYSCALL
> +
> +__bpf_kfunc_start_defs();
> +
> +/**
> + * bpf_css_flush_rstat - Flush a cgroup subsystem's rstat data
> + * @css: cgroup subsystem state to flush
> + */
> +__bpf_kfunc void bpf_css_flush_rstat(struct cgroup_subsys_state *css)
> +{
> +	css_rstat_flush(css);
> +}

This isn't a bug, but is bpf_css_flush_rstat() needed?

css_rstat_flush() is already marked __bpf_kfunc and registered with
KF_SLEEPABLE at rstat.c:409 and :806. Adding
'BTF_ID_FLAGS(func, css_rstat_flush, KF_SLEEPABLE)' to
bpf_rstat_common_kfunc_ids below would expose the same operation to BPF
without introducing a second name for it and without needing a wrapper.

> +
> +/**
> + * bpf_cgroup_base_stat - Read a cgroup's base statistics
> + * @cgrp: cgroup to read from
> + * @out: zero-initialized output in nanoseconds
> + *
> + * CPU time is adjusted as for cpu.stat.
> + */
> +__bpf_kfunc void bpf_cgroup_base_stat(struct cgroup *cgrp,
> +				      struct cgroup_base_stat *out)
> +{
> +	if (cgroup_parent(cgrp)) {
> +		__css_rstat_lock(&cgrp->self, -1);
> +		*out = cgrp->bstat;
> +		cputime_adjust(&cgrp->bstat.cputime, &cgrp->prev_cputime,
> +			       &out->cputime.utime, &out->cputime.stime);
> +		__css_rstat_unlock(&cgrp->self, -1);
> +	} else {
> +		root_cgroup_cputime(out);
> +	}
> +}
> +
> +__bpf_kfunc_end_defs();
> +
> +BTF_KFUNCS_START(bpf_rstat_common_kfunc_ids)
> +BTF_ID_FLAGS(func, bpf_css_flush_rstat, KF_SLEEPABLE)
> +BTF_ID_FLAGS(func, bpf_cgroup_base_stat, KF_SLEEPABLE)
> +BTF_KFUNCS_END(bpf_rstat_common_kfunc_ids)
> +
> +static const struct btf_kfunc_id_set bpf_rstat_common_kfunc_set = {
> +	.owner		= THIS_MODULE,
> +	.set		= &bpf_rstat_common_kfunc_ids,
> +};
> +
> +#endif /* CONFIG_BPF_SYSCALL */
> +

[ ... ]

>  static int __init bpf_rstat_kfunc_init(void)
>  {
> -	return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
> -					 &bpf_rstat_kfunc_set);
> +	int ret;
> +
> +	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
> +					&bpf_rstat_kfunc_set);
> +#ifdef CONFIG_BPF_SYSCALL
> +	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_UNSPEC,
> +					       &bpf_rstat_common_kfunc_set);
> +#endif
> +	return ret;
>  }
>  late_initcall(bpf_rstat_kfunc_init);

This isn't a bug, but are these #ifdef CONFIG_BPF_SYSCALL guards
necessary?

The existing kfuncs in this file (css_rstat_flush at line 409,
BTF_KFUNCS_START at 804, register_btf_kfunc_id_set at 818) have no
guards, and rstat.o is obj-y, so they compile fine with
CONFIG_BPF_SYSCALL=n via the no-op stubs in include/linux/btf.h:652.
The cited precedent mm/bpf_memcontrol.c uses 'obj-$(CONFIG_MEMCG) +=
bpf_memcontrol.o' with no preprocessor conditionals anywhere in the .c
file. Documentation/process/coding-style.rst:1166-1175 asks to avoid
preprocessor conditionals in .c files and prefer compiling out entire
functions via header stubs. Would dropping the guards and using
'obj-$(CONFIG_CGROUP_SCHED) += bpf_cpu.o' in the Makefile work here?


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