On Fri, May 5, 2023 at 9:56 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
>
> Em Fri, May 05, 2023 at 10:33:15AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Fri, May 05, 2023 at 01:03:14AM +0200, Jiri Olsa escreveu:
> > That with the preserve_access_index isn't needed, we need just the
> > fields that we access in the tools, right?
>
> I'm now doing build test this in many distro containers, without the two
> reverts, i.e. BPF skels continue as opt-out as in my pull request, to
> test build and also for the functionality tests on the tools using such
> bpf skels, see below, no touching of vmlinux nor BTF data during the
> build.
>
> - Arnaldo
>
> From 882adaee50bc27f85374aeb2fbaa5b76bef60d05 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <[email protected]>
> Date: Thu, 4 May 2023 19:03:51 -0300
> Subject: [PATCH 1/1] perf bpf skels: Stop using vmlinux.h generated from BTF,
> use subset of used structs + CO-RE
>
> Linus reported a build break due to using a vmlinux without a BTF elf
> section to generate the vmlinux.h header with bpftool for use in the BPF
> tools in tools/perf/util/bpf_skel/*.bpf.c.
>
> Instead add a vmlinux.h file with the structs needed with the fields the
> tools need, marking the structs with __attribute__((preserve_access_index)),
> so that libbpf's CO-RE code can fixup the struct field offsets.
>
> In some cases the vmlinux.h file that was being generated by bpftool
> from the kernel BTF information was not needed at all, just including
> linux/bpf.h, sometimes linux/perf_event.h was enough as non-UAPI
> types were not being used.
>
> To keep te patch small, include those UAPI headers from the trimmed down
> vmlinux.h file, that then provides the tools with just the structs and
> the subset of its fields needed for them.
>
> Testing it:
>
> # perf lock contention -b find / > /dev/null
> ^C contended total wait max wait avg wait type caller
>
> 7 53.59 us 10.86 us 7.66 us rwlock:R start_this_handle+0xa0
> 2 30.35 us 21.99 us 15.17 us rwsem:R iterate_dir+0x52
> 1 9.04 us 9.04 us 9.04 us rwlock:W start_this_handle+0x291
> 1 8.73 us 8.73 us 8.73 us spinlock raw_spin_rq_lock_nested+0x1e
> #
> # perf lock contention -abl find / > /dev/null
> ^C contended total wait max wait avg wait address symbol
>
> 1 262.96 ms 262.96 ms 262.96 ms ffff8e67502d0170 (mutex)
> 12 244.24 us 39.91 us 20.35 us ffff8e6af56f8070 mmap_lock (rwsem)
> 7 30.28 us 6.85 us 4.33 us ffff8e6c865f1d40 rq_lock (spinlock)
> 3 7.42 us 4.03 us 2.47 us ffff8e6c864b1d40 rq_lock (spinlock)
> 2 3.72 us 2.19 us 1.86 us ffff8e6c86571d40 rq_lock (spinlock)
> 1 2.42 us 2.42 us 2.42 us ffff8e6c86471d40 rq_lock (spinlock)
> 4 2.11 us 559 ns 527 ns ffffffff9a146c80 rcu_state (spinlock)
> 3 1.45 us 818 ns 482 ns ffff8e674ae8384c (rwlock)
> 1 870 ns 870 ns 870 ns ffff8e68456ee060 (rwlock)
> 1 663 ns 663 ns 663 ns ffff8e6c864f1d40 rq_lock (spinlock)
> 1 573 ns 573 ns 573 ns ffff8e6c86531d40 rq_lock (spinlock)
> 1 472 ns 472 ns 472 ns ffff8e6c86431740 (spinlock)
> 1 397 ns 397 ns 397 ns ffff8e67413a4f04 (spinlock)
> #
> # perf test offcpu
> 95: perf record offcpu profiling tests : Ok
> #
> # perf kwork latency --use-bpf
> Starting trace, Hit <Ctrl+C> to stop and report
> ^C
> Kwork Name | Cpu | Avg delay | Count | Max delay | Max delay start | Max delay end |
> --------------------------------------------------------------------------------------------------------------------------------
> (w)flush_memcg_stats_dwork | 0000 | 1056.212 ms | 2 | 2112.345 ms | 550113.229573 s | 550115.341919 s |
> (w)toggle_allocation_gate | 0000 | 10.144 ms | 62 | 416.389 ms | 550113.453518 s | 550113.869907 s |
> (w)0xffff8e6748e28080 | 0002 | 0.623 ms | 1 | 0.623 ms | 550110.989841 s | 550110.990464 s |
> (w)vmstat_shepherd | 0000 | 0.586 ms | 10 | 2.828 ms | 550111.971536 s | 550111.974364 s |
> (w)vmstat_update | 0007 | 0.363 ms | 5 | 1.634 ms | 550113.222520 s | 550113.224154 s |
> (w)vmstat_update | 0000 | 0.324 ms | 10 | 2.827 ms | 550111.971526 s | 550111.974354 s |
> (w)0xffff8e674c5f4a58 | 0002 | 0.102 ms | 5 | 0.134 ms | 550110.989839 s | 550110.989972 s |
> (w)psi_avgs_work | 0001 | 0.086 ms | 3 | 0.107 ms | 550114.957852 s | 550114.957959 s |
> (w)psi_avgs_work | 0000 | 0.079 ms | 5 | 0.100 ms | 550118.605668 s | 550118.605768 s |
> (w)kfree_rcu_monitor | 0006 | 0.079 ms | 1 | 0.079 ms | 550110.925821 s | 550110.925900 s |
> (w)psi_avgs_work | 0004 | 0.079 ms | 1 | 0.079 ms | 550109.581835 s | 550109.581914 s |
> (w)psi_avgs_work | 0001 | 0.078 ms | 1 | 0.078 ms | 550109.197809 s | 550109.197887 s |
> (w)psi_avgs_work | 0002 | 0.077 ms | 5 | 0.086 ms | 550110.669819 s | 550110.669905 s |
> <SNIP>
> # strace -e bpf -o perf-stat-bpf-counters.output perf stat -e cycles --bpf-counters sleep 1
>
> Performance counter stats for 'sleep 1':
>
> 6,197,983 cycles
>
> 1.003922848 seconds time elapsed
>
> 0.000000000 seconds user
> 0.002032000 seconds sys
>
> # head -7 perf-stat-bpf-counters.output
> bpf(BPF_OBJ_GET, {pathname="/sys/fs/bpf/perf_attr_map", bpf_fd=0, file_flags=0}, 16) = 3
> bpf(BPF_OBJ_GET_INFO_BY_FD, {info={bpf_fd=3, info_len=88, info=0x7ffcead64990}}, 16) = 0
> bpf(BPF_MAP_LOOKUP_ELEM, {map_fd=3, key=0x24129e0, value=0x7ffcead65a48, flags=BPF_ANY}, 32) = 0
> bpf(BPF_LINK_GET_FD_BY_ID, {link_id=1252}, 12) = -1 ENOENT (No such file or directory)
> bpf(BPF_PROG_LOAD, {prog_type=BPF_PROG_TYPE_SOCKET_FILTER, insn_cnt=2, insns=0x7ffcead65780, license="GPL", log_level=0, log_size=0, log_buf=NULL, kern_version=KERNEL_VERSION(0, 0, 0), prog_flags=0, prog_name="", prog_ifindex=0, expected_attach_type=BPF_CGROUP_INET_INGRESS, prog_btf_fd=0, func_info_rec_size=0, func_info=NULL, func_info_cnt=0, line_info_rec_size=0, line_info=NULL, line_info_cnt=0, attach_btf_id=0, attach_prog_fd=0}, 116) = 4
> bpf(BPF_PROG_LOAD, {prog_type=BPF_PROG_TYPE_SOCKET_FILTER, insn_cnt=2, insns=0x7ffcead65920, license="GPL", log_level=0, log_size=0, log_buf=NULL, kern_version=KERNEL_VERSION(0, 0, 0), prog_flags=0, prog_name="", prog_ifindex=0, expected_attach_type=BPF_CGROUP_INET_INGRESS, prog_btf_fd=0, func_info_rec_size=0, func_info=NULL, func_info_cnt=0, line_info_rec_size=0, line_info=NULL, line_info_cnt=0, attach_btf_id=0, attach_prog_fd=0, fd_array=NULL}, 128) = 4
> bpf(BPF_BTF_LOAD, {btf="\237\353\1\0\30\0\0\0\0\0\0\0\20\0\0\0\20\0\0\0\5\0\0\0\1\0\0\0\0\0\0\1"..., btf_log_buf=NULL, btf_size=45, btf_log_size=0, btf_log_level=0}, 28) = 4
> #
>
> Reported-by: Linus Torvalds <[email protected]>
> Suggested-by: Andrii Nakryiko <[email protected]>
> Cc: Adrian Hunter <[email protected]>
> Cc: Ian Rogers <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Co-developed-by: Jiri Olsa <[email protected]>
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
> ---
> tools/perf/Makefile.perf | 20 +---
> tools/perf/util/bpf_skel/.gitignore | 1 -
> tools/perf/util/bpf_skel/vmlinux.h | 173 ++++++++++++++++++++++++++++
> 3 files changed, 174 insertions(+), 20 deletions(-)
> create mode 100644 tools/perf/util/bpf_skel/vmlinux.h
>
> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> index 48aba186ceb50792..61c33d100b2bcc90 100644
> --- a/tools/perf/Makefile.perf
> +++ b/tools/perf/Makefile.perf
> @@ -1063,25 +1063,7 @@ $(BPFTOOL): | $(SKEL_TMP_OUT)
> $(Q)CFLAGS= $(MAKE) -C ../bpf/bpftool \
> OUTPUT=$(SKEL_TMP_OUT)/ bootstrap
>
> -VMLINUX_BTF_PATHS ?= $(if $(O),$(O)/vmlinux) \
> - $(if $(KBUILD_OUTPUT),$(KBUILD_OUTPUT)/vmlinux) \
> - ../../vmlinux \
> - /sys/kernel/btf/vmlinux \
> - /boot/vmlinux-$(shell uname -r)
> -VMLINUX_BTF ?= $(abspath $(firstword $(wildcard $(VMLINUX_BTF_PATHS))))
> -
> -$(SKEL_OUT)/vmlinux.h: $(VMLINUX_BTF) $(BPFTOOL)
> -ifeq ($(VMLINUX_H),)
> - $(QUIET_GEN)$(BPFTOOL) btf dump file $< format c > $@ || \
> - (echo "Failure to generate vmlinux.h needed for the recommended BPF skeleton support." && \
> - echo "To disable this use the build option NO_BPF_SKEL=1." && \
> - echo "Alternatively point at a pre-generated vmlinux.h with VMLINUX_H=<path>." && \
> - false)
> -else
> - $(Q)cp "$(VMLINUX_H)" $@
> -endif
Perhaps we can define VMLINUX_H in the Makefile.config to be
tools/perf/util/bpf_skel/vmlinux.h, then someone can clear it like:
make -C tools/perf VMLINIX_H=
to trigger generation, etc. Such thoughts may be better kept for 6.5 :-)
Thanks,
Ian
> -$(SKEL_TMP_OUT)/%.bpf.o: util/bpf_skel/%.bpf.c $(LIBBPF) $(SKEL_OUT)/vmlinux.h | $(SKEL_TMP_OUT)
> +$(SKEL_TMP_OUT)/%.bpf.o: util/bpf_skel/%.bpf.c $(LIBBPF) | $(SKEL_TMP_OUT)
> $(QUIET_CLANG)$(CLANG) -g -O2 -target bpf -Wall -Werror $(BPF_INCLUDE) \
> -c $(filter util/bpf_skel/%.bpf.c,$^) -o $@ && $(LLVM_STRIP) -g $@
>
> diff --git a/tools/perf/util/bpf_skel/.gitignore b/tools/perf/util/bpf_skel/.gitignore
> index cd01455e1b53c3d9..7a1c832825de8445 100644
> --- a/tools/perf/util/bpf_skel/.gitignore
> +++ b/tools/perf/util/bpf_skel/.gitignore
> @@ -1,4 +1,3 @@
> # SPDX-License-Identifier: GPL-2.0-only
> .tmp
> *.skel.h
> -vmlinux.h
> diff --git a/tools/perf/util/bpf_skel/vmlinux.h b/tools/perf/util/bpf_skel/vmlinux.h
> new file mode 100644
> index 0000000000000000..449b1ea91fc48143
> --- /dev/null
> +++ b/tools/perf/util/bpf_skel/vmlinux.h
> @@ -0,0 +1,173 @@
> +#ifndef __VMLINUX_H
> +#define __VMLINUX_H
> +
> +#include <linux/bpf.h>
> +#include <linux/types.h>
> +#include <linux/perf_event.h>
> +#include <stdbool.h>
> +
> +// non-UAPI kernel data structures, used in the .bpf.c BPF tool component.
> +
> +// Just the fields used in these tools preserving the access index so that
> +// libbpf can fixup offsets with the ones used in the kernel when loading the
> +// BPF bytecode, if they differ from what is used here.
> +
> +typedef __u8 u8;
> +typedef __u32 u32;
> +typedef __u64 u64;
> +typedef __s64 s64;
> +
> +typedef int pid_t;
> +
> +enum cgroup_subsys_id {
> + perf_event_cgrp_id = 8,
> +};
> +
> +enum {
> + HI_SOFTIRQ = 0,
> + TIMER_SOFTIRQ,
> + NET_TX_SOFTIRQ,
> + NET_RX_SOFTIRQ,
> + BLOCK_SOFTIRQ,
> + IRQ_POLL_SOFTIRQ,
> + TASKLET_SOFTIRQ,
> + SCHED_SOFTIRQ,
> + HRTIMER_SOFTIRQ,
> + RCU_SOFTIRQ, /* Preferable RCU should always be the last softirq */
> +
> + NR_SOFTIRQS
> +};
> +
> +typedef struct {
> + s64 counter;
> +} __attribute__((preserve_access_index)) atomic64_t;
> +
> +typedef atomic64_t atomic_long_t;
> +
> +struct raw_spinlock {
> + int rawlock;
> +} __attribute__((preserve_access_index));
> +
> +typedef struct raw_spinlock raw_spinlock_t;
> +
> +typedef struct {
> + struct raw_spinlock rlock;
> +} __attribute__((preserve_access_index)) spinlock_t;
> +
> +struct sighand_struct {
> + spinlock_t siglock;
> +} __attribute__((preserve_access_index));
> +
> +struct rw_semaphore {
> + atomic_long_t owner;
> +} __attribute__((preserve_access_index));
> +
> +struct mutex {
> + atomic_long_t owner;
> +} __attribute__((preserve_access_index));
> +
> +struct kernfs_node {
> + u64 id;
> +} __attribute__((preserve_access_index));
> +
> +struct cgroup {
> + struct kernfs_node *kn;
> + int level;
> +} __attribute__((preserve_access_index));
> +
> +struct cgroup_subsys_state {
> + struct cgroup *cgroup;
> +} __attribute__((preserve_access_index));
> +
> +struct css_set {
> + struct cgroup_subsys_state *subsys[13];
> + struct cgroup *dfl_cgrp;
> +} __attribute__((preserve_access_index));
> +
> +struct mm_struct {
> + struct rw_semaphore mmap_lock;
> +} __attribute__((preserve_access_index));
> +
> +struct task_struct {
> + unsigned int flags;
> + struct mm_struct *mm;
> + pid_t pid;
> + pid_t tgid;
> + char comm[16];
> + struct sighand_struct *sighand;
> + struct css_set *cgroups;
> +} __attribute__((preserve_access_index));
> +
> +struct trace_entry {
> + short unsigned int type;
> + unsigned char flags;
> + unsigned char preempt_count;
> + int pid;
> +} __attribute__((preserve_access_index));
> +
> +struct trace_event_raw_irq_handler_entry {
> + struct trace_entry ent;
> + int irq;
> + u32 __data_loc_name;
> + char __data[];
> +} __attribute__((preserve_access_index));
> +
> +struct trace_event_raw_irq_handler_exit {
> + struct trace_entry ent;
> + int irq;
> + int ret;
> + char __data[];
> +} __attribute__((preserve_access_index));
> +
> +struct trace_event_raw_softirq {
> + struct trace_entry ent;
> + unsigned int vec;
> + char __data[];
> +} __attribute__((preserve_access_index));
> +
> +struct trace_event_raw_workqueue_execute_start {
> + struct trace_entry ent;
> + void *work;
> + void *function;
> + char __data[];
> +} __attribute__((preserve_access_index));
> +
> +struct trace_event_raw_workqueue_execute_end {
> + struct trace_entry ent;
> + void *work;
> + void *function;
> + char __data[];
> +} __attribute__((preserve_access_index));
> +
> +struct trace_event_raw_workqueue_activate_work {
> + struct trace_entry ent;
> + void *work;
> + char __data[];
> +} __attribute__((preserve_access_index));
> +
> +struct perf_sample_data {
> + u64 addr;
> + u64 period;
> + union perf_sample_weight weight;
> + u64 txn;
> + union perf_mem_data_src data_src;
> + u64 ip;
> + struct {
> + u32 pid;
> + u32 tid;
> + } tid_entry;
> + u64 time;
> + u64 id;
> + struct {
> + u32 cpu;
> + } cpu_entry;
> + u64 phys_addr;
> + u64 data_page_size;
> + u64 code_page_size;
> +} __attribute__((__aligned__(64))) __attribute__((preserve_access_index));
> +
> +struct bpf_perf_event_data_kern {
> + struct perf_sample_data *data;
> + struct perf_event *event;
> +} __attribute__((preserve_access_index));
> +#endif // __VMLINUX_H
> --
> 2.39.2
>
On Fri, May 05, 2023 at 10:04:47AM -0700, Ian Rogers wrote:
> On Fri, May 5, 2023 at 9:56 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
> >
> > Em Fri, May 05, 2023 at 10:33:15AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Fri, May 05, 2023 at 01:03:14AM +0200, Jiri Olsa escreveu:
> > > That with the preserve_access_index isn't needed, we need just the
> > > fields that we access in the tools, right?
> >
> > I'm now doing build test this in many distro containers, without the two
> > reverts, i.e. BPF skels continue as opt-out as in my pull request, to
> > test build and also for the functionality tests on the tools using such
> > bpf skels, see below, no touching of vmlinux nor BTF data during the
> > build.
> >
> > - Arnaldo
> >
> > From 882adaee50bc27f85374aeb2fbaa5b76bef60d05 Mon Sep 17 00:00:00 2001
> > From: Arnaldo Carvalho de Melo <[email protected]>
> > Date: Thu, 4 May 2023 19:03:51 -0300
> > Subject: [PATCH 1/1] perf bpf skels: Stop using vmlinux.h generated from BTF,
> > use subset of used structs + CO-RE
> >
> > Linus reported a build break due to using a vmlinux without a BTF elf
> > section to generate the vmlinux.h header with bpftool for use in the BPF
> > tools in tools/perf/util/bpf_skel/*.bpf.c.
> >
> > Instead add a vmlinux.h file with the structs needed with the fields the
> > tools need, marking the structs with __attribute__((preserve_access_index)),
> > so that libbpf's CO-RE code can fixup the struct field offsets.
> >
> > In some cases the vmlinux.h file that was being generated by bpftool
> > from the kernel BTF information was not needed at all, just including
> > linux/bpf.h, sometimes linux/perf_event.h was enough as non-UAPI
> > types were not being used.
> >
> > To keep te patch small, include those UAPI headers from the trimmed down
> > vmlinux.h file, that then provides the tools with just the structs and
> > the subset of its fields needed for them.
> >
> > Testing it:
> >
> > # perf lock contention -b find / > /dev/null
I tested perf lock con -abv -L rcu_state sleep 1
and needed fix below
jirka
---
From b12aea55f1171dc09cde2957f9019c84bda7adbb Mon Sep 17 00:00:00 2001
From: Jiri Olsa <[email protected]>
Date: Fri, 5 May 2023 13:28:46 +0200
Subject: [PATCH] perf tools: Fix lock_contention bpf program
We need to define empty 'struct rq' so the runqueues gets
resolved properly:
# ./perf lock con -b
libbpf: extern (var ksym) 'runqueues': incompatible types, expected [99] fwd rq, but kernel has [19783] struct rq
libbpf: failed to load object 'lock_contention_bpf'
libbpf: failed to load BPF skeleton 'lock_contention_bpf': -22
Failed to load lock-contention BPF skeleton
Also rq__old/rq__new need additional '_' so the suffix is ignored
properly.
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/util/bpf_skel/lock_contention.bpf.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
index 8911e2a077d8..c2bf24c68c14 100644
--- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
+++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
@@ -416,13 +416,15 @@ int contention_end(u64 *ctx)
return 0;
}
+struct rq {};
+
extern struct rq runqueues __ksym;
-struct rq__old {
+struct rq___old {
raw_spinlock_t lock;
} __attribute__((preserve_access_index));
-struct rq__new {
+struct rq___new {
raw_spinlock_t __lock;
} __attribute__((preserve_access_index));
@@ -434,8 +436,8 @@ int BPF_PROG(collect_lock_syms)
for (int i = 0; i < MAX_CPUS; i++) {
struct rq *rq = bpf_per_cpu_ptr(&runqueues, i);
- struct rq__new *rq_new = (void *)rq;
- struct rq__old *rq_old = (void *)rq;
+ struct rq___new *rq_new = (void *)rq;
+ struct rq___old *rq_old = (void *)rq;
if (rq == NULL)
break;
--
2.40.1
On Fri, May 5, 2023 at 1:43 PM Jiri Olsa <[email protected]> wrote:
>
> On Fri, May 05, 2023 at 10:04:47AM -0700, Ian Rogers wrote:
> > On Fri, May 5, 2023 at 9:56 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > >
> > > Em Fri, May 05, 2023 at 10:33:15AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Fri, May 05, 2023 at 01:03:14AM +0200, Jiri Olsa escreveu:
> > > > That with the preserve_access_index isn't needed, we need just the
> > > > fields that we access in the tools, right?
> > >
> > > I'm now doing build test this in many distro containers, without the two
> > > reverts, i.e. BPF skels continue as opt-out as in my pull request, to
> > > test build and also for the functionality tests on the tools using such
> > > bpf skels, see below, no touching of vmlinux nor BTF data during the
> > > build.
> > >
> > > - Arnaldo
> > >
> > > From 882adaee50bc27f85374aeb2fbaa5b76bef60d05 Mon Sep 17 00:00:00 2001
> > > From: Arnaldo Carvalho de Melo <[email protected]>
> > > Date: Thu, 4 May 2023 19:03:51 -0300
> > > Subject: [PATCH 1/1] perf bpf skels: Stop using vmlinux.h generated from BTF,
> > > use subset of used structs + CO-RE
> > >
> > > Linus reported a build break due to using a vmlinux without a BTF elf
> > > section to generate the vmlinux.h header with bpftool for use in the BPF
> > > tools in tools/perf/util/bpf_skel/*.bpf.c.
> > >
> > > Instead add a vmlinux.h file with the structs needed with the fields the
> > > tools need, marking the structs with __attribute__((preserve_access_index)),
> > > so that libbpf's CO-RE code can fixup the struct field offsets.
> > >
> > > In some cases the vmlinux.h file that was being generated by bpftool
> > > from the kernel BTF information was not needed at all, just including
> > > linux/bpf.h, sometimes linux/perf_event.h was enough as non-UAPI
> > > types were not being used.
> > >
> > > To keep te patch small, include those UAPI headers from the trimmed down
> > > vmlinux.h file, that then provides the tools with just the structs and
> > > the subset of its fields needed for them.
> > >
> > > Testing it:
> > >
> > > # perf lock contention -b find / > /dev/null
>
> I tested perf lock con -abv -L rcu_state sleep 1
> and needed fix below
>
> jirka
I thought this was fixed by:
https://lore.kernel.org/lkml/[email protected]/
but I think that is just in perf-tools-next.
Thanks,
Ian
> ---
> From b12aea55f1171dc09cde2957f9019c84bda7adbb Mon Sep 17 00:00:00 2001
> From: Jiri Olsa <[email protected]>
> Date: Fri, 5 May 2023 13:28:46 +0200
> Subject: [PATCH] perf tools: Fix lock_contention bpf program
>
> We need to define empty 'struct rq' so the runqueues gets
> resolved properly:
>
> # ./perf lock con -b
> libbpf: extern (var ksym) 'runqueues': incompatible types, expected [99] fwd rq, but kernel has [19783] struct rq
> libbpf: failed to load object 'lock_contention_bpf'
> libbpf: failed to load BPF skeleton 'lock_contention_bpf': -22
> Failed to load lock-contention BPF skeleton
>
> Also rq__old/rq__new need additional '_' so the suffix is ignored
> properly.
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/perf/util/bpf_skel/lock_contention.bpf.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> index 8911e2a077d8..c2bf24c68c14 100644
> --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
> +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> @@ -416,13 +416,15 @@ int contention_end(u64 *ctx)
> return 0;
> }
>
> +struct rq {};
> +
> extern struct rq runqueues __ksym;
>
> -struct rq__old {
> +struct rq___old {
> raw_spinlock_t lock;
> } __attribute__((preserve_access_index));
>
> -struct rq__new {
> +struct rq___new {
> raw_spinlock_t __lock;
> } __attribute__((preserve_access_index));
>
> @@ -434,8 +436,8 @@ int BPF_PROG(collect_lock_syms)
>
> for (int i = 0; i < MAX_CPUS; i++) {
> struct rq *rq = bpf_per_cpu_ptr(&runqueues, i);
> - struct rq__new *rq_new = (void *)rq;
> - struct rq__old *rq_old = (void *)rq;
> + struct rq___new *rq_new = (void *)rq;
> + struct rq___old *rq_old = (void *)rq;
>
> if (rq == NULL)
> break;
> --
> 2.40.1
>
On Fri, May 5, 2023 at 1:46 PM Ian Rogers <[email protected]> wrote:
>
> On Fri, May 5, 2023 at 1:43 PM Jiri Olsa <[email protected]> wrote:
> >
> > On Fri, May 05, 2023 at 10:04:47AM -0700, Ian Rogers wrote:
> > > On Fri, May 5, 2023 at 9:56 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > > >
> > > > Em Fri, May 05, 2023 at 10:33:15AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > Em Fri, May 05, 2023 at 01:03:14AM +0200, Jiri Olsa escreveu:
> > > > > That with the preserve_access_index isn't needed, we need just the
> > > > > fields that we access in the tools, right?
> > > >
> > > > I'm now doing build test this in many distro containers, without the two
> > > > reverts, i.e. BPF skels continue as opt-out as in my pull request, to
> > > > test build and also for the functionality tests on the tools using such
> > > > bpf skels, see below, no touching of vmlinux nor BTF data during the
> > > > build.
> > > >
> > > > - Arnaldo
> > > >
> > > > From 882adaee50bc27f85374aeb2fbaa5b76bef60d05 Mon Sep 17 00:00:00 2001
> > > > From: Arnaldo Carvalho de Melo <[email protected]>
> > > > Date: Thu, 4 May 2023 19:03:51 -0300
> > > > Subject: [PATCH 1/1] perf bpf skels: Stop using vmlinux.h generated from BTF,
> > > > use subset of used structs + CO-RE
> > > >
> > > > Linus reported a build break due to using a vmlinux without a BTF elf
> > > > section to generate the vmlinux.h header with bpftool for use in the BPF
> > > > tools in tools/perf/util/bpf_skel/*.bpf.c.
> > > >
> > > > Instead add a vmlinux.h file with the structs needed with the fields the
> > > > tools need, marking the structs with __attribute__((preserve_access_index)),
> > > > so that libbpf's CO-RE code can fixup the struct field offsets.
> > > >
> > > > In some cases the vmlinux.h file that was being generated by bpftool
> > > > from the kernel BTF information was not needed at all, just including
> > > > linux/bpf.h, sometimes linux/perf_event.h was enough as non-UAPI
> > > > types were not being used.
> > > >
> > > > To keep te patch small, include those UAPI headers from the trimmed down
> > > > vmlinux.h file, that then provides the tools with just the structs and
> > > > the subset of its fields needed for them.
> > > >
> > > > Testing it:
> > > >
> > > > # perf lock contention -b find / > /dev/null
> >
> > I tested perf lock con -abv -L rcu_state sleep 1
> > and needed fix below
> >
> > jirka
>
> I thought this was fixed by:
> https://lore.kernel.org/lkml/[email protected]/
> but I think that is just in perf-tools-next.
Right, but we might still need the empty rq definition.
Thanks,
Namhyung
On Fri, May 05, 2023 at 01:46:30PM -0700, Ian Rogers wrote:
> On Fri, May 5, 2023 at 1:43 PM Jiri Olsa <[email protected]> wrote:
> >
> > On Fri, May 05, 2023 at 10:04:47AM -0700, Ian Rogers wrote:
> > > On Fri, May 5, 2023 at 9:56 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > > >
> > > > Em Fri, May 05, 2023 at 10:33:15AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > Em Fri, May 05, 2023 at 01:03:14AM +0200, Jiri Olsa escreveu:
> > > > > That with the preserve_access_index isn't needed, we need just the
> > > > > fields that we access in the tools, right?
> > > >
> > > > I'm now doing build test this in many distro containers, without the two
> > > > reverts, i.e. BPF skels continue as opt-out as in my pull request, to
> > > > test build and also for the functionality tests on the tools using such
> > > > bpf skels, see below, no touching of vmlinux nor BTF data during the
> > > > build.
> > > >
> > > > - Arnaldo
> > > >
> > > > From 882adaee50bc27f85374aeb2fbaa5b76bef60d05 Mon Sep 17 00:00:00 2001
> > > > From: Arnaldo Carvalho de Melo <[email protected]>
> > > > Date: Thu, 4 May 2023 19:03:51 -0300
> > > > Subject: [PATCH 1/1] perf bpf skels: Stop using vmlinux.h generated from BTF,
> > > > use subset of used structs + CO-RE
> > > >
> > > > Linus reported a build break due to using a vmlinux without a BTF elf
> > > > section to generate the vmlinux.h header with bpftool for use in the BPF
> > > > tools in tools/perf/util/bpf_skel/*.bpf.c.
> > > >
> > > > Instead add a vmlinux.h file with the structs needed with the fields the
> > > > tools need, marking the structs with __attribute__((preserve_access_index)),
> > > > so that libbpf's CO-RE code can fixup the struct field offsets.
> > > >
> > > > In some cases the vmlinux.h file that was being generated by bpftool
> > > > from the kernel BTF information was not needed at all, just including
> > > > linux/bpf.h, sometimes linux/perf_event.h was enough as non-UAPI
> > > > types were not being used.
> > > >
> > > > To keep te patch small, include those UAPI headers from the trimmed down
> > > > vmlinux.h file, that then provides the tools with just the structs and
> > > > the subset of its fields needed for them.
> > > >
> > > > Testing it:
> > > >
> > > > # perf lock contention -b find / > /dev/null
> >
> > I tested perf lock con -abv -L rcu_state sleep 1
> > and needed fix below
> >
> > jirka
>
> I thought this was fixed by:
> https://lore.kernel.org/lkml/[email protected]/
> but I think that is just in perf-tools-next.
ah ok, missed that one
thanks,
jirka
On Fri, May 5, 2023 at 2:15 PM Jiri Olsa <[email protected]> wrote:
>
> On Fri, May 05, 2023 at 01:46:30PM -0700, Ian Rogers wrote:
> > On Fri, May 5, 2023 at 1:43 PM Jiri Olsa <[email protected]> wrote:
> > >
> > > On Fri, May 05, 2023 at 10:04:47AM -0700, Ian Rogers wrote:
> > > > On Fri, May 5, 2023 at 9:56 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > > > >
> > > > > Em Fri, May 05, 2023 at 10:33:15AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > > Em Fri, May 05, 2023 at 01:03:14AM +0200, Jiri Olsa escreveu:
> > > > > > That with the preserve_access_index isn't needed, we need just the
> > > > > > fields that we access in the tools, right?
> > > > >
> > > > > I'm now doing build test this in many distro containers, without the two
> > > > > reverts, i.e. BPF skels continue as opt-out as in my pull request, to
> > > > > test build and also for the functionality tests on the tools using such
> > > > > bpf skels, see below, no touching of vmlinux nor BTF data during the
> > > > > build.
> > > > >
> > > > > - Arnaldo
> > > > >
> > > > > From 882adaee50bc27f85374aeb2fbaa5b76bef60d05 Mon Sep 17 00:00:00 2001
> > > > > From: Arnaldo Carvalho de Melo <[email protected]>
> > > > > Date: Thu, 4 May 2023 19:03:51 -0300
> > > > > Subject: [PATCH 1/1] perf bpf skels: Stop using vmlinux.h generated from BTF,
> > > > > use subset of used structs + CO-RE
> > > > >
> > > > > Linus reported a build break due to using a vmlinux without a BTF elf
> > > > > section to generate the vmlinux.h header with bpftool for use in the BPF
> > > > > tools in tools/perf/util/bpf_skel/*.bpf.c.
> > > > >
> > > > > Instead add a vmlinux.h file with the structs needed with the fields the
> > > > > tools need, marking the structs with __attribute__((preserve_access_index)),
> > > > > so that libbpf's CO-RE code can fixup the struct field offsets.
> > > > >
> > > > > In some cases the vmlinux.h file that was being generated by bpftool
> > > > > from the kernel BTF information was not needed at all, just including
> > > > > linux/bpf.h, sometimes linux/perf_event.h was enough as non-UAPI
> > > > > types were not being used.
> > > > >
> > > > > To keep te patch small, include those UAPI headers from the trimmed down
> > > > > vmlinux.h file, that then provides the tools with just the structs and
> > > > > the subset of its fields needed for them.
> > > > >
> > > > > Testing it:
> > > > >
> > > > > # perf lock contention -b find / > /dev/null
> > >
> > > I tested perf lock con -abv -L rcu_state sleep 1
> > > and needed fix below
> > >
> > > jirka
> >
> > I thought this was fixed by:
> > https://lore.kernel.org/lkml/[email protected]/
> > but I think that is just in perf-tools-next.
>
> ah ok, missed that one
Please try validating with veristat to check if all of perf's .bpf.o
files are successful. Veristat is part of selftests and can be built
with just `make -C tools/testing/selftests/bpf veristat`. After that;
sudo ~/bin/veristat tools/perf/util/bpf_skel/.tmp/*.bpf.o
This is a surer way to check that BPF object files are ok at least on
your currently running kernel, than trying to exercise each BPF
program through perf commands.
>
> thanks,
> jirka
Em Fri, May 05, 2023 at 10:43:36PM +0200, Jiri Olsa escreveu:
> On Fri, May 05, 2023 at 10:04:47AM -0700, Ian Rogers wrote:
> > On Fri, May 5, 2023 at 9:56 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > >
> > > Em Fri, May 05, 2023 at 10:33:15AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Fri, May 05, 2023 at 01:03:14AM +0200, Jiri Olsa escreveu:
> > > > That with the preserve_access_index isn't needed, we need just the
> > > > fields that we access in the tools, right?
> > >
> > > I'm now doing build test this in many distro containers, without the two
> > > reverts, i.e. BPF skels continue as opt-out as in my pull request, to
> > > test build and also for the functionality tests on the tools using such
> > > bpf skels, see below, no touching of vmlinux nor BTF data during the
> > > build.
> > >
> > > - Arnaldo
> > >
> > > From 882adaee50bc27f85374aeb2fbaa5b76bef60d05 Mon Sep 17 00:00:00 2001
> > > From: Arnaldo Carvalho de Melo <[email protected]>
> > > Date: Thu, 4 May 2023 19:03:51 -0300
> > > Subject: [PATCH 1/1] perf bpf skels: Stop using vmlinux.h generated from BTF,
> > > use subset of used structs + CO-RE
> > >
> > > Linus reported a build break due to using a vmlinux without a BTF elf
> > > section to generate the vmlinux.h header with bpftool for use in the BPF
> > > tools in tools/perf/util/bpf_skel/*.bpf.c.
> > >
> > > Instead add a vmlinux.h file with the structs needed with the fields the
> > > tools need, marking the structs with __attribute__((preserve_access_index)),
> > > so that libbpf's CO-RE code can fixup the struct field offsets.
> > >
> > > In some cases the vmlinux.h file that was being generated by bpftool
> > > from the kernel BTF information was not needed at all, just including
> > > linux/bpf.h, sometimes linux/perf_event.h was enough as non-UAPI
> > > types were not being used.
> > >
> > > To keep te patch small, include those UAPI headers from the trimmed down
> > > vmlinux.h file, that then provides the tools with just the structs and
> > > the subset of its fields needed for them.
> > >
> > > Testing it:
> > >
> > > # perf lock contention -b find / > /dev/null
>
> I tested perf lock con -abv -L rcu_state sleep 1
> and needed fix below
>
> jirka
patch not applying trying to do it manually.
- Arnaldo
>
> ---
> From b12aea55f1171dc09cde2957f9019c84bda7adbb Mon Sep 17 00:00:00 2001
> From: Jiri Olsa <[email protected]>
> Date: Fri, 5 May 2023 13:28:46 +0200
> Subject: [PATCH] perf tools: Fix lock_contention bpf program
>
> We need to define empty 'struct rq' so the runqueues gets
> resolved properly:
>
> # ./perf lock con -b
> libbpf: extern (var ksym) 'runqueues': incompatible types, expected [99] fwd rq, but kernel has [19783] struct rq
> libbpf: failed to load object 'lock_contention_bpf'
> libbpf: failed to load BPF skeleton 'lock_contention_bpf': -22
> Failed to load lock-contention BPF skeleton
>
> Also rq__old/rq__new need additional '_' so the suffix is ignored
> properly.
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/perf/util/bpf_skel/lock_contention.bpf.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> index 8911e2a077d8..c2bf24c68c14 100644
> --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
> +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> @@ -416,13 +416,15 @@ int contention_end(u64 *ctx)
> return 0;
> }
>
> +struct rq {};
> +
> extern struct rq runqueues __ksym;
>
> -struct rq__old {
> +struct rq___old {
> raw_spinlock_t lock;
> } __attribute__((preserve_access_index));
>
> -struct rq__new {
> +struct rq___new {
> raw_spinlock_t __lock;
> } __attribute__((preserve_access_index));
>
> @@ -434,8 +436,8 @@ int BPF_PROG(collect_lock_syms)
>
> for (int i = 0; i < MAX_CPUS; i++) {
> struct rq *rq = bpf_per_cpu_ptr(&runqueues, i);
> - struct rq__new *rq_new = (void *)rq;
> - struct rq__old *rq_old = (void *)rq;
> + struct rq___new *rq_new = (void *)rq;
> + struct rq___old *rq_old = (void *)rq;
>
> if (rq == NULL)
> break;
> --
> 2.40.1
>
--
- Arnaldo
Em Fri, May 05, 2023 at 02:21:56PM -0700, Andrii Nakryiko escreveu:
> On Fri, May 5, 2023 at 2:15 PM Jiri Olsa <[email protected]> wrote:
> >
> > On Fri, May 05, 2023 at 01:46:30PM -0700, Ian Rogers wrote:
> > > On Fri, May 5, 2023 at 1:43 PM Jiri Olsa <[email protected]> wrote:
> > > >
> > > > On Fri, May 05, 2023 at 10:04:47AM -0700, Ian Rogers wrote:
> > > > > On Fri, May 5, 2023 at 9:56 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > > > > >
> > > > > > Em Fri, May 05, 2023 at 10:33:15AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > > > Em Fri, May 05, 2023 at 01:03:14AM +0200, Jiri Olsa escreveu:
> > > > > > > That with the preserve_access_index isn't needed, we need just the
> > > > > > > fields that we access in the tools, right?
> > > > > >
> > > > > > I'm now doing build test this in many distro containers, without the two
> > > > > > reverts, i.e. BPF skels continue as opt-out as in my pull request, to
> > > > > > test build and also for the functionality tests on the tools using such
> > > > > > bpf skels, see below, no touching of vmlinux nor BTF data during the
> > > > > > build.
> > > > > >
> > > > > > - Arnaldo
> > > > > >
> > > > > > From 882adaee50bc27f85374aeb2fbaa5b76bef60d05 Mon Sep 17 00:00:00 2001
> > > > > > From: Arnaldo Carvalho de Melo <[email protected]>
> > > > > > Date: Thu, 4 May 2023 19:03:51 -0300
> > > > > > Subject: [PATCH 1/1] perf bpf skels: Stop using vmlinux.h generated from BTF,
> > > > > > use subset of used structs + CO-RE
> > > > > >
> > > > > > Linus reported a build break due to using a vmlinux without a BTF elf
> > > > > > section to generate the vmlinux.h header with bpftool for use in the BPF
> > > > > > tools in tools/perf/util/bpf_skel/*.bpf.c.
> > > > > >
> > > > > > Instead add a vmlinux.h file with the structs needed with the fields the
> > > > > > tools need, marking the structs with __attribute__((preserve_access_index)),
> > > > > > so that libbpf's CO-RE code can fixup the struct field offsets.
> > > > > >
> > > > > > In some cases the vmlinux.h file that was being generated by bpftool
> > > > > > from the kernel BTF information was not needed at all, just including
> > > > > > linux/bpf.h, sometimes linux/perf_event.h was enough as non-UAPI
> > > > > > types were not being used.
> > > > > >
> > > > > > To keep te patch small, include those UAPI headers from the trimmed down
> > > > > > vmlinux.h file, that then provides the tools with just the structs and
> > > > > > the subset of its fields needed for them.
> > > > > >
> > > > > > Testing it:
> > > > > >
> > > > > > # perf lock contention -b find / > /dev/null
> > > >
> > > > I tested perf lock con -abv -L rcu_state sleep 1
> > > > and needed fix below
> > > >
> > > > jirka
> > >
> > > I thought this was fixed by:
> > > https://lore.kernel.org/lkml/[email protected]/
> > > but I think that is just in perf-tools-next.
> >
> > ah ok, missed that one
>
> Please try validating with veristat to check if all of perf's .bpf.o
> files are successful. Veristat is part of selftests and can be built
> with just `make -C tools/testing/selftests/bpf veristat`. After that;
>
> sudo ~/bin/veristat tools/perf/util/bpf_skel/.tmp/*.bpf.o
>
> This is a surer way to check that BPF object files are ok at least on
> your currently running kernel, than trying to exercise each BPF
> program through perf commands.
[acme@quaco perf-tools]$ sudo tools/testing/selftests/bpf/veristat /tmp/build/perf-tools/util/bpf_skel/.tmp/*.bpf.o
Processing 'bperf_cgroup.bpf.o'...
Processing 'bperf_follower.bpf.o'...
Processing 'bperf_leader.bpf.o'...
Processing 'bpf_prog_profiler.bpf.o'...
Processing 'func_latency.bpf.o'...
Processing 'kwork_trace.bpf.o'...
Processing 'lock_contention.bpf.o'...
Processing 'off_cpu.bpf.o'...
Processing 'sample_filter.bpf.o'...
File Program Verdict Duration (us) Insns States Peak states
----------------------- ------------------------------- ------- ------------- ------ ------ -----------
bperf_cgroup.bpf.o on_cgrp_switch success 6479 17025 417 174
bperf_cgroup.bpf.o trigger_read success 6370 17025 417 174
bperf_follower.bpf.o fexit_XXX failure 0 0 0 0
bperf_leader.bpf.o on_switch success 360 49 3 3
bpf_prog_profiler.bpf.o fentry_XXX failure 0 0 0 0
bpf_prog_profiler.bpf.o fexit_XXX failure 0 0 0 0
func_latency.bpf.o func_begin success 351 69 6 6
func_latency.bpf.o func_end success 318 158 15 15
kwork_trace.bpf.o latency_softirq_entry success 334 108 10 10
kwork_trace.bpf.o latency_softirq_raise success 896 1993 34 34
kwork_trace.bpf.o latency_workqueue_activate_work success 333 46 4 4
kwork_trace.bpf.o latency_workqueue_execute_start success 1112 2219 41 41
kwork_trace.bpf.o report_irq_handler_entry success 1067 2118 34 34
kwork_trace.bpf.o report_irq_handler_exit success 334 110 10 10
kwork_trace.bpf.o report_softirq_entry success 897 1993 34 34
kwork_trace.bpf.o report_softirq_exit success 329 108 10 10
kwork_trace.bpf.o report_workqueue_execute_end success 1124 2219 41 41
kwork_trace.bpf.o report_workqueue_execute_start success 295 46 4 4
lock_contention.bpf.o collect_lock_syms failure 0 0 0 0
lock_contention.bpf.o contention_begin failure 0 0 0 0
lock_contention.bpf.o contention_end failure 0 0 0 0
off_cpu.bpf.o on_newtask success 387 37 3 3
off_cpu.bpf.o on_switch success 536 220 20 20
sample_filter.bpf.o perf_sample_filter success 190443 190237 11173 923
----------------------- ------------------------------- ------- ------------- ------ ------ -----------
Done. Processed 9 files, 0 programs. Skipped 24 files, 0 programs.
[acme@quaco perf-tools]$
What extra info can we get from these "failure" lines?
- Arnaldo
On Fri, May 5, 2023 at 2:52 PM Arnaldo Carvalho de Melo <[email protected]> wrote:
>
> Em Fri, May 05, 2023 at 02:21:56PM -0700, Andrii Nakryiko escreveu:
> > On Fri, May 5, 2023 at 2:15 PM Jiri Olsa <[email protected]> wrote:
> > >
> > > On Fri, May 05, 2023 at 01:46:30PM -0700, Ian Rogers wrote:
> > > > On Fri, May 5, 2023 at 1:43 PM Jiri Olsa <[email protected]> wrote:
> > > > >
> > > > > On Fri, May 05, 2023 at 10:04:47AM -0700, Ian Rogers wrote:
> > > > > > On Fri, May 5, 2023 at 9:56 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > > > > > >
> > > > > > > Em Fri, May 05, 2023 at 10:33:15AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > > > > Em Fri, May 05, 2023 at 01:03:14AM +0200, Jiri Olsa escreveu:
> > > > > > > > That with the preserve_access_index isn't needed, we need just the
> > > > > > > > fields that we access in the tools, right?
> > > > > > >
> > > > > > > I'm now doing build test this in many distro containers, without the two
> > > > > > > reverts, i.e. BPF skels continue as opt-out as in my pull request, to
> > > > > > > test build and also for the functionality tests on the tools using such
> > > > > > > bpf skels, see below, no touching of vmlinux nor BTF data during the
> > > > > > > build.
> > > > > > >
> > > > > > > - Arnaldo
> > > > > > >
> > > > > > > From 882adaee50bc27f85374aeb2fbaa5b76bef60d05 Mon Sep 17 00:00:00 2001
> > > > > > > From: Arnaldo Carvalho de Melo <[email protected]>
> > > > > > > Date: Thu, 4 May 2023 19:03:51 -0300
> > > > > > > Subject: [PATCH 1/1] perf bpf skels: Stop using vmlinux.h generated from BTF,
> > > > > > > use subset of used structs + CO-RE
> > > > > > >
> > > > > > > Linus reported a build break due to using a vmlinux without a BTF elf
> > > > > > > section to generate the vmlinux.h header with bpftool for use in the BPF
> > > > > > > tools in tools/perf/util/bpf_skel/*.bpf.c.
> > > > > > >
> > > > > > > Instead add a vmlinux.h file with the structs needed with the fields the
> > > > > > > tools need, marking the structs with __attribute__((preserve_access_index)),
> > > > > > > so that libbpf's CO-RE code can fixup the struct field offsets.
> > > > > > >
> > > > > > > In some cases the vmlinux.h file that was being generated by bpftool
> > > > > > > from the kernel BTF information was not needed at all, just including
> > > > > > > linux/bpf.h, sometimes linux/perf_event.h was enough as non-UAPI
> > > > > > > types were not being used.
> > > > > > >
> > > > > > > To keep te patch small, include those UAPI headers from the trimmed down
> > > > > > > vmlinux.h file, that then provides the tools with just the structs and
> > > > > > > the subset of its fields needed for them.
> > > > > > >
> > > > > > > Testing it:
> > > > > > >
> > > > > > > # perf lock contention -b find / > /dev/null
> > > > >
> > > > > I tested perf lock con -abv -L rcu_state sleep 1
> > > > > and needed fix below
> > > > >
> > > > > jirka
> > > >
> > > > I thought this was fixed by:
> > > > https://lore.kernel.org/lkml/[email protected]/
> > > > but I think that is just in perf-tools-next.
> > >
> > > ah ok, missed that one
> >
> > Please try validating with veristat to check if all of perf's .bpf.o
> > files are successful. Veristat is part of selftests and can be built
> > with just `make -C tools/testing/selftests/bpf veristat`. After that;
> >
> > sudo ~/bin/veristat tools/perf/util/bpf_skel/.tmp/*.bpf.o
> >
> > This is a surer way to check that BPF object files are ok at least on
> > your currently running kernel, than trying to exercise each BPF
> > program through perf commands.
>
> [acme@quaco perf-tools]$ sudo tools/testing/selftests/bpf/veristat /tmp/build/perf-tools/util/bpf_skel/.tmp/*.bpf.o
> Processing 'bperf_cgroup.bpf.o'...
> Processing 'bperf_follower.bpf.o'...
> Processing 'bperf_leader.bpf.o'...
> Processing 'bpf_prog_profiler.bpf.o'...
> Processing 'func_latency.bpf.o'...
> Processing 'kwork_trace.bpf.o'...
> Processing 'lock_contention.bpf.o'...
> Processing 'off_cpu.bpf.o'...
> Processing 'sample_filter.bpf.o'...
> File Program Verdict Duration (us) Insns States Peak states
> ----------------------- ------------------------------- ------- ------------- ------ ------ -----------
> bperf_cgroup.bpf.o on_cgrp_switch success 6479 17025 417 174
> bperf_cgroup.bpf.o trigger_read success 6370 17025 417 174
> bperf_follower.bpf.o fexit_XXX failure 0 0 0 0
> bperf_leader.bpf.o on_switch success 360 49 3 3
> bpf_prog_profiler.bpf.o fentry_XXX failure 0 0 0 0
> bpf_prog_profiler.bpf.o fexit_XXX failure 0 0 0 0
> func_latency.bpf.o func_begin success 351 69 6 6
> func_latency.bpf.o func_end success 318 158 15 15
> kwork_trace.bpf.o latency_softirq_entry success 334 108 10 10
> kwork_trace.bpf.o latency_softirq_raise success 896 1993 34 34
> kwork_trace.bpf.o latency_workqueue_activate_work success 333 46 4 4
> kwork_trace.bpf.o latency_workqueue_execute_start success 1112 2219 41 41
> kwork_trace.bpf.o report_irq_handler_entry success 1067 2118 34 34
> kwork_trace.bpf.o report_irq_handler_exit success 334 110 10 10
> kwork_trace.bpf.o report_softirq_entry success 897 1993 34 34
> kwork_trace.bpf.o report_softirq_exit success 329 108 10 10
> kwork_trace.bpf.o report_workqueue_execute_end success 1124 2219 41 41
> kwork_trace.bpf.o report_workqueue_execute_start success 295 46 4 4
> lock_contention.bpf.o collect_lock_syms failure 0 0 0 0
> lock_contention.bpf.o contention_begin failure 0 0 0 0
> lock_contention.bpf.o contention_end failure 0 0 0 0
> off_cpu.bpf.o on_newtask success 387 37 3 3
> off_cpu.bpf.o on_switch success 536 220 20 20
> sample_filter.bpf.o perf_sample_filter success 190443 190237 11173 923
> ----------------------- ------------------------------- ------- ------------- ------ ------ -----------
> Done. Processed 9 files, 0 programs. Skipped 24 files, 0 programs.
> [acme@quaco perf-tools]$
>
> What extra info can we get from these "failure" lines?
you can ask for verifier log with -v (or very detailed verifier log
with -vl2). And you can narrow down to single program at a time with
-f <prog_name>:
sudo ./veristat -v <path-to-bpf.o> -f <prog_name>
Not every possible program can be correctly loaded by veristat (e.g.,
if fentry/fexit doesn't specify target function). But in the above
case everything from lock_contention.bpf.o is definitely worth
checking.
>
> - Arnaldo
Em Fri, May 05, 2023 at 01:48:52PM -0700, Namhyung Kim escreveu:
> On Fri, May 5, 2023 at 1:46 PM Ian Rogers <[email protected]> wrote:
> >
> > On Fri, May 5, 2023 at 1:43 PM Jiri Olsa <[email protected]> wrote:
> > >
> > > On Fri, May 05, 2023 at 10:04:47AM -0700, Ian Rogers wrote:
> > > > On Fri, May 5, 2023 at 9:56 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > > > >
> > > > > Em Fri, May 05, 2023 at 10:33:15AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > > Em Fri, May 05, 2023 at 01:03:14AM +0200, Jiri Olsa escreveu:
> > > > > > That with the preserve_access_index isn't needed, we need just the
> > > > > > fields that we access in the tools, right?
> > > > >
> > > > > I'm now doing build test this in many distro containers, without the two
> > > > > reverts, i.e. BPF skels continue as opt-out as in my pull request, to
> > > > > test build and also for the functionality tests on the tools using such
> > > > > bpf skels, see below, no touching of vmlinux nor BTF data during the
> > > > > build.
> > > > >
> > > > > - Arnaldo
> > > > >
> > > > > From 882adaee50bc27f85374aeb2fbaa5b76bef60d05 Mon Sep 17 00:00:00 2001
> > > > > From: Arnaldo Carvalho de Melo <[email protected]>
> > > > > Date: Thu, 4 May 2023 19:03:51 -0300
> > > > > Subject: [PATCH 1/1] perf bpf skels: Stop using vmlinux.h generated from BTF,
> > > > > use subset of used structs + CO-RE
> > > > >
> > > > > Linus reported a build break due to using a vmlinux without a BTF elf
> > > > > section to generate the vmlinux.h header with bpftool for use in the BPF
> > > > > tools in tools/perf/util/bpf_skel/*.bpf.c.
> > > > >
> > > > > Instead add a vmlinux.h file with the structs needed with the fields the
> > > > > tools need, marking the structs with __attribute__((preserve_access_index)),
> > > > > so that libbpf's CO-RE code can fixup the struct field offsets.
> > > > >
> > > > > In some cases the vmlinux.h file that was being generated by bpftool
> > > > > from the kernel BTF information was not needed at all, just including
> > > > > linux/bpf.h, sometimes linux/perf_event.h was enough as non-UAPI
> > > > > types were not being used.
> > > > >
> > > > > To keep te patch small, include those UAPI headers from the trimmed down
> > > > > vmlinux.h file, that then provides the tools with just the structs and
> > > > > the subset of its fields needed for them.
> > > > >
> > > > > Testing it:
> > > > >
> > > > > # perf lock contention -b find / > /dev/null
> > >
> > > I tested perf lock con -abv -L rcu_state sleep 1
> > > and needed fix below
> > >
> > > jirka
> >
> > I thought this was fixed by:
> > https://lore.kernel.org/lkml/[email protected]/
Those are upstream already:
⬢[acme@toolbox perf-tools]$ git log --oneline torvalds/master | grep -m1 -B1 "perf lock contention: Fix struct rq lock access"
b9f82b5c63bf5390 perf lock contention: Rework offset calculation with BPF CO-RE
e53de7b65a3ca59a perf lock contention: Fix struct rq lock access
⬢[acme@toolbox perf-tools]$
> > but I think that is just in perf-tools-next.
> Right, but we might still need the empty rq definition.
Yeah, without the empty struct diff libbpf complains about a mismatch of
just a forward declaration as the type for 'runqueues' on the
lock_contention.bpf.c file while the kernel has a the type as 'struct
rq':
[root@quaco ~]# perf lock con -ab sleep 1
libbpf: extern (var ksym) 'runqueues': incompatible types, expected [95] fwd rq, but kernel has [55509] struct rq
libbpf: failed to load object 'lock_contention_bpf'
libbpf: failed to load BPF skeleton 'lock_contention_bpf': -22
Failed to load lock-contention BPF skeleton
lock contention BPF setup failed
[root@quaco ~]#
Adding:
struct rq {};
libbpf is happy:
[root@quaco ~]# perf lock con -ab sleep 1
contended total wait max wait avg wait type caller
2 50.64 us 25.38 us 25.32 us spinlock tick_do_update_jiffies64+0x25
1 26.18 us 26.18 us 26.18 us spinlock tick_do_update_jiffies64+0x25
[root@quaco ~]#
- Arnaldo