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

[bot+bpf-ci@kernel.org]

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

> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -923,4 +923,19 @@ static inline void cgroup_bpf_put(struct cgroup *cgrp) {}
>
>  struct cgroup_of_peak *of_peak(struct kernfs_open_file *of);
>
> +/* A cgroup's base CPU-time counters in microseconds, as cpu.stat prints them */
> +struct cpu_cgroup_cputime {
> +	u64 usage_usec;
> +	u64 user_usec;
> +	u64 system_usec;
> +	u64 nice_usec;
> +	u64 forceidle_usec;	/* 0 without CONFIG_SCHED_CORE */
> +};
> +
> +/* A task_group's own throttled time in nanoseconds; see cpu.stat.local */
> +struct task_group;
> +#ifdef CONFIG_CFS_BANDWIDTH
> +u64 throttled_time_self(struct task_group *tg);
> +#endif
> +
>  #endif /* _LINUX_CGROUP_H */

Should this scheduler interface be in include/linux/cgroup.h? The header
now forward-declares struct task_group and declares a CFS-bandwidth
function, although task_group and its accessors live in
kernel/sched/sched.h.

Nothing else in include/linux/cgroup.h references scheduler-internal
types. The only consumer is kernel/cgroup/bpf_cpu.c, which could get it
from kernel/cgroup/cgroup-internal.h instead (where the patch already
puts bpf_cpu_cgroup_cputime()).

Also, throttled_time_self() becomes a global, unprefixed symbol when it
was previously file-local to kernel/sched/core.c. Would a name like
sched_cgroup_throttled_time_self() avoid polluting the global namespace,
or could a small prefixed wrapper keep the original function static?

> diff --git a/kernel/cgroup/Makefile b/kernel/cgroup/Makefile
> --- a/kernel/cgroup/Makefile
> +++ b/kernel/cgroup/Makefile

[ ... ]

> diff --git a/kernel/cgroup/bpf_cpu.c b/kernel/cgroup/bpf_cpu.c
> --- /dev/null
> +++ b/kernel/cgroup/bpf_cpu.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CPU Controller-related BPF kfuncs
> + *
> + * bpf_cpu_cgroup_cputime() is defined in rstat.c, which owns the locking it
> + * needs, and only registered here.
> + *
> + * Author: Ziyang Men <ziyang.meme@gmail.com>
> + */
> +
> +#include <linux/bpf.h>
> +#include <linux/btf_ids.h>
> +#include <linux/cgroup.h>
> +
> +#include "cgroup-internal.h"
> +
> +__bpf_kfunc_start_defs();

[ ... ]

> +/**
> + * bpf_cpu_cgroup_throttled_self - Read a cgroup's own throttled time
> + * @cgrp: cgroup to read from
> + *
> + * Return: The throttled time in microseconds, or 0 if config is off.
> + */
> +__bpf_kfunc u64 bpf_cpu_cgroup_throttled_self(struct cgroup *cgrp)
> +{
> +/* cpu_cgrp_id needs the cpu controller, which CFS bandwidth depends on */
> +#ifdef CONFIG_CFS_BANDWIDTH
> +	struct cgroup_subsys_state *css;
> +
> +	guard(rcu)();
> +
> +	css = rcu_dereference(cgrp->subsys[cpu_cgrp_id]);
> +	if (!css)
> +		return 0;
> +
> +	return div_u64(throttled_time_self((struct task_group *)css),
> +		       NSEC_PER_USEC);

Is the raw cast to struct task_group * safe here? This works because
struct cgroup_subsys_state css is the first member of struct task_group,
but struct task_group is an incomplete type in this translation unit.

kernel/cgroup/bpf_cpu.c includes only <linux/bpf.h>, <linux/btf.h>,
<linux/cgroup.h> and "cgroup-internal.h", so the compiler cannot verify
the cast.

Every existing css->task_group conversion in the tree uses css_tg() from
kernel/sched/sched.h, which is container_of() and stays correct if the
member ever moves. The scheduler helpers cpu_cfs_local_stat_show() and
cpu_local_stat_show() do the same conversion with css_tg().

Would moving the cpu-controller kfunc to kernel/sched/core.c (where
struct task_group is complete and css_tg() is available) avoid hard-
coding a struct-layout invariant that this file cannot check?

> +#else
> +	return 0;
> +#endif
> +}

[ ... ]

> diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c

[ ... ]

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -10027,7 +10027,7 @@ static int cpu_cfs_stat_show(struct seq_file *sf, void *v)
>  	return 0;
>  }
>
> -static u64 throttled_time_self(struct task_group *tg)
> +u64 throttled_time_self(struct task_group *tg)
>  {
>  	int i;
>  	u64 total = 0;


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