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 15:26:38 +0800 |
| |
Hi Jiri,
On 5/27/2020 2:31 PM, Jin, Yao wrote: > 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
How about this fix?
void evlist__check_cpu_maps(struct evlist *evlist) { struct evsel *evsel, *pos, *tmp;
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) { if (pos != evsel) { pos->leader = pos; pos->core.nr_members = 0; } }
tmp = evsel->leader; tmp->leader = tmp; tmp->core.nr_members = 0;
evsel->leader = evsel; evsel->core.nr_members = 0; } }
Thanks Jin Yao
| |