[PATCH v3 0/5] NMI warning and debug improvements

[PATCH v3 0/5] NMI warning and debug improvements

[Ian Rogers]

The NMI warning wouldn't fire even if all the events were for one PMU
type. Remove a nearby, and no longer useful, mixed hardware event
group function. Improve the evlist to string function and dump it in
verbose mode after the reordered events warning.

As commonly happens legacy events like instructions will be uniquified
to hybrid events like cpu_core/instructions/, even though the
encodings differ. To make this correct either:
https://lore.kernel.org/lkml/20250312211623.2495798-1-irogers@google.com/
or:
https://lore.kernel.org/linux-perf-users/20250109222109.567031-1-irogers@google.com/
needs merging.

v3: Increase the verbose dump length from 1024 to 2048 as requested by
    Kan Liang.

v2: Rename evlist__has_hybrid to evlist__has_hybrid_pmus and add a
    max_length parameter to evlist__format_evsels as suggested by Kan
    Liang.

Ian Rogers (5):
  perf stat: Better hybrid support for the NMI watchdog warning
  perf stat: Remove print_mixed_hw_group_error
  perf evlist: Refactor evlist__scnprintf_evsels
  perf evlist: Add groups to evlist__format_evsels
  perf parse-events: Add debug dump of evlist if reordered

 tools/perf/builtin-record.c    |  9 ++++---
 tools/perf/util/evlist.c       | 34 ++++++++++++++++-------
 tools/perf/util/evlist.h       |  3 ++-
 tools/perf/util/parse-events.c | 16 ++++++++---
 tools/perf/util/stat-display.c | 49 +++++++++++-----------------------
 tools/perf/util/stat.h         |  1 -
 6 files changed, 60 insertions(+), 52 deletions(-)

-- 
2.49.0.504.g3bcea36a83-goog

[PATCH v3 1/5] perf stat: Better hybrid support for the NMI watchdog warning

[Ian Rogers]

Prior to this patch evlist__has_hybrid would return false if the
processor wasn't hybrid or the evlist didn't contain any core
events. If the only PMU used by events was cpu_core then it would
true even though there are no cpu_atom events. For example:

```
$ perf stat --cputype=cpu_core -e '{cycles,cycles,cycles,cycles,cycles,cycles,cycles,cycles,cycles}' true

 Performance counter stats for 'true':

     <not counted>      cpu_core/cycles/                                                        (0.00%)
     <not counted>      cpu_core/cycles/                                                        (0.00%)
     <not counted>      cpu_core/cycles/                                                        (0.00%)
     <not counted>      cpu_core/cycles/                                                        (0.00%)
     <not counted>      cpu_core/cycles/                                                        (0.00%)
     <not counted>      cpu_core/cycles/                                                        (0.00%)
     <not counted>      cpu_core/cycles/                                                        (0.00%)
     <not counted>      cpu_core/cycles/                                                        (0.00%)
     <not counted>      cpu_core/cycles/                                                        (0.00%)

       0.001981900 seconds time elapsed

       0.002311000 seconds user
       0.000000000 seconds sys
```

This patch changes evlist__has_hybrid to return true only if the
evlist contains events from >1 core PMU. This means the NMI watchdog
warning is shown for the case above.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/stat-display.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index e852ac0d9847..1115efb380fb 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -822,16 +822,28 @@ static bool is_mixed_hw_group(struct evsel *counter)
 	return false;
 }
 
-static bool evlist__has_hybrid(struct evlist *evlist)
+static bool evlist__has_hybrid_pmus(struct evlist *evlist)
 {
 	struct evsel *evsel;
+	struct perf_pmu *last_core_pmu = NULL;
 
 	if (perf_pmus__num_core_pmus() == 1)
 		return false;
 
 	evlist__for_each_entry(evlist, evsel) {
-		if (evsel->core.is_pmu_core)
+		if (evsel->core.is_pmu_core) {
+			struct perf_pmu *pmu = evsel__find_pmu(evsel);
+
+			if (pmu == last_core_pmu)
+				continue;
+
+			if (last_core_pmu == NULL) {
+				last_core_pmu = pmu;
+				continue;
+			}
+			/* A distinct core PMU. */
 			return true;
+		}
 	}
 
 	return false;
@@ -872,7 +884,7 @@ static void printout(struct perf_stat_config *config, struct outstate *os,
 		ok = false;
 
 		if (counter->supported) {
-			if (!evlist__has_hybrid(counter->evlist)) {
+			if (!evlist__has_hybrid_pmus(counter->evlist)) {
 				config->print_free_counters_hint = 1;
 				if (is_mixed_hw_group(counter))
 					config->print_mixed_hw_group_error = 1;
-- 
2.49.0.504.g3bcea36a83-goog

[PATCH v3 2/5] perf stat: Remove print_mixed_hw_group_error

[Ian Rogers]

print_mixed_hw_group_error will print a warning when a group of events
uses different PMUs. This isn't possible to happen as
parse_events__sort_events_and_fix_groups will break groups when this
happens, adding the warning at the start of perf of:

  WARNING: events were regrouped to match PMUs

As the previous mixed group warning can never happen, remove the
associated code.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/stat-display.c | 31 -------------------------------
 tools/perf/util/stat.h         |  1 -
 2 files changed, 32 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 1115efb380fb..aab3697aed63 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -798,30 +798,6 @@ static void abs_printout(struct perf_stat_config *config,
 	print_cgroup(config, os, evsel->cgrp);
 }
 
-static bool is_mixed_hw_group(struct evsel *counter)
-{
-	struct evlist *evlist = counter->evlist;
-	u32 pmu_type = counter->core.attr.type;
-	struct evsel *pos;
-
-	if (counter->core.nr_members < 2)
-		return false;
-
-	evlist__for_each_entry(evlist, pos) {
-		/* software events can be part of any hardware group */
-		if (pos->core.attr.type == PERF_TYPE_SOFTWARE)
-			continue;
-		if (pmu_type == PERF_TYPE_SOFTWARE) {
-			pmu_type = pos->core.attr.type;
-			continue;
-		}
-		if (pmu_type != pos->core.attr.type)
-			return true;
-	}
-
-	return false;
-}
-
 static bool evlist__has_hybrid_pmus(struct evlist *evlist)
 {
 	struct evsel *evsel;
@@ -886,8 +862,6 @@ static void printout(struct perf_stat_config *config, struct outstate *os,
 		if (counter->supported) {
 			if (!evlist__has_hybrid_pmus(counter->evlist)) {
 				config->print_free_counters_hint = 1;
-				if (is_mixed_hw_group(counter))
-					config->print_mixed_hw_group_error = 1;
 			}
 		}
 	}
@@ -1587,11 +1561,6 @@ static void print_footer(struct perf_stat_config *config)
 "	echo 0 > /proc/sys/kernel/nmi_watchdog\n"
 "	perf stat ...\n"
 "	echo 1 > /proc/sys/kernel/nmi_watchdog\n");
-
-	if (config->print_mixed_hw_group_error)
-		fprintf(output,
-			"The events in group usually have to be from "
-			"the same PMU. Try reorganizing the group.\n");
 }
 
 static void print_percore(struct perf_stat_config *config,
diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
index 2fda9acd7374..1bcd7634bf47 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -100,7 +100,6 @@ struct perf_stat_config {
 	int			 times;
 	int			 run_count;
 	int			 print_free_counters_hint;
-	int			 print_mixed_hw_group_error;
 	const char		*csv_sep;
 	struct stats		*walltime_nsecs_stats;
 	struct rusage		 ru_data;
-- 
2.49.0.504.g3bcea36a83-goog

[PATCH v3 3/5] perf evlist: Refactor evlist__scnprintf_evsels

[Ian Rogers]

Switch output to using a strbuf so the storage can be resized. Add a
maximum size argument to avoid too much output that may happen for
uncore events. Rename as scnprintf is no longer used.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-record.c |  9 ++++++---
 tools/perf/util/evlist.c    | 21 ++++++++++++---------
 tools/perf/util/evlist.h    |  3 ++-
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ba20bf7c011d..9a3f7d9f881d 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -51,6 +51,7 @@
 #include "util/clockid.h"
 #include "util/off_cpu.h"
 #include "util/bpf-filter.h"
+#include "util/strbuf.h"
 #include "asm/bug.h"
 #include "perf.h"
 #include "cputopo.h"
@@ -2784,13 +2785,15 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 		record__auxtrace_snapshot_exit(rec);
 
 	if (forks && workload_exec_errno) {
-		char msg[STRERR_BUFSIZE], strevsels[2048];
+		char msg[STRERR_BUFSIZE];
 		const char *emsg = str_error_r(workload_exec_errno, msg, sizeof(msg));
+		struct strbuf sb = STRBUF_INIT;
 
-		evlist__scnprintf_evsels(rec->evlist, sizeof(strevsels), strevsels);
+		evlist__format_evsels(rec->evlist, &sb, 2048);
 
 		pr_err("Failed to collect '%s' for the '%s' workload: %s\n",
-			strevsels, argv[0], emsg);
+			sb.buf, argv[0], emsg);
+		strbuf_release(&sb);
 		err = -1;
 		goto out_child;
 	}
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index c1a04141aed0..ffb5be41a465 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -35,6 +35,7 @@
 #include "util/util.h"
 #include "util/env.h"
 #include "util/intel-tpebs.h"
+#include "util/strbuf.h"
 #include <signal.h>
 #include <unistd.h>
 #include <sched.h>
@@ -2468,23 +2469,25 @@ struct evsel *evlist__find_evsel(struct evlist *evlist, int idx)
 	return NULL;
 }
 
-int evlist__scnprintf_evsels(struct evlist *evlist, size_t size, char *bf)
+void evlist__format_evsels(struct evlist *evlist, struct strbuf *sb, size_t max_length)
 {
 	struct evsel *evsel;
-	int printed = 0;
+	bool first = true;
 
 	evlist__for_each_entry(evlist, evsel) {
 		if (evsel__is_dummy_event(evsel))
 			continue;
-		if (size > (strlen(evsel__name(evsel)) + (printed ? 2 : 1))) {
-			printed += scnprintf(bf + printed, size - printed, "%s%s", printed ? "," : "", evsel__name(evsel));
-		} else {
-			printed += scnprintf(bf + printed, size - printed, "%s...", printed ? "," : "");
-			break;
+
+		if (!first)
+			strbuf_addch(sb, ',');
+
+		if (sb->len > max_length) {
+			strbuf_addstr(sb, "...");
+			return;
 		}
+		strbuf_addstr(sb, evsel__name(evsel));
+		first = false;
 	}
-
-	return printed;
 }
 
 void evlist__check_mem_load_aux(struct evlist *evlist)
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index edcbf1c10e92..21f6bff319fd 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -20,6 +20,7 @@ struct pollfd;
 struct thread_map;
 struct perf_cpu_map;
 struct record_opts;
+struct strbuf;
 struct target;
 
 /*
@@ -430,7 +431,7 @@ int event_enable_timer__process(struct event_enable_timer *eet);
 
 struct evsel *evlist__find_evsel(struct evlist *evlist, int idx);
 
-int evlist__scnprintf_evsels(struct evlist *evlist, size_t size, char *bf);
+void evlist__format_evsels(struct evlist *evlist, struct strbuf *sb, size_t max_length);
 void evlist__check_mem_load_aux(struct evlist *evlist);
 void evlist__warn_user_requested_cpus(struct evlist *evlist, const char *cpu_list);
 void evlist__uniquify_name(struct evlist *evlist);
-- 
2.49.0.504.g3bcea36a83-goog

[PATCH v3 4/5] perf evlist: Add groups to evlist__format_evsels

[Ian Rogers]

Make groups visible in output:

Before:
{cycles,instructions} ->
cpu_atom/cycles/,cpu_atom/instructions/,cpu_core/cycles/,cpu_core/instructions/

After:
{cycles,instructions} ->
{cpu_atom/cycles/,cpu_atom/instructions/},{cpu_core/cycles/,cpu_core/instructions/}

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evlist.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index ffb5be41a465..6ebe0795e1c9 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -2471,13 +2471,18 @@ struct evsel *evlist__find_evsel(struct evlist *evlist, int idx)
 
 void evlist__format_evsels(struct evlist *evlist, struct strbuf *sb, size_t max_length)
 {
-	struct evsel *evsel;
+	struct evsel *evsel, *leader = NULL;
 	bool first = true;
 
 	evlist__for_each_entry(evlist, evsel) {
+		struct evsel *new_leader = evsel__leader(evsel);
+
 		if (evsel__is_dummy_event(evsel))
 			continue;
 
+		if (leader != new_leader && leader && leader->core.nr_members > 1)
+			strbuf_addch(sb, '}');
+
 		if (!first)
 			strbuf_addch(sb, ',');
 
@@ -2485,9 +2490,15 @@ void evlist__format_evsels(struct evlist *evlist, struct strbuf *sb, size_t max_
 			strbuf_addstr(sb, "...");
 			return;
 		}
+		if (leader != new_leader && new_leader->core.nr_members > 1)
+			strbuf_addch(sb, '{');
+
 		strbuf_addstr(sb, evsel__name(evsel));
 		first = false;
+		leader = new_leader;
 	}
+	if (leader && leader->core.nr_members > 1)
+		strbuf_addch(sb, '}');
 }
 
 void evlist__check_mem_load_aux(struct evlist *evlist)
-- 
2.49.0.504.g3bcea36a83-goog

[PATCH v3 5/5] perf parse-events: Add debug dump of evlist if reordered

[Ian Rogers]

Add debug verbose output to show how evsels were reordered by
parse_events__sort_events_and_fix_groups. For example:
```
$ perf record -v -e '{instructions,cycles}' true
Using CPUID GenuineIntel-6-B7-1
WARNING: events were regrouped to match PMUs
evlist after sorting/fixing: '{cpu_atom/instructions/,cpu_atom/cycles/},{cpu_core/instructions/,cpu_core/cycles/}'
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 5152fd5a6ead..7952f431f82b 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -28,6 +28,7 @@
 #include "util/evsel_config.h"
 #include "util/event.h"
 #include "util/bpf-filter.h"
+#include "util/stat.h"
 #include "util/util.h"
 #include "tracepoint.h"
 
@@ -2196,14 +2197,23 @@ int __parse_events(struct evlist *evlist, const char *str, const char *pmu_filte
 	if (ret2 < 0)
 		return ret;
 
-	if (ret2 && warn_if_reordered && !parse_state.wild_card_pmus)
-		pr_warning("WARNING: events were regrouped to match PMUs\n");
-
 	/*
 	 * Add list to the evlist even with errors to allow callers to clean up.
 	 */
 	evlist__splice_list_tail(evlist, &parse_state.list);
 
+	if (ret2 && warn_if_reordered && !parse_state.wild_card_pmus) {
+		pr_warning("WARNING: events were regrouped to match PMUs\n");
+
+		if (verbose > 0) {
+			struct strbuf sb = STRBUF_INIT;
+
+			evlist__uniquify_name(evlist);
+			evlist__format_evsels(evlist, &sb, 2048);
+			pr_debug("evlist after sorting/fixing: '%s'\n", sb.buf);
+			strbuf_release(&sb);
+		}
+	}
 	if (!ret) {
 		struct evsel *last;
 
-- 
2.49.0.504.g3bcea36a83-goog

Re: [PATCH v3 0/5] NMI warning and debug improvements

[Liang, Kan]

On 2025-04-02 4:15 p.m., Ian Rogers wrote:
> The NMI warning wouldn't fire even if all the events were for one PMU
> type. Remove a nearby, and no longer useful, mixed hardware event
> group function. Improve the evlist to string function and dump it in
> verbose mode after the reordered events warning.
> 
> As commonly happens legacy events like instructions will be uniquified
> to hybrid events like cpu_core/instructions/, even though the
> encodings differ. To make this correct either:
> https://lore.kernel.org/lkml/20250312211623.2495798-1-irogers@google.com/
> or:
> https://lore.kernel.org/linux-perf-users/20250109222109.567031-1-irogers@google.com/
> needs merging.
> 
> v3: Increase the verbose dump length from 1024 to 2048 as requested by
>     Kan Liang.
> 
> v2: Rename evlist__has_hybrid to evlist__has_hybrid_pmus and add a
>     max_length parameter to evlist__format_evsels as suggested by Kan
>     Liang.
> 
> Ian Rogers (5):
>   perf stat: Better hybrid support for the NMI watchdog warning
>   perf stat: Remove print_mixed_hw_group_error
>   perf evlist: Refactor evlist__scnprintf_evsels
>   perf evlist: Add groups to evlist__format_evsels
>   perf parse-events: Add debug dump of evlist if reordered

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan

> 
>  tools/perf/builtin-record.c    |  9 ++++---
>  tools/perf/util/evlist.c       | 34 ++++++++++++++++-------
>  tools/perf/util/evlist.h       |  3 ++-
>  tools/perf/util/parse-events.c | 16 ++++++++---
>  tools/perf/util/stat-display.c | 49 +++++++++++-----------------------
>  tools/perf/util/stat.h         |  1 -
>  6 files changed, 60 insertions(+), 52 deletions(-)
> 

Re: [PATCH v3 0/5] NMI warning and debug improvements

[Ian Rogers]

On Wed, Apr 2, 2025 at 1:33 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
> On 2025-04-02 4:15 p.m., Ian Rogers wrote:
> > The NMI warning wouldn't fire even if all the events were for one PMU
> > type. Remove a nearby, and no longer useful, mixed hardware event
> > group function. Improve the evlist to string function and dump it in
> > verbose mode after the reordered events warning.
> >
> > As commonly happens legacy events like instructions will be uniquified
> > to hybrid events like cpu_core/instructions/, even though the
> > encodings differ. To make this correct either:
> > https://lore.kernel.org/lkml/20250312211623.2495798-1-irogers@google.com/
> > or:
> > https://lore.kernel.org/linux-perf-users/20250109222109.567031-1-irogers@google.com/
> > needs merging.
> >
> > v3: Increase the verbose dump length from 1024 to 2048 as requested by
> >     Kan Liang.
> >
> > v2: Rename evlist__has_hybrid to evlist__has_hybrid_pmus and add a
> >     max_length parameter to evlist__format_evsels as suggested by Kan
> >     Liang.
> >
> > Ian Rogers (5):
> >   perf stat: Better hybrid support for the NMI watchdog warning
> >   perf stat: Remove print_mixed_hw_group_error
> >   perf evlist: Refactor evlist__scnprintf_evsels
> >   perf evlist: Add groups to evlist__format_evsels
> >   perf parse-events: Add debug dump of evlist if reordered
>
> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Ping. The v3 patch series (this series) is here:
https://lore.kernel.org/lkml/20250402201549.4090305-1-irogers@google.com/

Thanks,
Ian

> Thanks,
> Kan
>
> >
> >  tools/perf/builtin-record.c    |  9 ++++---
> >  tools/perf/util/evlist.c       | 34 ++++++++++++++++-------
> >  tools/perf/util/evlist.h       |  3 ++-
> >  tools/perf/util/parse-events.c | 16 ++++++++---
> >  tools/perf/util/stat-display.c | 49 +++++++++++-----------------------
> >  tools/perf/util/stat.h         |  1 -
> >  6 files changed, 60 insertions(+), 52 deletions(-)
> >
>

Re: [PATCH v3 0/5] NMI warning and debug improvements

[Arnaldo Carvalho de Melo]

On Wed, Apr 02, 2025 at 04:33:25PM -0400, Liang, Kan wrote:
> 
> 
> On 2025-04-02 4:15 p.m., Ian Rogers wrote:
> > The NMI warning wouldn't fire even if all the events were for one PMU
> > type. Remove a nearby, and no longer useful, mixed hardware event
> > group function. Improve the evlist to string function and dump it in
> > verbose mode after the reordered events warning.
> > 
> > As commonly happens legacy events like instructions will be uniquified
> > to hybrid events like cpu_core/instructions/, even though the
> > encodings differ. To make this correct either:
> > https://lore.kernel.org/lkml/20250312211623.2495798-1-irogers@google.com/
> > or:
> > https://lore.kernel.org/linux-perf-users/20250109222109.567031-1-irogers@google.com/
> > needs merging.
> > 
> > v3: Increase the verbose dump length from 1024 to 2048 as requested by
> >     Kan Liang.
> > 
> > v2: Rename evlist__has_hybrid to evlist__has_hybrid_pmus and add a
> >     max_length parameter to evlist__format_evsels as suggested by Kan
> >     Liang.
> > 
> > Ian Rogers (5):
> >   perf stat: Better hybrid support for the NMI watchdog warning
> >   perf stat: Remove print_mixed_hw_group_error
> >   perf evlist: Refactor evlist__scnprintf_evsels
> >   perf evlist: Add groups to evlist__format_evsels
> >   perf parse-events: Add debug dump of evlist if reordered
> 
> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks, applied to perf-tools-next,

- Arnaldo