* [PATCH] perf stat: Ensure group is defined on top of the same cpu mask @ 2020-05-31 16:22 Jiri Olsa 2020-06-01 0:04 ` Ian Rogers 0 siblings, 1 reply; 17+ messages in thread From: Jiri Olsa @ 2020-05-31 16:22 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Jin Yao, lkml, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Ian Rogers, Stephane Eranian, Andi Kleen Jin Yao reported the issue (and posted first versions of this change) with groups being defined over events with different cpu mask. This causes assert aborts in get_group_fd, like: # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. Aborted All the events in the group have to be defined over the same cpus so the group_fd can be found for every leader/member pair. Adding check to ensure this condition is met and removing the group (with warning) if we detect mixed cpus, like: $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' WARNING: event cpu maps do not match, disabling group: anon group { power/energy-cores/, cycles } anon group { instructions, power/energy-cores/ } Fixes: 6a4bb04caacc8 ("perf tools: Enable grouping logic for parsed events") Co-developed-by: Jin Yao <yao.jin@linux.intel.com> Signed-off-by: Jin Yao <yao.jin@linux.intel.com> Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- tools/perf/builtin-stat.c | 51 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index b2b79aa161dd..512a41363d07 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -190,6 +190,55 @@ 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; + char buf[1024]; + + evlist__for_each_entry(evlist, evsel) { + leader = evsel->leader; + + /* Check that leader matches cpus with each member. */ + if (leader == evsel) + continue; + if (cpus_map_matched(leader, evsel)) + continue; + + /* + * If there's mismatch display dismantle the + * group and warn user. + */ + WARN_ONCE(1, "WARNING: group events cpu maps do not match, disabling group:\n"); + evsel__group_desc(leader, buf, sizeof(buf)); + 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) { @@ -1962,6 +2011,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; -- 2.25.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] perf stat: Ensure group is defined on top of the same cpu mask 2020-05-31 16:22 [PATCH] perf stat: Ensure group is defined on top of the same cpu mask Jiri Olsa @ 2020-06-01 0:04 ` Ian Rogers 2020-06-01 7:40 ` Jiri Olsa ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Ian Rogers @ 2020-06-01 0:04 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, Jin Yao, lkml, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen On Sun, May 31, 2020 at 9:22 AM Jiri Olsa <jolsa@kernel.org> wrote: > > Jin Yao reported the issue (and posted first versions of this change) > with groups being defined over events with different cpu mask. > > This causes assert aborts in get_group_fd, like: > > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > Aborted > > All the events in the group have to be defined over the same > cpus so the group_fd can be found for every leader/member pair. > > Adding check to ensure this condition is met and removing the > group (with warning) if we detect mixed cpus, like: > > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > WARNING: event cpu maps do not match, disabling group: > anon group { power/energy-cores/, cycles } > anon group { instructions, power/energy-cores/ } This is really cool! I wonder if there is a better wording for 'event cpu maps' ? It may be useful to list what the cpu maps are for the events as a diagnostic aid. Thanks, Ian > Fixes: 6a4bb04caacc8 ("perf tools: Enable grouping logic for parsed events") > Co-developed-by: Jin Yao <yao.jin@linux.intel.com> > Signed-off-by: Jin Yao <yao.jin@linux.intel.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > tools/perf/builtin-stat.c | 51 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index b2b79aa161dd..512a41363d07 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -190,6 +190,55 @@ 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; > + char buf[1024]; > + > + evlist__for_each_entry(evlist, evsel) { > + leader = evsel->leader; > + > + /* Check that leader matches cpus with each member. */ > + if (leader == evsel) > + continue; > + if (cpus_map_matched(leader, evsel)) > + continue; > + > + /* > + * If there's mismatch display dismantle the > + * group and warn user. > + */ > + WARN_ONCE(1, "WARNING: group events cpu maps do not match, disabling group:\n"); > + evsel__group_desc(leader, buf, sizeof(buf)); > + 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) > { > @@ -1962,6 +2011,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; > > -- > 2.25.4 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-01 0:04 ` Ian Rogers @ 2020-06-01 7:40 ` Jiri Olsa 2020-06-01 8:15 ` Jiri Olsa 2020-06-01 8:20 ` [PATCHv2] " Jiri Olsa 2 siblings, 0 replies; 17+ messages in thread From: Jiri Olsa @ 2020-06-01 7:40 UTC (permalink / raw) To: Ian Rogers Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Jin Yao, lkml, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen On Sun, May 31, 2020 at 05:04:47PM -0700, Ian Rogers wrote: > On Sun, May 31, 2020 at 9:22 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > Jin Yao reported the issue (and posted first versions of this change) > > with groups being defined over events with different cpu mask. > > > > This causes assert aborts in get_group_fd, like: > > > > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > > Aborted > > > > All the events in the group have to be defined over the same > > cpus so the group_fd can be found for every leader/member pair. > > > > Adding check to ensure this condition is met and removing the > > group (with warning) if we detect mixed cpus, like: > > > > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > > WARNING: event cpu maps do not match, disabling group: > > anon group { power/energy-cores/, cycles } > > anon group { instructions, power/energy-cores/ } > > This is really cool! I wonder if there is a better wording for 'event > cpu maps' ? It may be useful to list what the cpu maps are for the > events as a diagnostic aid. right, we could display that for -v option jirka ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-01 0:04 ` Ian Rogers 2020-06-01 7:40 ` Jiri Olsa @ 2020-06-01 8:15 ` Jiri Olsa 2020-06-01 8:20 ` [PATCHv2] " Jiri Olsa 2 siblings, 0 replies; 17+ messages in thread From: Jiri Olsa @ 2020-06-01 8:15 UTC (permalink / raw) To: Ian Rogers Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Jin Yao, lkml, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen On Sun, May 31, 2020 at 05:04:47PM -0700, Ian Rogers wrote: > On Sun, May 31, 2020 at 9:22 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > Jin Yao reported the issue (and posted first versions of this change) > > with groups being defined over events with different cpu mask. > > > > This causes assert aborts in get_group_fd, like: > > > > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > > Aborted > > > > All the events in the group have to be defined over the same > > cpus so the group_fd can be found for every leader/member pair. > > > > Adding check to ensure this condition is met and removing the > > group (with warning) if we detect mixed cpus, like: > > > > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > > WARNING: event cpu maps do not match, disabling group: > > anon group { power/energy-cores/, cycles } > > anon group { instructions, power/energy-cores/ } > > This is really cool! I wonder if there is a better wording for 'event > cpu maps' ? It may be useful to list what the cpu maps are for the > events as a diagnostic aid. right, something like this in verbose mode? it display cpu maps of events that did not match [root@krava perf]# ./perf stat -e '{cycles,power/energy-cores/}' -v WARNING: group events cpu maps do not match, disabling group: anon group { cycles, power/energy-cores/ } cycles: 0-7 power/energy-cores/: 0 jirka > > Thanks, > Ian > > > Fixes: 6a4bb04caacc8 ("perf tools: Enable grouping logic for parsed events") > > Co-developed-by: Jin Yao <yao.jin@linux.intel.com> > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > tools/perf/builtin-stat.c | 51 +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 51 insertions(+) > > > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > > index b2b79aa161dd..512a41363d07 100644 > > --- a/tools/perf/builtin-stat.c > > +++ b/tools/perf/builtin-stat.c > > @@ -190,6 +190,55 @@ 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; > > + char buf[1024]; > > + > > + evlist__for_each_entry(evlist, evsel) { > > + leader = evsel->leader; > > + > > + /* Check that leader matches cpus with each member. */ > > + if (leader == evsel) > > + continue; > > + if (cpus_map_matched(leader, evsel)) > > + continue; > > + > > + /* > > + * If there's mismatch display dismantle the > > + * group and warn user. > > + */ > > + WARN_ONCE(1, "WARNING: group events cpu maps do not match, disabling group:\n"); > > + evsel__group_desc(leader, buf, sizeof(buf)); > > + 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) > > { > > @@ -1962,6 +2011,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; > > > > -- > > 2.25.4 > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-01 0:04 ` Ian Rogers 2020-06-01 7:40 ` Jiri Olsa 2020-06-01 8:15 ` Jiri Olsa @ 2020-06-01 8:20 ` Jiri Olsa 2020-06-01 16:20 ` Ian Rogers 2 siblings, 1 reply; 17+ messages in thread From: Jiri Olsa @ 2020-06-01 8:20 UTC (permalink / raw) To: Ian Rogers Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Jin Yao, lkml, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen Jin Yao reported the issue (and posted first versions of this change) with groups being defined over events with different cpu mask. This causes assert aborts in get_group_fd, like: # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. Aborted All the events in the group have to be defined over the same cpus so the group_fd can be found for every leader/member pair. Adding check to ensure this condition is met and removing the group (with warning) if we detect mixed cpus, like: $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' WARNING: event cpu maps do not match, disabling group: anon group { power/energy-cores/, cycles } anon group { instructions, power/energy-cores/ } Ian asked also for cpu maps details, it's displayed in verbose mode: $ sudo perf stat -e '{cycles,power/energy-cores/}' -v WARNING: group events cpu maps do not match, disabling group: anon group { power/energy-cores/, cycles } power/energy-cores/: 0 cycles: 0-7 anon group { instructions, power/energy-cores/ } instructions: 0-7 power/energy-cores/: 0 Fixes: 6a4bb04caacc8 ("perf tools: Enable grouping logic for parsed events") Co-developed-by: Jin Yao <yao.jin@linux.intel.com> Signed-off-by: Jin Yao <yao.jin@linux.intel.com> Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- tools/perf/builtin-stat.c | 58 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) v2 changes: - display mixed events maps in verbose mode diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index b2b79aa161dd..dda60b9dbc63 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -190,6 +190,62 @@ 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; + char buf[1024]; + + evlist__for_each_entry(evlist, evsel) { + leader = evsel->leader; + + /* Check that leader matches cpus with each member. */ + if (leader == evsel) + continue; + if (cpus_map_matched(leader, evsel)) + continue; + + /* + * If there's mismatch display dismantle the + * group and warn user. + */ + WARN_ONCE(1, "WARNING: group events cpu maps do not match, disabling group:\n"); + evsel__group_desc(leader, buf, sizeof(buf)); + pr_warning(" %s\n", buf); + + if (verbose) { + cpu_map__snprint(leader->core.cpus, buf, sizeof(buf)); + pr_warning(" %s: %s\n", leader->name, buf); + cpu_map__snprint(evsel->core.cpus, buf, sizeof(buf)); + pr_warning(" %s: %s\n", evsel->name, 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) { @@ -2113,6 +2169,8 @@ int cmd_stat(int argc, const char **argv) goto out; } + evlist__check_cpu_maps(evsel_list); + /* * Initialize thread_map with comm names, * so we could print it out on output. -- 2.25.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-01 8:20 ` [PATCHv2] " Jiri Olsa @ 2020-06-01 16:20 ` Ian Rogers 2020-06-02 2:47 ` Namhyung Kim 2020-06-02 10:17 ` [PATCHv3] " Jiri Olsa 0 siblings, 2 replies; 17+ messages in thread From: Ian Rogers @ 2020-06-01 16:20 UTC (permalink / raw) To: Jiri Olsa Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Jin Yao, lkml, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen On Mon, Jun 1, 2020 at 1:20 AM Jiri Olsa <jolsa@redhat.com> wrote: > > Jin Yao reported the issue (and posted first versions of this change) > with groups being defined over events with different cpu mask. > > This causes assert aborts in get_group_fd, like: > > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > Aborted > > All the events in the group have to be defined over the same > cpus so the group_fd can be found for every leader/member pair. > > Adding check to ensure this condition is met and removing the > group (with warning) if we detect mixed cpus, like: > > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > WARNING: event cpu maps do not match, disabling group: > anon group { power/energy-cores/, cycles } > anon group { instructions, power/energy-cores/ } > > Ian asked also for cpu maps details, it's displayed in verbose mode: > > $ sudo perf stat -e '{cycles,power/energy-cores/}' -v > WARNING: group events cpu maps do not match, disabling group: > anon group { power/energy-cores/, cycles } > power/energy-cores/: 0 > cycles: 0-7 > anon group { instructions, power/energy-cores/ } > instructions: 0-7 > power/energy-cores/: 0 This is great! A nit, would 'grouped events cpus do not match' read better? I think the cpu map is more of an internal naming convention. Thanks, Ian > Fixes: 6a4bb04caacc8 ("perf tools: Enable grouping logic for parsed events") > Co-developed-by: Jin Yao <yao.jin@linux.intel.com> > Signed-off-by: Jin Yao <yao.jin@linux.intel.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > tools/perf/builtin-stat.c | 58 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 58 insertions(+) > > v2 changes: > - display mixed events maps in verbose mode > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index b2b79aa161dd..dda60b9dbc63 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -190,6 +190,62 @@ 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; > + char buf[1024]; > + > + evlist__for_each_entry(evlist, evsel) { > + leader = evsel->leader; > + > + /* Check that leader matches cpus with each member. */ > + if (leader == evsel) > + continue; > + if (cpus_map_matched(leader, evsel)) > + continue; > + > + /* > + * If there's mismatch display dismantle the > + * group and warn user. > + */ > + WARN_ONCE(1, "WARNING: group events cpu maps do not match, disabling group:\n"); > + evsel__group_desc(leader, buf, sizeof(buf)); > + pr_warning(" %s\n", buf); > + > + if (verbose) { > + cpu_map__snprint(leader->core.cpus, buf, sizeof(buf)); > + pr_warning(" %s: %s\n", leader->name, buf); > + cpu_map__snprint(evsel->core.cpus, buf, sizeof(buf)); > + pr_warning(" %s: %s\n", evsel->name, 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) > { > @@ -2113,6 +2169,8 @@ int cmd_stat(int argc, const char **argv) > goto out; > } > > + evlist__check_cpu_maps(evsel_list); > + > /* > * Initialize thread_map with comm names, > * so we could print it out on output. > -- > 2.25.4 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-01 16:20 ` Ian Rogers @ 2020-06-02 2:47 ` Namhyung Kim 2020-06-02 8:15 ` Jiri Olsa 2020-06-02 10:17 ` [PATCHv3] " Jiri Olsa 1 sibling, 1 reply; 17+ messages in thread From: Namhyung Kim @ 2020-06-02 2:47 UTC (permalink / raw) To: Ian Rogers Cc: Jiri Olsa, Jiri Olsa, Arnaldo Carvalho de Melo, Jin Yao, lkml, Ingo Molnar, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen On Tue, Jun 2, 2020 at 1:21 AM Ian Rogers <irogers@google.com> wrote: > > On Mon, Jun 1, 2020 at 1:20 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > Jin Yao reported the issue (and posted first versions of this change) > > with groups being defined over events with different cpu mask. > > > > This causes assert aborts in get_group_fd, like: > > > > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > > Aborted > > > > All the events in the group have to be defined over the same > > cpus so the group_fd can be found for every leader/member pair. > > > > Adding check to ensure this condition is met and removing the > > group (with warning) if we detect mixed cpus, like: > > > > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > > WARNING: event cpu maps do not match, disabling group: > > anon group { power/energy-cores/, cycles } > > anon group { instructions, power/energy-cores/ } > > > > Ian asked also for cpu maps details, it's displayed in verbose mode: > > > > $ sudo perf stat -e '{cycles,power/energy-cores/}' -v > > WARNING: group events cpu maps do not match, disabling group: > > anon group { power/energy-cores/, cycles } > > power/energy-cores/: 0 > > cycles: 0-7 > > anon group { instructions, power/energy-cores/ } > > instructions: 0-7 > > power/energy-cores/: 0 > > This is great! A nit, would 'grouped events cpus do not match' read > better? I think the cpu map is more of an internal naming convention. Allowed cpus? Thanks Namhyung ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-02 2:47 ` Namhyung Kim @ 2020-06-02 8:15 ` Jiri Olsa 2020-06-02 11:50 ` Namhyung Kim 0 siblings, 1 reply; 17+ messages in thread From: Jiri Olsa @ 2020-06-02 8:15 UTC (permalink / raw) To: Namhyung Kim Cc: Ian Rogers, Jiri Olsa, Arnaldo Carvalho de Melo, Jin Yao, lkml, Ingo Molnar, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen On Tue, Jun 02, 2020 at 11:47:19AM +0900, Namhyung Kim wrote: > On Tue, Jun 2, 2020 at 1:21 AM Ian Rogers <irogers@google.com> wrote: > > > > On Mon, Jun 1, 2020 at 1:20 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > Jin Yao reported the issue (and posted first versions of this change) > > > with groups being defined over events with different cpu mask. > > > > > > This causes assert aborts in get_group_fd, like: > > > > > > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > > > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > > > Aborted > > > > > > All the events in the group have to be defined over the same > > > cpus so the group_fd can be found for every leader/member pair. > > > > > > Adding check to ensure this condition is met and removing the > > > group (with warning) if we detect mixed cpus, like: > > > > > > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > > > WARNING: event cpu maps do not match, disabling group: > > > anon group { power/energy-cores/, cycles } > > > anon group { instructions, power/energy-cores/ } > > > > > > Ian asked also for cpu maps details, it's displayed in verbose mode: > > > > > > $ sudo perf stat -e '{cycles,power/energy-cores/}' -v > > > WARNING: group events cpu maps do not match, disabling group: > > > anon group { power/energy-cores/, cycles } > > > power/energy-cores/: 0 > > > cycles: 0-7 > > > anon group { instructions, power/energy-cores/ } > > > instructions: 0-7 > > > power/energy-cores/: 0 > > > > This is great! A nit, would 'grouped events cpus do not match' read > > better? I think the cpu map is more of an internal naming convention. ok > > Allowed cpus? hum, what you mean? jirka > > Thanks > Namhyung > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-02 8:15 ` Jiri Olsa @ 2020-06-02 11:50 ` Namhyung Kim 2020-06-02 12:10 ` Jiri Olsa 0 siblings, 1 reply; 17+ messages in thread From: Namhyung Kim @ 2020-06-02 11:50 UTC (permalink / raw) To: Jiri Olsa Cc: Ian Rogers, Jiri Olsa, Arnaldo Carvalho de Melo, Jin Yao, lkml, Ingo Molnar, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen Hi Jiri, On Tue, Jun 2, 2020 at 5:16 PM Jiri Olsa <jolsa@redhat.com> wrote: > > On Tue, Jun 02, 2020 at 11:47:19AM +0900, Namhyung Kim wrote: > > On Tue, Jun 2, 2020 at 1:21 AM Ian Rogers <irogers@google.com> wrote: > > > > > > On Mon, Jun 1, 2020 at 1:20 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > > Jin Yao reported the issue (and posted first versions of this change) > > > > with groups being defined over events with different cpu mask. > > > > > > > > This causes assert aborts in get_group_fd, like: > > > > > > > > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > > > > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > > > > Aborted > > > > > > > > All the events in the group have to be defined over the same > > > > cpus so the group_fd can be found for every leader/member pair. > > > > > > > > Adding check to ensure this condition is met and removing the > > > > group (with warning) if we detect mixed cpus, like: > > > > > > > > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > > > > WARNING: event cpu maps do not match, disabling group: > > > > anon group { power/energy-cores/, cycles } > > > > anon group { instructions, power/energy-cores/ } > > > > > > > > Ian asked also for cpu maps details, it's displayed in verbose mode: > > > > > > > > $ sudo perf stat -e '{cycles,power/energy-cores/}' -v > > > > WARNING: group events cpu maps do not match, disabling group: > > > > anon group { power/energy-cores/, cycles } > > > > power/energy-cores/: 0 > > > > cycles: 0-7 > > > > anon group { instructions, power/energy-cores/ } > > > > instructions: 0-7 > > > > power/energy-cores/: 0 > > > > > > This is great! A nit, would 'grouped events cpus do not match' read > > > better? I think the cpu map is more of an internal naming convention. > > Allowed cpus? > > hum, what you mean? I mean that we can use 'allowed cpus' rather then 'cpu map' in the message. Something like this? allowed cpus for events in a group do not match, disabling group: Thanks Namhyung ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-02 11:50 ` Namhyung Kim @ 2020-06-02 12:10 ` Jiri Olsa 2020-06-02 13:25 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 17+ messages in thread From: Jiri Olsa @ 2020-06-02 12:10 UTC (permalink / raw) To: Namhyung Kim Cc: Ian Rogers, Jiri Olsa, Arnaldo Carvalho de Melo, Jin Yao, lkml, Ingo Molnar, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen On Tue, Jun 02, 2020 at 08:50:17PM +0900, Namhyung Kim wrote: > Hi Jiri, > > On Tue, Jun 2, 2020 at 5:16 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Tue, Jun 02, 2020 at 11:47:19AM +0900, Namhyung Kim wrote: > > > On Tue, Jun 2, 2020 at 1:21 AM Ian Rogers <irogers@google.com> wrote: > > > > > > > > On Mon, Jun 1, 2020 at 1:20 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > > > > Jin Yao reported the issue (and posted first versions of this change) > > > > > with groups being defined over events with different cpu mask. > > > > > > > > > > This causes assert aborts in get_group_fd, like: > > > > > > > > > > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > > > > > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > > > > > Aborted > > > > > > > > > > All the events in the group have to be defined over the same > > > > > cpus so the group_fd can be found for every leader/member pair. > > > > > > > > > > Adding check to ensure this condition is met and removing the > > > > > group (with warning) if we detect mixed cpus, like: > > > > > > > > > > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > > > > > WARNING: event cpu maps do not match, disabling group: > > > > > anon group { power/energy-cores/, cycles } > > > > > anon group { instructions, power/energy-cores/ } > > > > > > > > > > Ian asked also for cpu maps details, it's displayed in verbose mode: > > > > > > > > > > $ sudo perf stat -e '{cycles,power/energy-cores/}' -v > > > > > WARNING: group events cpu maps do not match, disabling group: > > > > > anon group { power/energy-cores/, cycles } > > > > > power/energy-cores/: 0 > > > > > cycles: 0-7 > > > > > anon group { instructions, power/energy-cores/ } > > > > > instructions: 0-7 > > > > > power/energy-cores/: 0 > > > > > > > > This is great! A nit, would 'grouped events cpus do not match' read > > > > better? I think the cpu map is more of an internal naming convention. > > > Allowed cpus? > > > > hum, what you mean? > > I mean that we can use 'allowed cpus' rather then 'cpu map' in the message. > Something like this? > > allowed cpus for events in a group do not match, disabling group: hm, I like more the one Ian suggested.. anyway, leaving this to Arnaldo, he can change that before committing ;-) thanks, jirka ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-02 12:10 ` Jiri Olsa @ 2020-06-02 13:25 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 17+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-06-02 13:25 UTC (permalink / raw) To: Jiri Olsa Cc: Namhyung Kim, Ian Rogers, Jiri Olsa, Jin Yao, lkml, Ingo Molnar, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen Em Tue, Jun 02, 2020 at 02:10:17PM +0200, Jiri Olsa escreveu: > On Tue, Jun 02, 2020 at 08:50:17PM +0900, Namhyung Kim wrote: > > Hi Jiri, > > > > On Tue, Jun 2, 2020 at 5:16 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > On Tue, Jun 02, 2020 at 11:47:19AM +0900, Namhyung Kim wrote: > > > > On Tue, Jun 2, 2020 at 1:21 AM Ian Rogers <irogers@google.com> wrote: > > > > > > > > > > On Mon, Jun 1, 2020 at 1:20 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > > > > > > Jin Yao reported the issue (and posted first versions of this change) > > > > > > with groups being defined over events with different cpu mask. > > > > > > > > > > > > This causes assert aborts in get_group_fd, like: > > > > > > > > > > > > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > > > > > > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > > > > > > Aborted > > > > > > > > > > > > All the events in the group have to be defined over the same > > > > > > cpus so the group_fd can be found for every leader/member pair. > > > > > > > > > > > > Adding check to ensure this condition is met and removing the > > > > > > group (with warning) if we detect mixed cpus, like: > > > > > > > > > > > > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > > > > > > WARNING: event cpu maps do not match, disabling group: > > > > > > anon group { power/energy-cores/, cycles } > > > > > > anon group { instructions, power/energy-cores/ } > > > > > > > > > > > > Ian asked also for cpu maps details, it's displayed in verbose mode: > > > > > > > > > > > > $ sudo perf stat -e '{cycles,power/energy-cores/}' -v > > > > > > WARNING: group events cpu maps do not match, disabling group: > > > > > > anon group { power/energy-cores/, cycles } > > > > > > power/energy-cores/: 0 > > > > > > cycles: 0-7 > > > > > > anon group { instructions, power/energy-cores/ } > > > > > > instructions: 0-7 > > > > > > power/energy-cores/: 0 > > > > > > > > > > This is great! A nit, would 'grouped events cpus do not match' read > > > > > better? I think the cpu map is more of an internal naming convention. > > > > Allowed cpus? > > > > > > hum, what you mean? > > > > I mean that we can use 'allowed cpus' rather then 'cpu map' in the message. > > Something like this? > > > > allowed cpus for events in a group do not match, disabling group: > > hm, I like more the one Ian suggested.. anyway, leaving this to Arnaldo, > he can change that before committing ;-) I think its ok as-is, Ian, can I have your acked-by? - Arnaldo ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-01 16:20 ` Ian Rogers 2020-06-02 2:47 ` Namhyung Kim @ 2020-06-02 10:17 ` Jiri Olsa 2020-06-02 13:42 ` Arnaldo Carvalho de Melo 2020-06-02 15:48 ` Ian Rogers 1 sibling, 2 replies; 17+ messages in thread From: Jiri Olsa @ 2020-06-02 10:17 UTC (permalink / raw) To: Ian Rogers Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Jin Yao, lkml, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen Jin Yao reported the issue (and posted first versions of this change) with groups being defined over events with different cpu mask. This causes assert aborts in get_group_fd, like: # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. Aborted All the events in the group have to be defined over the same cpus so the group_fd can be found for every leader/member pair. Adding check to ensure this condition is met and removing the group (with warning) if we detect mixed cpus, like: $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' WARNING: event cpu maps do not match, disabling group: anon group { power/energy-cores/, cycles } anon group { instructions, power/energy-cores/ } Ian asked also for cpu maps details, it's displayed in verbose mode: $ sudo perf stat -e '{cycles,power/energy-cores/}' -v WARNING: group events cpu maps do not match, disabling group: anon group { power/energy-cores/, cycles } power/energy-cores/: 0 cycles: 0-7 anon group { instructions, power/energy-cores/ } instructions: 0-7 power/energy-cores/: 0 Fixes: 6a4bb04caacc8 ("perf tools: Enable grouping logic for parsed events") Co-developed-by: Jin Yao <yao.jin@linux.intel.com> Signed-off-by: Jin Yao <yao.jin@linux.intel.com> Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- tools/perf/builtin-stat.c | 55 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) v3 changes: - reword the warning with Ian's suggestion diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index b2b79aa161dd..9be020e0098a 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -190,6 +190,59 @@ 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; + char buf[1024]; + + evlist__for_each_entry(evlist, evsel) { + leader = evsel->leader; + + /* Check that leader matches cpus with each member. */ + if (leader == evsel) + continue; + if (cpus_map_matched(leader, evsel)) + continue; + + /* If there's mismatch disable the group and warn user. */ + WARN_ONCE(1, "WARNING: grouped events cpus do not match, disabling group:\n"); + evsel__group_desc(leader, buf, sizeof(buf)); + pr_warning(" %s\n", buf); + + if (verbose) { + cpu_map__snprint(leader->core.cpus, buf, sizeof(buf)); + pr_warning(" %s: %s\n", leader->name, buf); + cpu_map__snprint(evsel->core.cpus, buf, sizeof(buf)); + pr_warning(" %s: %s\n", evsel->name, 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) { @@ -2113,6 +2166,8 @@ int cmd_stat(int argc, const char **argv) goto out; } + evlist__check_cpu_maps(evsel_list); + /* * Initialize thread_map with comm names, * so we could print it out on output. -- 2.25.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-02 10:17 ` [PATCHv3] " Jiri Olsa @ 2020-06-02 13:42 ` Arnaldo Carvalho de Melo 2020-06-02 14:05 ` Jiri Olsa 2020-06-02 15:48 ` Ian Rogers 1 sibling, 1 reply; 17+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-06-02 13:42 UTC (permalink / raw) To: Jiri Olsa Cc: Ian Rogers, Jiri Olsa, Jin Yao, lkml, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen Em Tue, Jun 02, 2020 at 12:17:36PM +0200, Jiri Olsa escreveu: > Jin Yao reported the issue (and posted first versions of this change) > with groups being defined over events with different cpu mask. > This causes assert aborts in get_group_fd, like: > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > Aborted > All the events in the group have to be defined over the same > cpus so the group_fd can be found for every leader/member pair. > Adding check to ensure this condition is met and removing the > group (with warning) if we detect mixed cpus, like: > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > WARNING: event cpu maps do not match, disabling group: > anon group { power/energy-cores/, cycles } > anon group { instructions, power/energy-cores/ } So it doesn't disable the 'group', it disables the 'grouping' of those events, right? I.e. reading the WARNING, I thought that it would count nothing, since it lists both groups as being disabled, but when I tested I noticed that: [root@seventh ~]# perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' WARNING: grouped events cpus do not match, disabling group: anon group { power/energy-cores/, cycles } anon group { instructions, power/energy-cores/ } ^C Performance counter stats for 'system wide': 12.62 Joules power/energy-cores/ 106,920,637 cycles 80,228,899 instructions # 0.75 insn per cycle 12.62 Joules power/energy-cores/ 14.514476987 seconds time elapsed [root@seventh ~]# I.e. it counted the events, ungrouped, or am I missing something? If I do: [root@seventh ~]# perf stat -e '{power/energy-cores/,power/energy-ram/},{instructions,cycles}' -a sleep 2 Performance counter stats for 'system wide': 1.73 Joules power/energy-cores/ 0.92 Joules power/energy-ram/ 12,191,658 instructions # 0.67 insn per cycle 18,275,233 cycles 2.001272492 seconds time elapsed [root@seventh ~]# It works, grouped. One observation, shouldn't we somehow show in the output that the first two were indeed grouped, ditto for the second two? Also, this needs improvement: [root@seventh ~]# perf stat -e '{power/energy-cores/,power/energy-ram/},{instructions,cycles}' sleep 2 Error: The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (power/energy-cores/). /bin/dmesg | grep -i perf may provide additional information. [root@seventh ~]# Probably stating that the power/ events can only be done on a system wide mode or per-cpu? I'm applying the patch now, with the above observations as committer notes, we can improve this in follow on patch, - Arnaldo > Ian asked also for cpu maps details, it's displayed in verbose mode: > > $ sudo perf stat -e '{cycles,power/energy-cores/}' -v > WARNING: group events cpu maps do not match, disabling group: > anon group { power/energy-cores/, cycles } > power/energy-cores/: 0 > cycles: 0-7 > anon group { instructions, power/energy-cores/ } > instructions: 0-7 > power/energy-cores/: 0 > > Fixes: 6a4bb04caacc8 ("perf tools: Enable grouping logic for parsed events") > Co-developed-by: Jin Yao <yao.jin@linux.intel.com> > Signed-off-by: Jin Yao <yao.jin@linux.intel.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > tools/perf/builtin-stat.c | 55 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > v3 changes: > - reword the warning with Ian's suggestion > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index b2b79aa161dd..9be020e0098a 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -190,6 +190,59 @@ 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; > + char buf[1024]; > + > + evlist__for_each_entry(evlist, evsel) { > + leader = evsel->leader; > + > + /* Check that leader matches cpus with each member. */ > + if (leader == evsel) > + continue; > + if (cpus_map_matched(leader, evsel)) > + continue; > + > + /* If there's mismatch disable the group and warn user. */ > + WARN_ONCE(1, "WARNING: grouped events cpus do not match, disabling group:\n"); > + evsel__group_desc(leader, buf, sizeof(buf)); > + pr_warning(" %s\n", buf); > + > + if (verbose) { > + cpu_map__snprint(leader->core.cpus, buf, sizeof(buf)); > + pr_warning(" %s: %s\n", leader->name, buf); > + cpu_map__snprint(evsel->core.cpus, buf, sizeof(buf)); > + pr_warning(" %s: %s\n", evsel->name, 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) > { > @@ -2113,6 +2166,8 @@ int cmd_stat(int argc, const char **argv) > goto out; > } > > + evlist__check_cpu_maps(evsel_list); > + > /* > * Initialize thread_map with comm names, > * so we could print it out on output. > -- > 2.25.4 > -- - Arnaldo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-02 13:42 ` Arnaldo Carvalho de Melo @ 2020-06-02 14:05 ` Jiri Olsa 2020-06-02 15:03 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 17+ messages in thread From: Jiri Olsa @ 2020-06-02 14:05 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Ian Rogers, Jiri Olsa, Jin Yao, lkml, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen On Tue, Jun 02, 2020 at 10:42:56AM -0300, Arnaldo Carvalho de Melo wrote: > Em Tue, Jun 02, 2020 at 12:17:36PM +0200, Jiri Olsa escreveu: > > Jin Yao reported the issue (and posted first versions of this change) > > with groups being defined over events with different cpu mask. > > > This causes assert aborts in get_group_fd, like: > > > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > > Aborted > > > All the events in the group have to be defined over the same > > cpus so the group_fd can be found for every leader/member pair. > > > Adding check to ensure this condition is met and removing the > > group (with warning) if we detect mixed cpus, like: > > > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > > WARNING: event cpu maps do not match, disabling group: > > anon group { power/energy-cores/, cycles } > > anon group { instructions, power/energy-cores/ } > > So it doesn't disable the 'group', it disables the 'grouping' of those > events, right? I.e. reading the WARNING, I thought that it would count > nothing, since it lists both groups as being disabled, but when I tested > I noticed that: > > [root@seventh ~]# perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > WARNING: grouped events cpus do not match, disabling group: > anon group { power/energy-cores/, cycles } > anon group { instructions, power/energy-cores/ } > ^C > Performance counter stats for 'system wide': > > 12.62 Joules power/energy-cores/ > 106,920,637 cycles > 80,228,899 instructions # 0.75 insn per cycle > 12.62 Joules power/energy-cores/ > > 14.514476987 seconds time elapsed > > > [root@seventh ~]# > > I.e. it counted the events, ungrouped, or am I missing something? right, it disables 'grouping', events are scheduled/counted individualy this way we will not hit the issue when looking for group_fd FD and there's not any, because of different cpu maps > > If I do: > > [root@seventh ~]# perf stat -e '{power/energy-cores/,power/energy-ram/},{instructions,cycles}' -a sleep 2 > > Performance counter stats for 'system wide': > > 1.73 Joules power/energy-cores/ > 0.92 Joules power/energy-ram/ > 12,191,658 instructions # 0.67 insn per cycle > 18,275,233 cycles > > 2.001272492 seconds time elapsed > > [root@seventh ~]# > > It works, grouped. One observation, shouldn't we somehow show in the > output that the first two were indeed grouped, ditto for the second two? yea, we don't display groups in output.. also there's no number for the group, it's still separate events numbers in output grouping is only used when creating events > > Also, this needs improvement: > > [root@seventh ~]# perf stat -e '{power/energy-cores/,power/energy-ram/},{instructions,cycles}' sleep 2 > Error: > The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (power/energy-cores/). > /bin/dmesg | grep -i perf may provide additional information. yes, power events don't work with events without cpu being defined, which is what we do for 'workload' session.. we should either check for that and display some sensible error for power events or perhaps check if we could monitor like perf record does with creating events for task and every cpu in the system thanks, jirka ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-02 14:05 ` Jiri Olsa @ 2020-06-02 15:03 ` Arnaldo Carvalho de Melo 2020-06-02 15:28 ` Jiri Olsa 0 siblings, 1 reply; 17+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-06-02 15:03 UTC (permalink / raw) To: Jiri Olsa Cc: Ian Rogers, Jiri Olsa, Jin Yao, lkml, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen Em Tue, Jun 02, 2020 at 04:05:08PM +0200, Jiri Olsa escreveu: > On Tue, Jun 02, 2020 at 10:42:56AM -0300, Arnaldo Carvalho de Melo wrote: > > Em Tue, Jun 02, 2020 at 12:17:36PM +0200, Jiri Olsa escreveu: > > > Jin Yao reported the issue (and posted first versions of this change) > > > with groups being defined over events with different cpu mask. > > > > > This causes assert aborts in get_group_fd, like: > > > > > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > > > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > > > Aborted > > > > > All the events in the group have to be defined over the same > > > cpus so the group_fd can be found for every leader/member pair. > > > > > Adding check to ensure this condition is met and removing the > > > group (with warning) if we detect mixed cpus, like: > > > > > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > > > WARNING: event cpu maps do not match, disabling group: > > > anon group { power/energy-cores/, cycles } > > > anon group { instructions, power/energy-cores/ } > > > > So it doesn't disable the 'group', it disables the 'grouping' of those > > events, right? I.e. reading the WARNING, I thought that it would count > > nothing, since it lists both groups as being disabled, but when I tested > > I noticed that: > > > > [root@seventh ~]# perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > > WARNING: grouped events cpus do not match, disabling group: > > anon group { power/energy-cores/, cycles } > > anon group { instructions, power/energy-cores/ } > > ^C > > Performance counter stats for 'system wide': > > > > 12.62 Joules power/energy-cores/ > > 106,920,637 cycles > > 80,228,899 instructions # 0.75 insn per cycle > > 12.62 Joules power/energy-cores/ > > > > 14.514476987 seconds time elapsed > > > > > > [root@seventh ~]# > > > > I.e. it counted the events, ungrouped, or am I missing something? > > right, it disables 'grouping', events are scheduled/counted individualy Ok, I applied this already, we can fix this in the next cycle. > this way we will not hit the issue when looking for group_fd FD > and there's not any, because of different cpu maps > > If I do: > > [root@seventh ~]# perf stat -e '{power/energy-cores/,power/energy-ram/},{instructions,cycles}' -a sleep 2 > > Performance counter stats for 'system wide': > > 1.73 Joules power/energy-cores/ > > 0.92 Joules power/energy-ram/ > > 12,191,658 instructions # 0.67 insn per cycles/ > > 18,275,233 cycles > > 2.001272492 seconds time elapsed > > [root@seventh ~]# > > > > It works, grouped. One observation, shouldn't we somehow show in the > > output that the first two were indeed grouped, ditto for the second two? > yea, we don't display groups in output.. also there's no number > for the group, it's still separate events numbers in output > grouping is only used when creating events perhaps if we just add a blank line to separate groups? I.e. the above would be: [root@seventh ~]# perf stat -e '{power/energy-cores/,power/energy-ram/},{instructions,cycles}' -a sleep 2 Performance counter stats for 'system wide': 1.73 Joules power/energy-cores/ 0.92 Joules power/energy-ram/ 12,191,658 instructions # 0.67 insn per cycle 18,275,233 cycles 2.001272492 seconds time elapsed [root@seventh ~]# Humm, in the presence of at least one group, any ungrouped events would have to be also separated, i.e.: [root@seventh ~]# perf stat -e '{power/energy-cores/,power/energy-ram/},instructions,cycles' -a sleep 2 Performance counter stats for 'system wide': 1.73 Joules power/energy-cores/ 0.92 Joules power/energy-ram/ 12,191,658 instructions # 0.67 insn per cycle 18,275,233 cycles 2.001272492 seconds time elapsed [root@seventh ~]# wdyt? > > Also, this needs improvement: > > [root@seventh ~]# perf stat -e '{power/energy-cores/,power/energy-ram/},{instructions,cycles}' sleep 2 > > Error: > > The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (power/energy-cores/). > > /bin/dmesg | grep -i perf may provide additional information. > yes, power events don't work with events without cpu being defined, > which is what we do for 'workload' session.. we should either check > for that and display some sensible error for power events > or perhaps check if we could monitor like perf record does with creating > events for task and every cpu in the system - Arnaldo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-02 15:03 ` Arnaldo Carvalho de Melo @ 2020-06-02 15:28 ` Jiri Olsa 0 siblings, 0 replies; 17+ messages in thread From: Jiri Olsa @ 2020-06-02 15:28 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Ian Rogers, Jiri Olsa, Jin Yao, lkml, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen On Tue, Jun 02, 2020 at 12:03:52PM -0300, Arnaldo Carvalho de Melo wrote: SNIP > > right, it disables 'grouping', events are scheduled/counted individualy > > Ok, I applied this already, we can fix this in the next cycle. > > > this way we will not hit the issue when looking for group_fd FD > > and there's not any, because of different cpu maps > > > > If I do: > > > > [root@seventh ~]# perf stat -e '{power/energy-cores/,power/energy-ram/},{instructions,cycles}' -a sleep 2 > > > > Performance counter stats for 'system wide': > > > > 1.73 Joules power/energy-cores/ > > > 0.92 Joules power/energy-ram/ > > > 12,191,658 instructions # 0.67 insn per cycles/ > > > 18,275,233 cycles > > > > 2.001272492 seconds time elapsed > > > > [root@seventh ~]# > > > > > > It works, grouped. One observation, shouldn't we somehow show in the > > > output that the first two were indeed grouped, ditto for the second two? > > > yea, we don't display groups in output.. also there's no number > > for the group, it's still separate events numbers in output > > grouping is only used when creating events > > perhaps if we just add a blank line to separate groups? I.e. the above > would be: > > [root@seventh ~]# perf stat -e '{power/energy-cores/,power/energy-ram/},{instructions,cycles}' -a sleep 2 > > Performance counter stats for 'system wide': > > 1.73 Joules power/energy-cores/ > 0.92 Joules power/energy-ram/ > > 12,191,658 instructions # 0.67 insn per cycle > 18,275,233 cycles > > 2.001272492 seconds time elapsed maybe it could be separated by new line and we could put note in the comment: Performance counter stats for 'system wide': 1.73 Joules power/energy-cores/ 0.92 Joules power/energy-ram/ # groupped 12,191,658 instructions # 0.67 insn per cycle 18,275,233 cycles 2.001272492 seconds time elapsed for events that already have defined comment, it'd be on the next line, like: Performance counter stats for 'system wide': 1.73 Joules power/energy-cores/ 0.92 Joules power/energy-ram/ # XXXXX # groupped 12,191,658 instructions # 0.67 insn per cycle 18,275,233 cycles 2.001272492 seconds time elapsed jirka ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask 2020-06-02 10:17 ` [PATCHv3] " Jiri Olsa 2020-06-02 13:42 ` Arnaldo Carvalho de Melo @ 2020-06-02 15:48 ` Ian Rogers 1 sibling, 0 replies; 17+ messages in thread From: Ian Rogers @ 2020-06-02 15:48 UTC (permalink / raw) To: Jiri Olsa Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Jin Yao, lkml, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Michael Petlan, Stephane Eranian, Andi Kleen On Tue, Jun 2, 2020 at 3:17 AM Jiri Olsa <jolsa@redhat.com> wrote: > > Jin Yao reported the issue (and posted first versions of this change) > with groups being defined over events with different cpu mask. > > This causes assert aborts in get_group_fd, like: > > # perf stat -M "C2_Pkg_Residency" -a -- sleep 1 > perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed. > Aborted > > All the events in the group have to be defined over the same > cpus so the group_fd can be found for every leader/member pair. > > Adding check to ensure this condition is met and removing the > group (with warning) if we detect mixed cpus, like: > > $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}' > WARNING: event cpu maps do not match, disabling group: > anon group { power/energy-cores/, cycles } > anon group { instructions, power/energy-cores/ } > > Ian asked also for cpu maps details, it's displayed in verbose mode: > > $ sudo perf stat -e '{cycles,power/energy-cores/}' -v > WARNING: group events cpu maps do not match, disabling group: > anon group { power/energy-cores/, cycles } > power/energy-cores/: 0 > cycles: 0-7 > anon group { instructions, power/energy-cores/ } > instructions: 0-7 > power/energy-cores/: 0 > > Fixes: 6a4bb04caacc8 ("perf tools: Enable grouping logic for parsed events") > Co-developed-by: Jin Yao <yao.jin@linux.intel.com> > Signed-off-by: Jin Yao <yao.jin@linux.intel.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> Acked-by: Ian Rogers <irogers@google.com> Thanks! Ian > --- > tools/perf/builtin-stat.c | 55 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > v3 changes: > - reword the warning with Ian's suggestion > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index b2b79aa161dd..9be020e0098a 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -190,6 +190,59 @@ 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; > + char buf[1024]; > + > + evlist__for_each_entry(evlist, evsel) { > + leader = evsel->leader; > + > + /* Check that leader matches cpus with each member. */ > + if (leader == evsel) > + continue; > + if (cpus_map_matched(leader, evsel)) > + continue; > + > + /* If there's mismatch disable the group and warn user. */ > + WARN_ONCE(1, "WARNING: grouped events cpus do not match, disabling group:\n"); > + evsel__group_desc(leader, buf, sizeof(buf)); > + pr_warning(" %s\n", buf); > + > + if (verbose) { > + cpu_map__snprint(leader->core.cpus, buf, sizeof(buf)); > + pr_warning(" %s: %s\n", leader->name, buf); > + cpu_map__snprint(evsel->core.cpus, buf, sizeof(buf)); > + pr_warning(" %s: %s\n", evsel->name, 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) > { > @@ -2113,6 +2166,8 @@ int cmd_stat(int argc, const char **argv) > goto out; > } > > + evlist__check_cpu_maps(evsel_list); > + > /* > * Initialize thread_map with comm names, > * so we could print it out on output. > -- > 2.25.4 > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-06-02 15:48 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-31 16:22 [PATCH] perf stat: Ensure group is defined on top of the same cpu mask Jiri Olsa 2020-06-01 0:04 ` Ian Rogers 2020-06-01 7:40 ` Jiri Olsa 2020-06-01 8:15 ` Jiri Olsa 2020-06-01 8:20 ` [PATCHv2] " Jiri Olsa 2020-06-01 16:20 ` Ian Rogers 2020-06-02 2:47 ` Namhyung Kim 2020-06-02 8:15 ` Jiri Olsa 2020-06-02 11:50 ` Namhyung Kim 2020-06-02 12:10 ` Jiri Olsa 2020-06-02 13:25 ` Arnaldo Carvalho de Melo 2020-06-02 10:17 ` [PATCHv3] " Jiri Olsa 2020-06-02 13:42 ` Arnaldo Carvalho de Melo 2020-06-02 14:05 ` Jiri Olsa 2020-06-02 15:03 ` Arnaldo Carvalho de Melo 2020-06-02 15:28 ` Jiri Olsa 2020-06-02 15:48 ` Ian Rogers
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.