Re: [PATCH v3 3/4] perf-stat: introduce config stat.bpf-counter-events
On Mon, Apr 19, 2021 at 08:26:02PM +, Song Liu wrote:
>
>
> > On Apr 17, 2021, at 9:45 AM, Namhyung Kim wrote:
> >
> > Hi Song,
> >
> > On Sat, Apr 17, 2021 at 7:13 AM 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. Events with name in the option will use
> >> BPF.
> >>
> >> 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
> >>
> >> The second command will use BPF for "instructions" but not "cs".
> >>
> >> Signed-off-by: Song Liu
> >> ---
> >> @@ -535,12 +549,13 @@ static int enable_counters(void)
> >>struct evsel *evsel;
> >>int err;
> >>
> >> - if (target__has_bpf()) {
> >> - evlist__for_each_entry(evsel_list, evsel) {
> >> - err = bpf_counter__enable(evsel);
> >> - if (err)
> >> - return err;
> >> - }
> >> + evlist__for_each_entry(evsel_list, evsel) {
> >> + if (!evsel__is_bpf(evsel))
> >> + continue;
> >> +
> >> + err = bpf_counter__enable(evsel);
> >> + if (err)
> >> + return err;
> >
> > I just realized it doesn't have a disable counterpart.
>
> I guess it is not really necessary for perf-stat? It is probably good
> to have it though. How about we do it in a follow up patch?
good catch, should it at least do:
evsel->follower_skel->bss->enabled = 0;
because then the follower goes down only with perf process, right?
still doing the counts till the end..?
also while checking on that I realized we open the counters
in separate path for this in bperf_reload_leader_program by
calling evsel__open_per_cpu.. and we're missing all the
fallback code and setup from create_perf_stat_counter
I think we should at least call create_perf_stat_counter,
and also propagate error value if it fails
jirka
>
> [...]
>
> >> + if (!evsel__bpf_counter_events)
> >> + return false;
> >> +
> >> + ptr = strstr(evsel__bpf_counter_events, name);
> >> + name_len = strlen(name);
> >> +
> >> + /* check name matches a full token in evsel__bpf_counter_events */
> >> + match = (ptr != NULL) &&
> >> + ((ptr == evsel__bpf_counter_events) || (*(ptr - 1) ==
> >> ',')) &&
> >> + ((*(ptr + name_len) == ',') || (*(ptr + name_len) ==
> >> '\0'));
> >
> > I'm not sure we have an event name which is a substring of another.
> > Maybe it can retry if it fails to match.
>
> We have ref-cycles and cycles. And some raw events may be substring of others?
>
> Thanks,
> Song
>
> [...]
>
Re: [PATCH v3 3/4] perf-stat: introduce config stat.bpf-counter-events
> On Apr 19, 2021, at 7:26 AM, Jiri Olsa wrote:
>
> On Fri, Apr 16, 2021 at 03:13:24PM -0700, Song Liu wrote:
>
> SNIP
>
>> +/*
>> + * 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 (!evsel__is_bpf(counter)) {
>> +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 +455,10 @@ static void read_counters(struct timespec *rs)
>> int err;
>>
>> if (!stat_config.stop_read_counter) {
>> -if (target__has_bpf())
>> -err = read_bpf_map_counters();
>> -else
>> +err = read_bpf_map_counters();
>> +if (err < 0)
>> +return;
>> +if (err)
>> err = read_affinity_counters(rs);
>
> this part is confusing for me.. I understand we don't want to enter
> read_affinity_counters when there's no bpf counter, so we don't set
> affinities in vain.. but there must be better way ;-)
>
>> diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
>> index 5de991ab46af9..3189b63714371 100644
>> --- a/tools/perf/util/bpf_counter.c
>> +++ b/tools/perf/util/bpf_counter.c
>> @@ -792,6 +792,8 @@ int bpf_counter__load(struct evsel *evsel, struct target
>> *target)
>> evsel->bpf_counter_ops = _program_profiler_ops;
>> else if (target->use_bpf)
>> evsel->bpf_counter_ops = _ops;
>> +else if (evsel__match_bpf_counter_events(evsel->name))
>> +evsel->bpf_counter_ops = _ops;
>
> please put this with the target->use_bpf check,
> it seems like it's another thing
Thanks for the review. Fixing these in v4.
Song
Re: [PATCH v3 3/4] perf-stat: introduce config stat.bpf-counter-events
> On Apr 17, 2021, at 9:45 AM, Namhyung Kim wrote:
>
> Hi Song,
>
> On Sat, Apr 17, 2021 at 7:13 AM 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. Events with name in the option will use
>> BPF.
>>
>> 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
>>
>> The second command will use BPF for "instructions" but not "cs".
>>
>> Signed-off-by: Song Liu
>> ---
>> @@ -535,12 +549,13 @@ static int enable_counters(void)
>>struct evsel *evsel;
>>int err;
>>
>> - if (target__has_bpf()) {
>> - evlist__for_each_entry(evsel_list, evsel) {
>> - err = bpf_counter__enable(evsel);
>> - if (err)
>> - return err;
>> - }
>> + evlist__for_each_entry(evsel_list, evsel) {
>> + if (!evsel__is_bpf(evsel))
>> + continue;
>> +
>> + err = bpf_counter__enable(evsel);
>> + if (err)
>> + return err;
>
> I just realized it doesn't have a disable counterpart.
I guess it is not really necessary for perf-stat? It is probably good
to have it though. How about we do it in a follow up patch?
[...]
>> + if (!evsel__bpf_counter_events)
>> + return false;
>> +
>> + ptr = strstr(evsel__bpf_counter_events, name);
>> + name_len = strlen(name);
>> +
>> + /* check name matches a full token in evsel__bpf_counter_events */
>> + match = (ptr != NULL) &&
>> + ((ptr == evsel__bpf_counter_events) || (*(ptr - 1) == ','))
>> &&
>> + ((*(ptr + name_len) == ',') || (*(ptr + name_len) == '\0'));
>
> I'm not sure we have an event name which is a substring of another.
> Maybe it can retry if it fails to match.
We have ref-cycles and cycles. And some raw events may be substring of others?
Thanks,
Song
[...]
Re: [PATCH v3 3/4] perf-stat: introduce config stat.bpf-counter-events
On Fri, Apr 16, 2021 at 03:13:24PM -0700, Song Liu wrote:
SNIP
> +/*
> + * 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 (!evsel__is_bpf(counter)) {
> + 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 +455,10 @@ static void read_counters(struct timespec *rs)
> int err;
>
> if (!stat_config.stop_read_counter) {
> - if (target__has_bpf())
> - err = read_bpf_map_counters();
> - else
> + err = read_bpf_map_counters();
> + if (err < 0)
> + return;
> + if (err)
> err = read_affinity_counters(rs);
this part is confusing for me.. I understand we don't want to enter
read_affinity_counters when there's no bpf counter, so we don't set
affinities in vain.. but there must be better way ;-)
> diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
> index 5de991ab46af9..3189b63714371 100644
> --- a/tools/perf/util/bpf_counter.c
> +++ b/tools/perf/util/bpf_counter.c
> @@ -792,6 +792,8 @@ int bpf_counter__load(struct evsel *evsel, struct target
> *target)
> evsel->bpf_counter_ops = _program_profiler_ops;
> else if (target->use_bpf)
> evsel->bpf_counter_ops = _ops;
> + else if (evsel__match_bpf_counter_events(evsel->name))
> + evsel->bpf_counter_ops = _ops;
please put this with the target->use_bpf check,
it seems like it's another thing
thanks,
jirka
Re: [PATCH v3 3/4] perf-stat: introduce config stat.bpf-counter-events
Hi Song,
On Sat, Apr 17, 2021 at 7:13 AM 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. Events with name in the option will use
> BPF.
>
> 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
>
> The second command will use BPF for "instructions" but not "cs".
>
> Signed-off-by: Song Liu
> ---
> @@ -535,12 +549,13 @@ static int enable_counters(void)
> struct evsel *evsel;
> int err;
>
> - if (target__has_bpf()) {
> - evlist__for_each_entry(evsel_list, evsel) {
> - err = bpf_counter__enable(evsel);
> - if (err)
> - return err;
> - }
> + evlist__for_each_entry(evsel_list, evsel) {
> + if (!evsel__is_bpf(evsel))
> + continue;
> +
> + err = bpf_counter__enable(evsel);
> + if (err)
> + return err;
I just realized it doesn't have a disable counterpart.
> }
>
> if (stat_config.initial_delay < 0) {
> @@ -784,11 +799,9 @@ static int __run_perf_stat(int argc, const char **argv,
> int run_idx)
> if (affinity__setup() < 0)
> return -1;
>
> - if (target__has_bpf()) {
> - evlist__for_each_entry(evsel_list, counter) {
> - if (bpf_counter__load(counter, ))
> - return -1;
> - }
> + evlist__for_each_entry(evsel_list, counter) {
> + if (bpf_counter__load(counter, ))
> + return -1;
> }
>
> evlist__for_each_cpu (evsel_list, i, cpu) {
[SNIP]
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 2d2614eeaa20e..080ddcfefbcd2 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -492,6 +492,28 @@ const char *evsel__hw_names[PERF_COUNT_HW_MAX] = {
> "ref-cycles",
> };
>
> +char *evsel__bpf_counter_events;
> +
> +bool evsel__match_bpf_counter_events(const char *name)
> +{
> + int name_len;
> + bool match;
> + char *ptr;
> +
> + if (!evsel__bpf_counter_events)
> + return false;
> +
> + ptr = strstr(evsel__bpf_counter_events, name);
> + name_len = strlen(name);
> +
> + /* check name matches a full token in evsel__bpf_counter_events */
> + match = (ptr != NULL) &&
> + ((ptr == evsel__bpf_counter_events) || (*(ptr - 1) == ',')) &&
> + ((*(ptr + name_len) == ',') || (*(ptr + name_len) == '\0'));
I'm not sure we have an event name which is a substring of another.
Maybe it can retry if it fails to match.
Thanks,
Namhyung
> +
> + return match;
> +}
> +
> static const char *__evsel__hw_name(u64 config)
> {
> if (config < PERF_COUNT_HW_MAX && evsel__hw_names[config])
[PATCH v3 3/4] perf-stat: introduce config stat.bpf-counter-events
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. Events with name in the option will use
BPF.
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
The second command will use BPF for "instructions" but not "cs".
Signed-off-by: Song Liu
---
tools/perf/Documentation/perf-stat.txt | 2 ++
tools/perf/builtin-stat.c | 43 +-
tools/perf/util/bpf_counter.c | 2 ++
tools/perf/util/config.c | 4 +++
tools/perf/util/evsel.c| 22 +
tools/perf/util/evsel.h| 8 +
tools/perf/util/target.h | 5 ---
7 files changed, 66 insertions(+), 20 deletions(-)
diff --git a/tools/perf/Documentation/perf-stat.txt
b/tools/perf/Documentation/perf-stat.txt
index 6ec5960b08c3d..78afe13cd7d47 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=".
--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 2a2c15cac80a3..0c76271f3ef53 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -413,6 +413,8 @@ static int read_affinity_counters(struct timespec *rs)
evlist__for_each_entry(evsel_list, counter) {
if (evsel__cpu_iter_skip(counter, cpu))
continue;
+ if (evsel__is_bpf(counter))
+ continue;
if (!counter->err) {
counter->err = read_counter_cpu(counter, rs,
counter->cpu_iter - 1);
@@ -423,17 +425,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 (!evsel__is_bpf(counter)) {
+ 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 +455,10 @@ static void read_counters(struct timespec *rs)
int err;
if (!stat_config.stop_read_counter) {
- if (target__has_bpf())
- err = read_bpf_map_counters();
- else
+ err = read_bpf_map_counters();
+ if (err < 0)
+ return;
+ if (err)
err = read_affinity_counters(rs);
if (err < 0)
return;
@@ -535,12 +549,13 @@ static int enable_counters(void)
struct evsel *evsel;
int err;
- if (target__has_bpf()) {
- evlist__for_each_entry(evsel_list, evsel) {
- err = bpf_counter__enable(evsel);
- if (err)
- return err;
- }
+ evlist__for_each_entry(evsel_list, evsel) {
+ if (!evsel__is_bpf(evsel))
+ continue;
+
+ err = bpf_counter__enable(evsel);
+ if (err)
+ return err;
}
if (stat_config.initial_delay < 0) {
@@ -784,11 +799,9 @@ static int __run_perf_stat(int argc, const char **argv,
int run_idx)
if (affinity__setup() < 0)
return -1;
- if (target__has_bpf()) {
- evlist__for_each_entry(evsel_list, counter) {
- if (bpf_counter__load(counter, ))
- return -1;
- }
+ evlist__for_each_entry(evsel_list, counter) {
+ if (bpf_counter__load(counter, ))
+ return -1;
}
evlist__for_each_cpu (evsel_list, i, cpu) {
diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
index 5de991ab46af9..3189b63714371 100644
--- a/tools/perf/util/bpf_counter.c
+++
