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

[Arnaldo Carvalho de Melo]

Em Thu, Apr 08, 2021 at 04:39:33PM +0000, Song Liu escreveu:
> > On Apr 8, 2021, at 4:47 AM, Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> > On Tue, Apr 06, 2021 at 05:36:01PM -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
> >> 
> > 
> > so if we are mixing events now, how about uing modifier for bpf counters,
> > instead of configuring .perfconfig list we could use:
> > 
> >  perf stat -e instructions:b,cs
> > 
> > thoughts?
> > 
> > the change below adds 'b' modifier and sets 'evsel::bpf_counter',
> > feel free to use it
> 
> I think we will need both 'b' modifier and .perfconfig configuration. 

Agreed, maximum flexibility.

> For systems with BPF-managed perf events running in the background, 
> .perfconfig makes sure perf-stat sessions will share PMCs with these 
> background monitoring tools. 'b' modifier, on the other hand, is useful
> when the user knows there is opportunity to share the PMCs. 
> 
> Does this make sense? 

I think so.

- Arnaldo
 
> Thanks,
> Song
> 
> > 
> > jirka
> > 
> > 
> > ---
> > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > index ca52581f1b17..c55e4e58d1dc 100644
> > --- a/tools/perf/util/evsel.h
> > +++ b/tools/perf/util/evsel.h
> > @@ -82,6 +82,7 @@ struct evsel {
> > 		bool			auto_merge_stats;
> > 		bool			collect_stat;
> > 		bool			weak_group;
> > +		bool			bpf_counter;
> > 		int			bpf_fd;
> > 		struct bpf_object	*bpf_obj;
> > 	};
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 9ecb45bea948..b5850f1ea90b 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1801,6 +1801,7 @@ struct event_modifier {
> > 	int pinned;
> > 	int weak;
> > 	int exclusive;
> > +	int bpf_counter;
> > };
> > 
> > static int get_event_modifier(struct event_modifier *mod, char *str,
> > @@ -1821,6 +1822,7 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
> > 	int exclude = eu | ek | eh;
> > 	int exclude_GH = evsel ? evsel->exclude_GH : 0;
> > 	int weak = 0;
> > +	int bpf_counter = 0;
> > 
> > 	memset(mod, 0, sizeof(*mod));
> > 
> > @@ -1864,6 +1866,8 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
> > 			exclusive = 1;
> > 		} else if (*str == 'W') {
> > 			weak = 1;
> > +		} else if (*str == 'b') {
> > +			bpf_counter = 1;
> > 		} else
> > 			break;
> > 
> > @@ -1895,6 +1899,7 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
> > 	mod->sample_read = sample_read;
> > 	mod->pinned = pinned;
> > 	mod->weak = weak;
> > +	mod->bpf_counter = bpf_counter;
> > 	mod->exclusive = exclusive;
> > 
> > 	return 0;
> > @@ -1909,7 +1914,7 @@ static int check_modifier(char *str)
> > 	char *p = str;
> > 
> > 	/* The sizeof includes 0 byte as well. */
> > -	if (strlen(str) > (sizeof("ukhGHpppPSDIWe") - 1))
> > +	if (strlen(str) > (sizeof("ukhGHpppPSDIWeb") - 1))
> > 		return -1;
> > 
> > 	while (*p) {
> > @@ -1950,6 +1955,7 @@ int parse_events__modifier_event(struct list_head *list, char *str, bool add)
> > 		evsel->sample_read         = mod.sample_read;
> > 		evsel->precise_max         = mod.precise_max;
> > 		evsel->weak_group	   = mod.weak;
> > +		evsel->bpf_counter         = mod.bpf_counter;
> > 
> > 		if (evsel__is_group_leader(evsel)) {
> > 			evsel->core.attr.pinned = mod.pinned;
> > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> > index 0b36285a9435..fb8646cc3e83 100644
> > --- a/tools/perf/util/parse-events.l
> > +++ b/tools/perf/util/parse-events.l
> > @@ -210,7 +210,7 @@ name_tag	[\'][a-zA-Z_*?\[\]][a-zA-Z0-9_*?\-,\.\[\]:=]*[\']
> > name_minus	[a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
> > drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
> > /* If you add a modifier you need to update check_modifier() */
> > -modifier_event	[ukhpPGHSDIWe]+
> > +modifier_event	[ukhpPGHSDIWeb]+
> > modifier_bp	[rwx]{1,3}
> > 
> > %%
> > 
> 

-- 

- Arnaldo