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
