Re: [PATCH] perf evlist: Ensure grouped events with same cpu map

[Jin, Yao]

Hi Jiri,

On 5/22/2020 5:53 PM, Jiri Olsa wrote:
> On Thu, May 21, 2020 at 02:22:40PM +0800, Jin Yao wrote:
>
> SNIP
>
> > ---
> >   tools/perf/builtin-stat.c |  3 +++
> >   tools/perf/util/evlist.c  | 32 ++++++++++++++++++++++++++++++++
> >   tools/perf/util/evlist.h  |  5 +++++
> >   3 files changed, 40 insertions(+)
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 377e575f9645..0e4fc6b3323c 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -584,6 +584,9 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> >   	if (affinity__setup(&affinity) < 0)
> >   		return -1;
> >   
> > +	if (!evlist__cpus_matched(evsel_list))
> > +		evlist__force_disable_group(evsel_list);
> > +
> >   	evlist__for_each_cpu (evsel_list, i, cpu) {
> >   		affinity__set(&affinity, cpu);
> >   
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 2a9de6491700..fc6e410ca63b 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -1704,3 +1704,35 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
> >   	}
> >   	return leader;
> >   }
> > +
> > +bool evlist__cpus_matched(struct evlist *evlist)
> > +{
> > +	struct evsel *prev = evlist__first(evlist), *evsel = prev;
> > +
> > +	if (prev->core.nr_members <= 1)
> > +		return true;
>
> hum, this assumes there's only one group in evlist?
>
> how about case like A,{B,C},D,{E,F}

Yes, you are right, I need to consider the case such as A,{B,C},D,{E,F}. I will post v2.

> also please add automated tests for this

I will add case for testing the group members.

> > +
> > +	evlist__for_each_entry_continue(evlist, evsel) {
> > +		if (evsel->core.cpus->nr != prev->core.cpus->nr)
> > +			return false;
> > +
> > +		for (int i = 0; i < evsel->core.cpus->nr; i++) {
> > +			if (evsel->core.cpus->map[i] != prev->core.cpus->map[i])
> > +				return false;
> > +		}
> > +
> > +		prev = evsel;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +void evlist__force_disable_group(struct evlist *evlist)
> > +{
> > +	struct evsel *evsel;
>
> we need to put some warning for user in here

Yes, agree. I will add warning in v2.

Thanks
Jin Yao

> thanks,
> jirka
>
> > +
> > +	evlist__for_each_entry(evlist, evsel) {
> > +		evsel->leader = evsel;
> > +		evsel->core.nr_members = 0;
> > +	}
> > +}
> > diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> > index b6f325dfb4d2..ea7a53166cbd 100644
> > --- a/tools/perf/util/evlist.h
> > +++ b/tools/perf/util/evlist.h
> > @@ -355,4 +355,9 @@ void perf_evlist__force_leader(struct evlist *evlist);
> >   struct evsel *perf_evlist__reset_weak_group(struct evlist *evlist,
> >   						 struct evsel *evsel,
> >   						bool close);
> > +
> > +bool evlist__cpus_matched(struct evlist *evlist);
> > +
> > +void evlist__force_disable_group(struct evlist *evlist);
> > +
> >   #endif /* __PERF_EVLIST_H */
> > --
> > 2.17.1