Re: [PATCH v2 1/2] perf evlist: Ensure grouped events with same cpu map

From: Jin, Yao
Date: Wed May 27 2020 - 02:31:09 EST


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