From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Song Liu <songliubraving@fb.com>
Cc: lkml <linux-kernel@vger.kernel.org>,
"peterz@infradead.org" <peterz@infradead.org>,
"mingo@redhat.com" <mingo@redhat.com>,
"alexander.shishkin@linux.intel.com"
<alexander.shishkin@linux.intel.com>,
"namhyung@kernel.org" <namhyung@kernel.org>,
"mark.rutland@arm.com" <mark.rutland@arm.com>,
"jolsa@redhat.com" <jolsa@redhat.com>,
Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH v6 3/4] perf-stat: enable counting events for BPF programs
Date: Tue, 29 Dec 2020 16:18:48 -0300 [thread overview]
Message-ID: <20201229191848.GL521329@kernel.org> (raw)
In-Reply-To: <9BDC4556-E802-4152-91E1-1A4696F62AAE@fb.com>
Em Tue, Dec 29, 2020 at 07:11:12PM +0000, Song Liu escreveu:
>
>
> > On Dec 29, 2020, at 10:48 AM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> >
> > Em Tue, Dec 29, 2020 at 06:42:18PM +0000, Song Liu escreveu:
> >>
> >>
> >>> On Dec 29, 2020, at 7:15 AM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> >>>
> >>> Em Mon, Dec 28, 2020 at 11:43:25PM +0000, Song Liu escreveu:
> >>>>
> >>>>
> >>>>> On Dec 28, 2020, at 12:11 PM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> >>>>>
> >>>>> Em Mon, Dec 28, 2020 at 09:40:53AM -0800, Song Liu escreveu:
> >>>>>> Introduce perf-stat -b option, which counts events for BPF programs, like:
> >>>>>>
> >>>>>> [root@localhost ~]# ~/perf stat -e ref-cycles,cycles -b 254 -I 1000
> >>>>>> 1.487903822 115,200 ref-cycles
> >>>>>> 1.487903822 86,012 cycles
> >>>>>> 2.489147029 80,560 ref-cycles
> >>>>>> 2.489147029 73,784 cycles
> >>>>>> 3.490341825 60,720 ref-cycles
> >>>>>> 3.490341825 37,797 cycles
> >>>>>> 4.491540887 37,120 ref-cycles
> >>>>>> 4.491540887 31,963 cycles
> >>>>>>
> >>>>>> The example above counts cycles and ref-cycles of BPF program of id 254.
> >>>>>> This is similar to bpftool-prog-profile command, but more flexible.
> >>>>>>
> >>>>>> perf-stat -b creates per-cpu perf_event and loads fentry/fexit BPF
> >>>>>> programs (monitor-progs) to the target BPF program (target-prog). The
> >>>>>> monitor-progs read perf_event before and after the target-prog, and
> >>>>>> aggregate the difference in a BPF map. Then the user space reads data
> >>>>>> from these maps.
> >>>>>>
> >>>>>> A new struct bpf_counter is introduced to provide common interface that
> >>>>>> uses BPF programs/maps to count perf events.
> >>>>>
> >>>>> Segfaulting here:
> >>>>>
> >>>>> [root@five ~]# bpftool prog | grep tracepoint
> >>>>> 110: tracepoint name syscall_unaugme tag 57cd311f2e27366b gpl
> >>>>> 111: tracepoint name sys_enter_conne tag 3555418ac9476139 gpl
> >>>>> 112: tracepoint name sys_enter_sendt tag bc7fcadbaf7b8145 gpl
> >>>>> 113: tracepoint name sys_enter_open tag 0e59c3ac2bea5280 gpl
> >>>>> 114: tracepoint name sys_enter_opena tag 0baf443610f59837 gpl
> >>>>> 115: tracepoint name sys_enter_renam tag 24664e4aca62d7fa gpl
> >>>>> 116: tracepoint name sys_enter_renam tag 20093e51a8634ebb gpl
> >>>>> 117: tracepoint name sys_enter tag 0bc3fc9d11754ba1 gpl
> >>>>> 118: tracepoint name sys_exit tag 29c7ae234d79bd5c gpl
> >>>>> [root@five ~]#
> >>>>> [root@five ~]# gdb perf
> >>>>> GNU gdb (GDB) Fedora 10.1-2.fc33
> >>>>> Reading symbols from perf...
> >>>>> (gdb) run stat -e instructions,cycles -b 113 -I 1000
> >>>>> Starting program: /root/bin/perf stat -e instructions,cycles -b 113 -I 1000
> >>>>> [Thread debugging using libthread_db enabled]
> >>>>> Using host libthread_db library "/lib64/libthread_db.so.1".
> >>>>> libbpf: elf: skipping unrecognized data section(9) .eh_frame
> >>>>> libbpf: elf: skipping relo section(15) .rel.eh_frame for section(9) .eh_frame
> >>>>> libbpf: elf: skipping unrecognized data section(9) .eh_frame
> >>>>> libbpf: elf: skipping relo section(15) .rel.eh_frame for section(9) .eh_frame
> >>>>>
> >>>>> Program received signal SIGSEGV, Segmentation fault.
> >>>>> 0x000000000058d55b in bpf_program_profiler__read (evsel=0xc612c0) at util/bpf_counter.c:217
> >>>>> 217 reading_map_fd = bpf_map__fd(skel->maps.accum_readings);
> >>>>> (gdb) bt
> >>>>> #0 0x000000000058d55b in bpf_program_profiler__read (evsel=0xc612c0) at util/bpf_counter.c:217
> >>>>> #1 0x0000000000000000 in ?? ()
> >>>>> (gdb)
> >>>>>
> >>>>> [acme@five perf]$ clang -v |& head -2
> >>>>> clang version 11.0.0 (Fedora 11.0.0-2.fc33)
> >>>>> Target: x86_64-unknown-linux-gnu
> >>>>> [acme@five perf]$
> >>>>>
> >>>>> Do you need any extra info?
> >>>>
> >>>> Hmm... I am not able to reproduce this. I am trying to setup an environment similar
> >>>> to fc33 (clang 11, etc.). Does this segfault every time, and on all programs?
> >>>
> >>> I'll try it with a BPF proggie attached to a kprobes, but here is
> >>> something else I noticed:
> >>>
> >>> [root@five perf]# export PYTHONPATH=/tmp/build/perf/python
> >>> [root@five perf]# tools/perf/python/twatch.py
> >>> Traceback (most recent call last):
> >>> File "/home/acme/git/perf/tools/perf/python/twatch.py", line 9, in <module>
> >>> import perf
> >>> ImportError: /tmp/build/perf/python/perf.cpython-39-x86_64-linux-gnu.so: undefined symbol: bpf_counter__destroy
> >>> [root@five perf]# perf test python
> >>> 19: 'import perf' in python : FAILED!
> >>> [root@five perf]# perf test -v python
> >>> 19: 'import perf' in python :
> >>> --- start ---
> >>> test child forked, pid 3198864
> >>> python usage test: "echo "import sys ; sys.path.append('/tmp/build/perf/python'); import perf" | '/usr/bin/python3' "
> >>> Traceback (most recent call last):
> >>> File "<stdin>", line 1, in <module>
> >>> ImportError: /tmp/build/perf/python/perf.cpython-39-x86_64-linux-gnu.so: undefined symbol: bpf_counter__destroy
> >>> test child finished with -1
> >>> ---- end ----
> >>> 'import perf' in python: FAILED!
> >>> [root@five perf]#
> >>>
> >>> This should be trivial, I hope, just add the new object file to
> >>> tools/perf/util/python-ext-sources, then do a 'perf test python', if it
> >>> fails, use 'perf test -v python' to see what is preventing the python
> >>> binding from loading.
> >>
> >> I fixed the undefined bpf_counter__destroy. But this one looks trickier:
> >>
> >> 19: 'import perf' in python :
> >> --- start ---
> >> test child forked, pid 2714986
> >> python usage test: "echo "import sys ; sys.path.append('python'); import perf" | '/bin/python2' "
> >> Traceback (most recent call last):
> >> File "<stdin>", line 1, in <module>
> >> ImportError: XXXXX /tools/perf/python/perf.so: undefined symbol: bpf_map_update_elem
> >>
> >> Given I already have:
> >
> > I'll check this one to get a patch that at least moves the needle here,
> > i.e. probably we can leave supporting bpf counters in the python binding
> > for a later step.
>
> Thanks Arnaldo!
>
> Currently, I have:
> 1. Fixed issues highlighted by Namhyung;
> 2. Merged 3/4 and 4/4;
> 3. NOT found segfault;
> 4. NOT fixed python import perf.
>
> I don't have good ideas with 3 and 4... Shall I send current code as v7?
For 4, please fold the patch below into the relevant patch, we don't
need bpf_counter.h included in util/evsel.h, you even added a forward
declaration for that 'struct bpf_counter_ops'.
And in general we should refrain from adding extra includes to header
files, .h-ell is not good.
Then we provide a stub for that bpf_counter__destroy() so that
util/evsel.o when linked into the perf python biding find it there,
links ok.
As we don't have a way to create such events via the perf python
binding, there will nothing to be done when destroying evsels created
via python.
- Arnaldo
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 40e3946cd7518113..8226b1fefa8cf2a3 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -10,7 +10,6 @@
#include <internal/evsel.h>
#include <perf/evsel.h>
#include "symbol_conf.h"
-#include "bpf_counter.h"
#include <internal/cpumap.h>
struct bpf_object;
diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index cc5ade85a33fc999..9609cc166d71a6f5 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -79,6 +79,21 @@ int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
return 0;
}
+/*
+ * XXX: All these evsel destructors need some better mechanism, like a linked
+ * list of destructors registered when the relevant code indeed is used instead
+ * of having more and more calls in perf_evsel__delete(). -- acme
+ *
+ * For now, add one more:
+ *
+ * Not to drag the BPF bandwagon...
+ */
+void bpf_counter__destroy(struct evsel *evsel);
+
+void bpf_counter__destroy(struct evsel *evsel __maybe_unused)
+{
+}
+
/*
* Support debug printing even though util/debug.c is not linked. That means
* implementing 'verbose' and 'eprintf'.
next prev parent reply other threads:[~2020-12-29 19:19 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-28 17:40 [PATCH v6 0/4] Introduce perf-stat -b for BPF programs Song Liu
2020-12-28 17:40 ` [PATCH v6 1/4] bpftool: add Makefile target bootstrap Song Liu
2020-12-28 17:40 ` [PATCH v6 2/4] perf: support build BPF skeletons with perf Song Liu
2020-12-29 7:01 ` Namhyung Kim
2020-12-29 11:48 ` Arnaldo Carvalho de Melo
2020-12-29 17:14 ` Song Liu
2020-12-29 18:16 ` Arnaldo Carvalho de Melo
2020-12-28 17:40 ` [PATCH v6 3/4] perf-stat: enable counting events for BPF programs Song Liu
2020-12-28 20:11 ` Arnaldo Carvalho de Melo
2020-12-28 23:43 ` Song Liu
2020-12-29 5:53 ` Song Liu
2020-12-29 15:15 ` Arnaldo Carvalho de Melo
2020-12-29 18:42 ` Song Liu
2020-12-29 18:48 ` Arnaldo Carvalho de Melo
2020-12-29 19:11 ` Song Liu
2020-12-29 19:18 ` Arnaldo Carvalho de Melo [this message]
2020-12-29 19:23 ` Arnaldo Carvalho de Melo
2020-12-29 19:32 ` Arnaldo Carvalho de Melo
2020-12-29 21:40 ` Song Liu
2020-12-29 7:22 ` Namhyung Kim
2020-12-29 17:46 ` Song Liu
2020-12-29 17:59 ` Song Liu
2020-12-28 17:40 ` [PATCH v6 4/4] perf-stat: add documentation for -b option Song Liu
2020-12-29 7:24 ` Namhyung Kim
2020-12-29 16:59 ` Song Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201229191848.GL521329@kernel.org \
--to=acme@kernel.org \
--cc=Kernel-team@fb.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=songliubraving@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.