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

Song Liu
2021-04-06T21:46:45Z

> 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