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

[Jiri Olsa <jolsa@redhat.com>]

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 ;-)

> 
> 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

thanks,
jirka