[PATCH] perf stat: Ensure group is defined on top of the same cpu mask

[PATCH] perf stat: Ensure group is defined on top of the same cpu mask

[Jiri Olsa]

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

Re: [PATCH] perf stat: Ensure group is defined on top of the same cpu mask

[Ian Rogers]

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
>

Re: [PATCH] perf stat: Ensure group is defined on top of the same cpu mask

[Jiri Olsa]

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

Re: [PATCH] perf stat: Ensure group is defined on top of the same cpu mask

[Jiri Olsa]

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
> >
> 

[PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask

[Jiri Olsa]

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

Re: [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask

[Ian Rogers]

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
>

Re: [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask

[Namhyung Kim]

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

Re: [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask

[Jiri Olsa]

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
> 

[PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask

[Jiri Olsa]

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

Re: [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask

[Namhyung Kim]

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

Re: [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask

[Jiri Olsa]

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

Re: [PATCHv2] perf stat: Ensure group is defined on top of the same cpu mask

[Arnaldo Carvalho de Melo]

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

Re: [PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask

[Arnaldo Carvalho de Melo]

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

Re: [PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask

[Jiri Olsa]

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

Re: [PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask

[Arnaldo Carvalho de Melo]

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

Re: [PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask

[Jiri Olsa]

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

Re: [PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask

[Ian Rogers]

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
>