Re: [PATCH v3 2/2] perf-stat: enable counting events for BPF programs

[Song Liu]

> On Dec 9, 2020, at 9:03 AM, Jiri Olsa <jolsa@xxxxxxxxxx> 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 <songliubraving@xxxxxx>
> 
> 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