Messages in this thread Patch in this message | | | Date | Wed, 27 May 2020 18:28:00 +0200 | From | Jiri Olsa <> | Subject | Re: [PATCH v2 1/2] perf evlist: Ensure grouped events with same cpu map |
| |
On Wed, May 27, 2020 at 09:49:11PM +0800, Jin, Yao wrote: > Hi Jiri, > > On 5/27/2020 6:28 PM, Jiri Olsa wrote: > > On Wed, May 27, 2020 at 02:31:03PM +0800, Jin, Yao wrote: > > > > SNIP > > > > > > 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. > > > > hum, AFAICS the iteration will not break but continue to next evsel and > > pass the 'continue' for another group member.. what do I miss? > > > > jirka > > > > Let me use this example again. > > 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, > > Yes, once for_each_group_member breaks (due to the issue in 'pos->leader = > pos'), evlist__for_each_entry will continue to the second > unc_cbo_cache_lookup.any_i. But now evsel->leader != evsel (evsel->leader is > "cycles"), so it will go to cpu_maps_matched. > > But actually we don't need to go to cpu_maps_matched again. > > for_each_group_member(pos, evsel->leader) { > pos->leader = pos; > pos->core.nr_members = 0; > } > > If we solve the issue in above code, for_each_group_member doesn't break, > the leaders of all members in this group will be set to themselves. > > if (evsel->leader == evsel) > continue;
I see.. the problem is in the for_each_group_member, how about saving the leader into separate variable, like below
jirka
--- diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index f789103d8306..a754cad3f5a0 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -189,6 +189,51 @@ static struct perf_stat_config stat_config = { .big_num = true, }; +static bool cpus_map_matched(struct evsel *a, struct evsel *b) +{ + if (!a->core.cpus && !b->core.cpus) + return true; + + if (!a->core.cpus || !b->core.cpus) + return false; + + if (a->core.cpus->nr != b->core.cpus->nr) + return false; + + for (int i = 0; i < a->core.cpus->nr; i++) { + if (a->core.cpus->map[i] != b->core.cpus->map[i]) + return false; + } + + return true; +} + + +static void evlist__check_cpu_maps(struct evlist *evlist) +{ + struct evsel *evsel, *pos, *leader; + + evlist__for_each_entry(evlist, evsel) { + char buf[1024]; + + leader = evsel->leader; + if (leader == evsel) + continue; + if (cpus_map_matched(leader, evsel)) + continue; + + evsel__group_desc(leader, buf, sizeof(buf)); + WARN_ONCE(1, "WARNING: event cpu maps do not match, disabling group:\n"); + pr_warning(" %s\n", buf); + + for_each_group_evsel(pos, leader) { + pos->leader = pos; + pos->core.nr_members = 0; + } + evsel->leader->core.nr_members = 0; + } +} + static inline void diff_timespec(struct timespec *r, struct timespec *a, struct timespec *b) { @@ -1956,6 +2001,8 @@ int cmd_stat(int argc, const char **argv) } else if (argc && !strncmp(argv[0], "rep", 3)) return __cmd_report(argc, argv); + evlist__check_cpu_maps(evsel_list); + interval = stat_config.interval; timeout = stat_config.timeout;
| |