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 <[email protected]>
Signed-off-by: Jin Yao <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
---
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
On Sun, May 31, 2020 at 9:22 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> Signed-off-by: Jin Yao <[email protected]>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> 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
>
On Sun, May 31, 2020 at 05:04:47PM -0700, Ian Rogers wrote:
> On Sun, May 31, 2020 at 9:22 AM Jiri Olsa <[email protected]> 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
On Sun, May 31, 2020 at 05:04:47PM -0700, Ian Rogers wrote:
> On Sun, May 31, 2020 at 9:22 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> > Signed-off-by: Jin Yao <[email protected]>
> > Signed-off-by: Jiri Olsa <[email protected]>
> > ---
> > 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
> >
>
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 <[email protected]>
Signed-off-by: Jin Yao <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
---
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
On Mon, Jun 1, 2020 at 1:20 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> Signed-off-by: Jin Yao <[email protected]>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> 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
>
On Tue, Jun 2, 2020 at 1:21 AM Ian Rogers <[email protected]> wrote:
>
> On Mon, Jun 1, 2020 at 1:20 AM Jiri Olsa <[email protected]> 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
On Tue, Jun 02, 2020 at 11:47:19AM +0900, Namhyung Kim wrote:
> On Tue, Jun 2, 2020 at 1:21 AM Ian Rogers <[email protected]> wrote:
> >
> > On Mon, Jun 1, 2020 at 1:20 AM Jiri Olsa <[email protected]> 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
>
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 <[email protected]>
Signed-off-by: Jin Yao <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
---
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
Hi Jiri,
On Tue, Jun 2, 2020 at 5:16 PM Jiri Olsa <[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Mon, Jun 1, 2020 at 1:20 AM Jiri Olsa <[email protected]> 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
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 <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > On Mon, Jun 1, 2020 at 1:20 AM Jiri Olsa <[email protected]> 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
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 <[email protected]> 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 <[email protected]> wrote:
> > > > >
> > > > > On Mon, Jun 1, 2020 at 1:20 AM Jiri Olsa <[email protected]> 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
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 <[email protected]>
> Signed-off-by: Jin Yao <[email protected]>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> 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
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
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
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
On Tue, Jun 2, 2020 at 3:17 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> Signed-off-by: Jin Yao <[email protected]>
> Signed-off-by: Jiri Olsa <[email protected]>
Acked-by: Ian Rogers <[email protected]>
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
>