Messages in this thread | | | From | Song Liu <> | Subject | Re: [PATCH 2/2] perf-stat: introduce config stat.bpf-counter-events | Date | Tue, 6 Apr 2021 21:46:28 +0000 |
| |
> 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
| |