Re: [PATCH v2 1/2] perf evlist: Ensure grouped events with same cpu map
From: Jin, Yao
Date: Wed May 27 2020 - 03:26:44 EST
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