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

["Jin, Yao" <yao.jin@linux.intel.com>]

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