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

Jin, Yao
2020-05-25T06:38:03Z

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