Messages in this thread | | | Subject | Re: [PATCH v2 1/2] perf evlist: Ensure grouped events with same cpu map | From | "Jin, Yao" <> | Date | Wed, 27 May 2020 14:31:03 +0800 |
| |
Hi Jiri,
On 5/27/2020 11:20 AM, Jin, Yao wrote: > Hi Jiri, > > On 5/26/2020 7:51 PM, Jiri Olsa wrote: >> On Mon, May 25, 2020 at 02:55:58PM +0800, Jin Yao wrote: >> >> SNIP >> >>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c >>> index 2a9de6491700..1161cffc0688 100644 >>> --- a/tools/perf/util/evlist.c >>> +++ b/tools/perf/util/evlist.c >>> @@ -1704,3 +1704,52 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list, >>> } >>> return leader; >>> } >>> + >>> +static bool cpus_map_matched(struct evsel *prev, struct evsel *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; >>> + } >>> + >>> + return true; >>> +} >>> + >>> +bool evlist__cpus_map_matched(struct evlist *evlist) >>> +{ >>> + struct evsel *prev = evlist__first(evlist), *evsel = prev; >>> + int nr_members = prev->core.nr_members; >>> + >>> + evlist__for_each_entry_continue(evlist, evsel) { >>> + if (nr_members <= 1) { >>> + prev = evsel; >>> + nr_members = evsel->core.nr_members; >>> + continue; >>> + } >>> + >>> + nr_members--; >>> + >>> + if (!cpus_map_matched(prev, evsel)) >>> + return false; >>> + >>> + prev = evsel; >>> + } >>> + >>> + return true; >>> +} >>> + >>> +void evlist__force_disable_group(struct evlist *evlist) >>> +{ >>> + struct evsel *evsel; >>> + >>> + pr_warning("WARNING: event cpu maps are not fully matched, " >>> + "stop event grouping\n"); >>> + >>> + evlist__for_each_entry(evlist, evsel) { >>> + evsel->leader = evsel; >>> + evsel->core.nr_members = 0; >>> + } >>> +} >> >> I think this is too much, we need to disable only groups with not >> matching cpus, not all of them, how about something like this >> > > Yes, that's too much. > >> >> struct evsel *pos; >> >> evlist__for_each_entry(evlist, evsel) { >> if (evsel->leader == evsel) >> continue; >> if (!cpus_map_matched(evsel->leader, evsel)) >> continue; >> pr_warn("Disabling group... >> >> for_each_group_member(pos, evsel->leader) { >> pos->leader = pos; >> evsel->core.nr_members = 0; >> } >> } >> >> jirka >> > > Hmm, change "!cpus_map_matched()" to "cpus_map_matched()"? and use for_each_group_evsel() to replace > for_each_group_member()? > > How about something like following? > > void evlist__check_cpu_maps(struct evlist *evlist) > { > struct evsel *evsel, *pos; > > evlist__for_each_entry(evlist, evsel) { > if (evsel->leader == evsel) > continue; > > if (cpu_maps_matched(evsel->leader, evsel)) > continue; > > pr_warning("WARNING: event cpu maps are not fully matched, " > "disable group\n"); > > for_each_group_evsel(pos, evsel->leader) { > pos->leader = pos; > pos->core.nr_members = 0; > } > > /* > * For core & uncore mixed event group, for example, > * '{cycles,unc_cbo_cache_lookup.any_i}', > * In evlist: > * cycles, > * unc_cbo_cache_lookup.any_i, > * unc_cbo_cache_lookup.any_i, > * unc_cbo_cache_lookup.any_i, > * unc_cbo_cache_lookup.any_i, > * > * cycles is leader and all unc_cbo_cache_lookup.any_i > * point to this leader. But for_each_group_evsel can't > * iterate all members from cycles. It only iterates > * cycles and one unc_cbo_cache_lookup.any_i. So we > * set extra evsel here. > */ > evsel->leader = evsel; > evsel->core.nr_members = 0; > } > } > > Thanks > Jin Yao
Issue is found!
It looks we can't set "pos->leader = pos" in either for_each_group_member() or in for_each_group_evsel() because it may exit the iteration immediately.
evlist__for_each_entry(evlist, evsel) { if (evsel->leader == evsel) continue;
if (cpu_maps_matched(evsel->leader, evsel)) continue;
pr_warning("WARNING: event cpu maps are not fully matched, " "disable group\n");
for_each_group_member(pos, evsel->leader) { pos->leader = pos; pos->core.nr_members = 0; }
Let me use the example of '{cycles,unc_cbo_cache_lookup.any_i}' again.
In evlist: cycles, unc_cbo_cache_lookup.any_i, unc_cbo_cache_lookup.any_i, unc_cbo_cache_lookup.any_i, unc_cbo_cache_lookup.any_i,
When we reach the for_each_group_member at first time, evsel is the first unc_cbo_cache_lookup.any_i and evsel->leader is cycles. pos is same as the evsel (the first unc_cbo_cache_lookup.any_i).
Once we execute "pos->leader = pos;", it's actually "evsel->leader = evsel". So now evsel->leader is changed to the first unc_cbo_cache_lookup.any_i.
In next iteration, pos is the second unc_cbo_cache_lookup.any_i. pos->leader is cycles but unfortunately evsel->leader has been changed to the first unc_cbo_cache_lookup.any_i. So iteration stops immediately.
I'm now thinking if we can solve this issue by an easy way.
Thanks Jin Yao
| |