Re: [PATCH 2/2] perf-stat: introduce config stat.bpf-counter-events

[Song Liu <songliubraving@fb.com>]

> On Apr 6, 2021, at 7:21 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> 
> On Fri, Apr 02, 2021 at 05:29:38PM -0700, Song Liu wrote:
>> Currently, to use BPF to aggregate perf event counters, the user uses
>> --bpf-counters option. Enable "use bpf by default" events with a config
>> option, stat.bpf-counter-events. This is limited to hardware events in
>> evsel__hw_names.
>> 
>> This also enables mixed BPF event and regular event in the same sesssion.
>> For example:
>> 
>>   perf config stat.bpf-counter-events=instructions
>>   perf stat -e instructions,cs
> 
> hum, so this will effectively allow to mix 'bpf-shared' counters
> with normals ones.. I don't think we're ready for that ;-)

I think we are ready. :) all bpf_counter stuff is within evsel, so mixing 
them doesn't need much work. 

> 
>> 
>> The second command will use BPF for "instructions" but not "cs".
>> 
>> Signed-off-by: Song Liu <song@kernel.org>
>> ---
>> tools/perf/Documentation/perf-stat.txt |  2 ++
>> tools/perf/builtin-stat.c              | 41 ++++++++++++++++----------
>> tools/perf/util/bpf_counter.c          | 11 +++++++
>> tools/perf/util/config.c               | 32 ++++++++++++++++++++
>> tools/perf/util/evsel.c                |  2 ++
>> tools/perf/util/evsel.h                |  1 +
>> tools/perf/util/target.h               |  5 ----
>> 7 files changed, 74 insertions(+), 20 deletions(-)
>> 
>> diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
>> index 744211fa8c186..6d4733eaac170 100644
>> --- a/tools/perf/Documentation/perf-stat.txt
>> +++ b/tools/perf/Documentation/perf-stat.txt
>> @@ -97,6 +97,8 @@ report::
>> 	Use BPF programs to aggregate readings from perf_events.  This
>> 	allows multiple perf-stat sessions that are counting the same metric (cycles,
>> 	instructions, etc.) to share hardware counters.
>> +	To use BPF programs on common hardware events by default, use
>> +	"perf config stat.bpf-counter-events=<list_of_events>".
>> 
>> --bpf-attr-map::
>> 	With option "--bpf-counters", different perf-stat sessions share
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index 4bb48c6b66980..5adfa708ffe68 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -423,17 +423,28 @@ static int read_affinity_counters(struct timespec *rs)
>> 	return 0;
>> }
>> 
>> +/*
>> + * Returns:
>> + *     0   if all events use BPF;
>> + *     1   if some events do NOT use BPF;
>> + *     < 0 on errors;
>> + */
>> static int read_bpf_map_counters(void)
>> {
>> +	bool has_none_bpf_events = false;
>> 	struct evsel *counter;
>> 	int err;
>> 
>> 	evlist__for_each_entry(evsel_list, counter) {
>> +		if (!counter->bpf_counter_ops) {
>> +			has_none_bpf_events = true;
>> +			continue;
>> +		}
>> 		err = bpf_counter__read(counter);
>> 		if (err)
>> 			return err;
>> 	}
>> -	return 0;
>> +	return has_none_bpf_events ? 1 : 0;
>> }
>> 
>> static void read_counters(struct timespec *rs)
>> @@ -442,9 +453,10 @@ static void read_counters(struct timespec *rs)
>> 	int err;
>> 
>> 	if (!stat_config.stop_read_counter) {
>> -		if (target__has_bpf(&target))
>> -			err = read_bpf_map_counters();
>> -		else
>> +		err = read_bpf_map_counters();
>> +		if (err < 0)
>> +			return;
>> +		if (err)
>> 			err = read_affinity_counters(rs);
> 
> so read_affinity_counters will read also 'bpf-shared' counters no?
> as long as it was separated, I did not see a problem, now we have
> counters that either have bpf ops set or have not
> 
> it'd be great to do some generic separation.. I was thinking to move
> bpf_counter_ops into some generic counter ops and we would just fill
> in the proper ops for the counter.. buuut the affinity readings are
> not compatible with what we are doing in bperf_read and the profiler
> bpf read
> 
> so I think the solution will be just to skip those events in
> read_affinity_counters and all the other code, and have some
> helper like:
> 
>   bool evsel__is_bpf(evsel)
> 
> so it's clear why it's skipped

Yes, this will be better! Current version does have the problem of 
extra read in read_affinity_counters(). Will fix this. 

Thanks,
Song