Re: [PATCH v2 2/4] bpf: add bpf_cgroup_{flush_stats,stall} function

[bot+bpf-ci@kernel.org]

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

> diff --git a/include/linux/psi.h b/include/linux/psi.h
> index 7b4fd8190810..243dcf97bea4 100644
> --- a/include/linux/psi.h
> +++ b/include/linux/psi.h
> @@ -52,6 +52,10 @@ static inline void psi_memstall_enter(unsigned long *flags) {}
>  static inline void psi_memstall_leave(unsigned long *flags) {}
>
>  #ifdef CONFIG_CGROUPS
> +static inline struct psi_group *cgroup_psi(struct cgroup *cgrp)
> +{
> +	return NULL;
> +}
>  static inline int psi_cgroup_alloc(struct cgroup *cgrp)
>  {
>  	return 0;

[ ... ]

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index baa12b24bb64..5562a6c9d2f9 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -29,6 +29,7 @@
>  #include <linux/task_work.h>
>  #include <linux/irq_work.h>
>  #include <linux/buildid.h>
> +#include <linux/psi.h>
>
>  #include "../../lib/kstrtox.h"
>

[ ... ]

> @@ -2881,6 +2882,37 @@ bpf_task_get_cgroup1(struct task_struct *task, int hierarchy_id)
>  		return NULL;
>  	return cgrp;
>  }
> +
> +/**
> + * bpf_cgroup_stall - acquire the total stall time of cgroup
> + * @cgrp: cgroup struct
> + * @states: psi states
> + *
> + * Return the total stall time.
> + */
> +__bpf_kfunc u64 bpf_cgroup_stall(struct cgroup *cgrp, enum psi_states states)
> +{
> +	struct psi_group *group = cgroup_psi(cgrp);
> +
> +	if (unlikely(!group || (u32)states >= NR_PSI_STATES - 1))
> +		return (u64)-1;
> +
> +	return div_u64(group->total[PSI_AVGS][states], NSEC_PER_MSEC);
> +}
> +
> +/**
> + * bpf_cgroup_flush_stats - Flush cgroup's statistics
> + * @cgrp: cgroup struct
> + */
> +__bpf_kfunc void bpf_cgroup_flush_stats(struct cgroup *cgrp)
> +{
> +	struct psi_group *group = cgroup_psi(cgrp);
> +
> +	if (unlikely(!group))
> +		return;
> +
> +	psi_group_flush_stats(group);
> +}
>  #endif /* CONFIG_CGROUPS */

Will these functions build when CONFIG_BPF_SYSCALL=y, CONFIG_CGROUPS=y,
but CONFIG_PSI=n?

The functions reference several PSI-only symbols:

In bpf_cgroup_stall():
- enum psi_states (parameter type) only exists in include/linux/psi_types.h
  when CONFIG_PSI=y
- NR_PSI_STATES and PSI_AVGS are enum members only defined when CONFIG_PSI=y
- struct psi_group's total field doesn't exist when CONFIG_PSI=n (psi_types.h
  defines an empty struct in that case)

In bpf_cgroup_flush_stats():
- psi_group_flush_stats() is only declared in include/linux/psi.h when
  CONFIG_PSI=y with no stub for the !CONFIG_PSI case

kernel/bpf/helpers.o is compiled whenever CONFIG_BPF_SYSCALL=y. Since PSI
is an independent config option, the combination CONFIG_BPF_SYSCALL=y &&
CONFIG_CGROUPS=y && CONFIG_PSI=n is valid and would hit undefined symbols.

The cgroup_psi() stub added in include/linux/psi.h only handles the
!CONFIG_PSI case for cgroup_psi() itself, but doesn't solve the unresolved
PSI types and functions. Should these kfuncs be guarded with CONFIG_PSI,
similar to how kernel/cgroup/cgroup.c handles cgroup_pressure_show()?

>
>  /**
> @@ -4734,6 +4766,8 @@ BTF_ID_FLAGS(func, bpf_cgroup_ancestor, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
>  BTF_ID_FLAGS(func, bpf_cgroup_from_id, KF_ACQUIRE | KF_RET_NULL)
>  BTF_ID_FLAGS(func, bpf_task_under_cgroup, KF_RCU)
>  BTF_ID_FLAGS(func, bpf_task_get_cgroup1, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
> +BTF_ID_FLAGS(func, bpf_cgroup_stall)
> +BTF_ID_FLAGS(func, bpf_cgroup_flush_stats, KF_SLEEPABLE)
>  #endif

These BTF_ID_FLAGS entries would also need CONFIG_PSI guards if the function
definitions are moved inside CONFIG_PSI.


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