| Message ID | 20220610194435.2268290-7-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: rstat: cgroup hierarchical stats | expand |
Hi Yosry,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Yosry-Ahmed/bpf-rstat-cgroup-hierarchical-stats/20220611-034720
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20220611/202206110457.uD5lLvbh-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/83f297e2b47dc41b511f071b9eadf38339387b41
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yosry-Ahmed/bpf-rstat-cgroup-hierarchical-stats/20220611-034720
git checkout 83f297e2b47dc41b511f071b9eadf38339387b41
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash kernel/cgroup/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> kernel/cgroup/rstat.c:161:22: warning: no previous prototype for 'bpf_rstat_flush' [-Wmissing-prototypes]
161 | __weak noinline void bpf_rstat_flush(struct cgroup *cgrp,
| ^~~~~~~~~~~~~~~
kernel/cgroup/rstat.c:509:10: error: 'const struct btf_kfunc_id_set' has no member named 'sleepable_set'; did you mean 'release_set'?
509 | .sleepable_set = &bpf_rstat_sleepable_kfunc_ids,
| ^~~~~~~~~~~~~
| release_set
>> kernel/cgroup/rstat.c:509:27: warning: excess elements in struct initializer
509 | .sleepable_set = &bpf_rstat_sleepable_kfunc_ids,
| ^
kernel/cgroup/rstat.c:509:27: note: (near initialization for 'bpf_rstat_kfunc_set')
vim +/bpf_rstat_flush +161 kernel/cgroup/rstat.c
148
149 /*
150 * A hook for bpf stat collectors to attach to and flush their stats.
151 * Together with providing bpf kfuncs for cgroup_rstat_updated() and
152 * cgroup_rstat_flush(), this enables a complete workflow where bpf progs that
153 * collect cgroup stats can integrate with rstat for efficient flushing.
154 *
155 * A static noinline declaration here could cause the compiler to optimize away
156 * the function. A global noinline declaration will keep the definition, but may
157 * optimize away the callsite. Therefore, __weak is needed to ensure that the
158 * call is still emitted, by telling the compiler that we don't know what the
159 * function might eventually be.
160 */
> 161 __weak noinline void bpf_rstat_flush(struct cgroup *cgrp,
162 struct cgroup *parent, int cpu)
163 {
164 }
165
Hi Yosry,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Yosry-Ahmed/bpf-rstat-cgroup-hierarchical-stats/20220611-034720
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20220611/202206110544.D5cTU0WQ-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/83f297e2b47dc41b511f071b9eadf38339387b41
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yosry-Ahmed/bpf-rstat-cgroup-hierarchical-stats/20220611-034720
git checkout 83f297e2b47dc41b511f071b9eadf38339387b41
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
kernel/cgroup/rstat.c:161:22: warning: no previous prototype for 'bpf_rstat_flush' [-Wmissing-prototypes]
161 | __weak noinline void bpf_rstat_flush(struct cgroup *cgrp,
| ^~~~~~~~~~~~~~~
>> kernel/cgroup/rstat.c:509:10: error: 'const struct btf_kfunc_id_set' has no member named 'sleepable_set'; did you mean 'release_set'?
509 | .sleepable_set = &bpf_rstat_sleepable_kfunc_ids,
| ^~~~~~~~~~~~~
| release_set
kernel/cgroup/rstat.c:509:27: warning: excess elements in struct initializer
509 | .sleepable_set = &bpf_rstat_sleepable_kfunc_ids,
| ^
kernel/cgroup/rstat.c:509:27: note: (near initialization for 'bpf_rstat_kfunc_set')
vim +509 kernel/cgroup/rstat.c
505
506 static const struct btf_kfunc_id_set bpf_rstat_kfunc_set = {
507 .owner = THIS_MODULE,
508 .check_set = &bpf_rstat_check_kfunc_ids,
> 509 .sleepable_set = &bpf_rstat_sleepable_kfunc_ids,
510 };
511
On Fri, Jun 10, 2022 at 2:23 PM kernel test robot <lkp@intel.com> wrote: > > Hi Yosry, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on bpf-next/master] > > url: https://github.com/intel-lab-lkp/linux/commits/Yosry-Ahmed/bpf-rstat-cgroup-hierarchical-stats/20220611-034720 > base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master > config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20220611/202206110544.D5cTU0WQ-lkp@intel.com/config) > compiler: gcc-11 (Debian 11.3.0-3) 11.3.0 > reproduce (this is a W=1 build): > # https://github.com/intel-lab-lkp/linux/commit/83f297e2b47dc41b511f071b9eadf38339387b41 > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Yosry-Ahmed/bpf-rstat-cgroup-hierarchical-stats/20220611-034720 > git checkout 83f297e2b47dc41b511f071b9eadf38339387b41 > # save the config file > mkdir build_dir && cp config build_dir/.config > make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash > > If you fix the issue, kindly add following tag where applicable > Reported-by: kernel test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > kernel/cgroup/rstat.c:161:22: warning: no previous prototype for 'bpf_rstat_flush' [-Wmissing-prototypes] > 161 | __weak noinline void bpf_rstat_flush(struct cgroup *cgrp, > | ^~~~~~~~~~~~~~~ > >> kernel/cgroup/rstat.c:509:10: error: 'const struct btf_kfunc_id_set' has no member named 'sleepable_set'; did you mean 'release_set'? > 509 | .sleepable_set = &bpf_rstat_sleepable_kfunc_ids, > | ^~~~~~~~~~~~~ > | release_set > kernel/cgroup/rstat.c:509:27: warning: excess elements in struct initializer > 509 | .sleepable_set = &bpf_rstat_sleepable_kfunc_ids, > | ^ > kernel/cgroup/rstat.c:509:27: note: (near initialization for 'bpf_rstat_kfunc_set') > > > vim +509 kernel/cgroup/rstat.c > > 505 > 506 static const struct btf_kfunc_id_set bpf_rstat_kfunc_set = { > 507 .owner = THIS_MODULE, > 508 .check_set = &bpf_rstat_check_kfunc_ids, > > 509 .sleepable_set = &bpf_rstat_sleepable_kfunc_ids, > 510 }; > 511 > > -- > 0-DAY CI Kernel Test Service > https://01.org/lkp AFAICT these failures are because the patch series depends on a patch in the mailing list [1] that is not in bpf-next, as explained by the cover letter. [1] https://lore.kernel.org/bpf/20220421140740.459558-5-benjamin.tissoires@redhat.com/
Hi Yosry,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Yosry-Ahmed/bpf-rstat-cgroup-hierarchical-stats/20220611-034720
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: hexagon-randconfig-r012-20220611 (https://download.01.org/0day-ci/archive/20220611/202206111842.O3viR9gq-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project ff4abe755279a3a47cc416ef80dbc900d9a98a19)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/83f297e2b47dc41b511f071b9eadf38339387b41
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yosry-Ahmed/bpf-rstat-cgroup-hierarchical-stats/20220611-034720
git checkout 83f297e2b47dc41b511f071b9eadf38339387b41
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash kernel/cgroup/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
>> kernel/cgroup/rstat.c:161:22: warning: no previous prototype for function 'bpf_rstat_flush' [-Wmissing-prototypes]
__weak noinline void bpf_rstat_flush(struct cgroup *cgrp,
^
kernel/cgroup/rstat.c:161:17: note: declare 'static' if the function is not intended to be used outside of this translation unit
__weak noinline void bpf_rstat_flush(struct cgroup *cgrp,
^
static
>> kernel/cgroup/rstat.c:509:3: error: field designator 'sleepable_set' does not refer to any field in type 'const struct btf_kfunc_id_set'
.sleepable_set = &bpf_rstat_sleepable_kfunc_ids,
^
1 warning and 1 error generated.
vim +509 kernel/cgroup/rstat.c
505
506 static const struct btf_kfunc_id_set bpf_rstat_kfunc_set = {
507 .owner = THIS_MODULE,
508 .check_set = &bpf_rstat_check_kfunc_ids,
> 509 .sleepable_set = &bpf_rstat_sleepable_kfunc_ids,
510 };
511
On Fri, Jun 10, 2022 at 02:30:00PM -0700, Yosry Ahmed wrote: > > AFAICT these failures are because the patch series depends on a patch > in the mailing list [1] that is not in bpf-next, as explained by the > cover letter. > > [1] https://lore.kernel.org/bpf/20220421140740.459558-5-benjamin.tissoires@redhat.com/ You probably want to rebase and include that patch as patch 1 in your series preserving Benjamin's SOB and cc-ing him on the series. Otherwise we cannot land the set, BPF CI cannot test it, and review is hard to do.
On Sat, Jun 11, 2022 at 12:57 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Jun 10, 2022 at 02:30:00PM -0700, Yosry Ahmed wrote: > > > > AFAICT these failures are because the patch series depends on a patch > > in the mailing list [1] that is not in bpf-next, as explained by the > > cover letter. > > > > [1] https://lore.kernel.org/bpf/20220421140740.459558-5-benjamin.tissoires@redhat.com/ > > You probably want to rebase and include that patch as patch 1 in your series > preserving Benjamin's SOB and cc-ing him on the series. > Otherwise we cannot land the set, BPF CI cannot test it, and review is hard to do. Sounds good. Will rebase do that and send a v3.
On 6/10/22 12:44 PM, Yosry Ahmed wrote: > Enable bpf programs to make use of rstat to collect cgroup hierarchical > stats efficiently: > - Add cgroup_rstat_updated() kfunc, for bpf progs that collect stats. > - Add cgroup_rstat_flush() kfunc, for bpf progs that read stats. > - Add an empty bpf_rstat_flush() hook that is called during rstat > flushing, for bpf progs that flush stats to attach to. Attaching a bpf > prog to this hook effectively registers it as a flush callback. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> Acked-by: Yonghong Song <yhs@fb.com>
diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c index 24b5c2ab55983..94140bd0d02a4 100644 --- a/kernel/cgroup/rstat.c +++ b/kernel/cgroup/rstat.c @@ -3,6 +3,11 @@ #include <linux/sched/cputime.h> +#include <linux/bpf.h> +#include <linux/btf.h> +#include <linux/btf_ids.h> + + static DEFINE_SPINLOCK(cgroup_rstat_lock); static DEFINE_PER_CPU(raw_spinlock_t, cgroup_rstat_cpu_lock); @@ -141,6 +146,23 @@ static struct cgroup *cgroup_rstat_cpu_pop_updated(struct cgroup *pos, return pos; } +/* + * A hook for bpf stat collectors to attach to and flush their stats. + * Together with providing bpf kfuncs for cgroup_rstat_updated() and + * cgroup_rstat_flush(), this enables a complete workflow where bpf progs that + * collect cgroup stats can integrate with rstat for efficient flushing. + * + * A static noinline declaration here could cause the compiler to optimize away + * the function. A global noinline declaration will keep the definition, but may + * optimize away the callsite. Therefore, __weak is needed to ensure that the + * call is still emitted, by telling the compiler that we don't know what the + * function might eventually be. + */ +__weak noinline void bpf_rstat_flush(struct cgroup *cgrp, + struct cgroup *parent, int cpu) +{ +} + /* see cgroup_rstat_flush() */ static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep) __releases(&cgroup_rstat_lock) __acquires(&cgroup_rstat_lock) @@ -168,6 +190,7 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep) struct cgroup_subsys_state *css; cgroup_base_stat_flush(pos, cpu); + bpf_rstat_flush(pos, cgroup_parent(pos), cpu); rcu_read_lock(); list_for_each_entry_rcu(css, &pos->rstat_css_list, @@ -469,3 +492,26 @@ void cgroup_base_stat_cputime_show(struct seq_file *seq) "system_usec %llu\n", usage, utime, stime); } + +/* Add bpf kfuncs for cgroup_rstat_updated() and cgroup_rstat_flush() */ +BTF_SET_START(bpf_rstat_check_kfunc_ids) +BTF_ID(func, cgroup_rstat_updated) +BTF_ID(func, cgroup_rstat_flush) +BTF_SET_END(bpf_rstat_check_kfunc_ids) + +BTF_SET_START(bpf_rstat_sleepable_kfunc_ids) +BTF_ID(func, cgroup_rstat_flush) +BTF_SET_END(bpf_rstat_sleepable_kfunc_ids) + +static const struct btf_kfunc_id_set bpf_rstat_kfunc_set = { + .owner = THIS_MODULE, + .check_set = &bpf_rstat_check_kfunc_ids, + .sleepable_set = &bpf_rstat_sleepable_kfunc_ids, +}; + +static int __init bpf_rstat_kfunc_init(void) +{ + return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, + &bpf_rstat_kfunc_set); +} +late_initcall(bpf_rstat_kfunc_init);
Enable bpf programs to make use of rstat to collect cgroup hierarchical stats efficiently: - Add cgroup_rstat_updated() kfunc, for bpf progs that collect stats. - Add cgroup_rstat_flush() kfunc, for bpf progs that read stats. - Add an empty bpf_rstat_flush() hook that is called during rstat flushing, for bpf progs that flush stats to attach to. Attaching a bpf prog to this hook effectively registers it as a flush callback. Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- kernel/cgroup/rstat.c | 46 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)