From: Song Liu <songliubraving@fb.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Song Liu <song@kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Kernel Team <Kernel-team@fb.com>,
"acme@kernel.org" <acme@kernel.org>,
"acme@redhat.com" <acme@redhat.com>,
"namhyung@kernel.org" <namhyung@kernel.org>,
"jolsa@kernel.org" <jolsa@kernel.org>
Subject: Re: [PATCH 2/2] perf-stat: introduce config stat.bpf-counter-events
Date: Tue, 6 Apr 2021 21:46:28 +0000 [thread overview]
Message-ID: <8970DB18-42B1-44A0-84E9-5C317245F453@fb.com> (raw)
In-Reply-To: <YGxue1aHQUFj+UaG@krava>
> 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
next prev parent reply other threads:[~2021-04-06 21:46 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-03 0:29 [PATCH 1/2] perf util: move bperf definitions to a libperf header Song Liu
2021-04-03 0:29 ` [PATCH 2/2] perf-stat: introduce config stat.bpf-counter-events Song Liu
2021-04-06 14:21 ` Jiri Olsa
2021-04-06 21:46 ` Song Liu [this message]
2021-04-06 14:21 ` [PATCH 1/2] perf util: move bperf definitions to a libperf header Jiri Olsa
2021-04-06 16:26 ` Song Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8970DB18-42B1-44A0-84E9-5C317245F453@fb.com \
--to=songliubraving@fb.com \
--cc=Kernel-team@fb.com \
--cc=acme@kernel.org \
--cc=acme@redhat.com \
--cc=jolsa@kernel.org \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=namhyung@kernel.org \
--cc=song@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).