> On Dec 9, 2020, at 9:03 AM, Jiri Olsa <jo...@redhat.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@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
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
