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@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.
>
> Signed-off-by: Song Liu <songliubrav...@fb.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
> ---
> 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
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
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
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
thanks,
jirka
