lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 10 Dec 2020 00:15:16 +0000 From: Song Liu <songliubraving@...com> To: Jiri Olsa <jolsa@...hat.com> CC: lkml <linux-kernel@...r.kernel.org>, Kernel Team <Kernel-team@...com>, "peterz@...radead.org" <peterz@...radead.org>, "mingo@...hat.com" <mingo@...hat.com>, "acme@...nel.org" <acme@...nel.org>, "mark.rutland@....com" <mark.rutland@....com>, "alexander.shishkin@...ux.intel.com" <alexander.shishkin@...ux.intel.com>, "namhyung@...nel.org" <namhyung@...nel.org> Subject: Re: [PATCH v3 2/2] perf-stat: enable counting events for BPF programs > On Dec 9, 2020, at 9:03 AM, Jiri Olsa <jolsa@...hat.com> wrote: > > On Tue, Dec 08, 2020 at 10:16:46AM -0800, Song Liu wrote: >> Introduce perf-stat -b option, which counts events for BPF programs, like: >> >> [root@...alhost ~]# ~/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. >> >> Signed-off-by: Song Liu <songliubraving@...com> > > I'm getting this at the end of the compilation: > > LINK perf > rm /home/jolsa/linux-perf/tools/perf/util/bpf_skel/.tmp/bpf_prog_profiler.bpf.o > > I guess we can keep it or make it silent somehow I also noticed this, but haven't figured out how to silent it. I guess we can fix it out later. > >> --- >> tools/perf/Makefile.perf | 2 +- >> tools/perf/builtin-stat.c | 77 ++++- >> tools/perf/util/Build | 1 + >> tools/perf/util/bpf_counter.c | 297 ++++++++++++++++++ >> tools/perf/util/bpf_counter.h | 73 +++++ >> .../util/bpf_skel/bpf_prog_profiler.bpf.c | 93 ++++++ >> tools/perf/util/evsel.c | 11 + >> tools/perf/util/evsel.h | 6 + >> tools/perf/util/stat-display.c | 4 +- >> tools/perf/util/target.c | 34 +- >> tools/perf/util/target.h | 10 + >> 11 files changed, 591 insertions(+), 17 deletions(-) >> create mode 100644 tools/perf/util/bpf_counter.c >> create mode 100644 tools/perf/util/bpf_counter.h >> create mode 100644 tools/perf/util/bpf_skel/bpf_prog_profiler.bpf.c > > we need man page update, would be great with some example How about we do this in a follow up patch? > > SNIP > >> - int status = -EINVAL, run_idx; >> + int status = -EINVAL, run_idx, err; >> const char *mode; >> FILE *output = stderr; >> unsigned int interval, timeout; >> const char * const stat_subcommands[] = { "record", "report" }; >> + char errbuf[BUFSIZ]; >> >> setlocale(LC_ALL, ""); >> >> @@ -2169,6 +2213,12 @@ int cmd_stat(int argc, const char **argv) >> } else if (big_num_opt == 0) /* User passed --no-big-num */ >> stat_config.big_num = false; >> >> + err = target__validate(&target); >> + if (err) { >> + target__strerror(&target, err, errbuf, BUFSIZ); >> + pr_warning("%s\n", errbuf); >> + } >> + > > is there a reason for this to move before setup_system_wide? > > I don't think it's a big deal, but just curious if that's intentional Yes, this is intentional. It is to fix "BPF switch overriding CPU" cases. Without moving target_validate(), we got # perf stat -e cycles,instructions -b 59 -C 0 BPF switch overriding CPU Error: The sys_perf_event_open() syscall returned with 9 (Bad file descriptor) for event (cycles). /bin/dmesg | grep -i perf may provide additional information. > > SNIP > >> + >> +int bpf_counter__enable(struct evsel *evsel) >> +{ >> + if (list_empty(&evsel->bpf_counter_list)) >> + return 0; >> + return evsel->bpf_counter_ops->enable(evsel); >> +} >> + >> +int bpf_counter__read(struct evsel *evsel) >> +{ >> + if (list_empty(&evsel->bpf_counter_list)) >> + return -EAGAIN; >> + return evsel->bpf_counter_ops->read(evsel); >> +} >> + >> +int bpf_counter__destroy(struct evsel *evsel) >> +{ > > this could return void Fixed in v4. > > SNIP > >> @@ -247,6 +252,7 @@ void evsel__init(struct evsel *evsel, >> evsel->bpf_obj = NULL; >> evsel->bpf_fd = -1; >> INIT_LIST_HEAD(&evsel->config_terms); >> + INIT_LIST_HEAD(&evsel->bpf_counter_list); >> perf_evsel__object.init(evsel); >> evsel->sample_size = __evsel__sample_size(attr->sample_type); >> evsel__calc_id_pos(evsel); >> @@ -1365,6 +1371,7 @@ void evsel__exit(struct evsel *evsel) >> { >> assert(list_empty(&evsel->core.node)); >> assert(evsel->evlist == NULL); >> + bpf_counter__destroy(evsel); >> evsel__free_counts(evsel); >> perf_evsel__free_fd(&evsel->core); >> perf_evsel__free_id(&evsel->core); >> @@ -1770,6 +1777,8 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus, >> evsel->core.attr.sample_id_all = 0; >> >> display_attr(&evsel->core.attr); >> + if (!list_empty(&evsel->bpf_counter_list)) >> + evsel->core.attr.inherit = 0; > > I think this should go to evsel__config where we set all attr bits evsel__config() is not called by perf-stat. I moved the logic to create_perf_stat_counter() instead. Thanks, Song
Powered by blists - more mailing lists