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

[PATCH v1 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.

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       | 32 +++++++++++++++---------
 tools/perf/util/evlist.h       |  3 ++-
 tools/perf/util/parse-events.c | 16 +++++++++---
 tools/perf/util/stat-display.c | 45 ++++++++++------------------------
 tools/perf/util/stat.h         |  1 -
 6 files changed, 55 insertions(+), 51 deletions(-)

-- 
2.49.0.rc1.451.g8f38331e32-goog

[PATCH v1 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 | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index e852ac0d9847..f311f1960e29 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -825,13 +825,25 @@ static bool is_mixed_hw_group(struct evsel *counter)
 static bool evlist__has_hybrid(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;
-- 
2.49.0.rc1.451.g8f38331e32-goog

[PATCH v1 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 f311f1960e29..1751a450f449 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(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(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.rc1.451.g8f38331e32-goog

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

[Ian Rogers]

Switch output to using a strbuf so the storage can be resized. 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    | 19 +++++++++----------
 tools/perf/util/evlist.h    |  3 ++-
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ba20bf7c011d..cea5959adadc 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);
 
 		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 49e10d6981ad..96cfc7ed1512 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,21 @@ 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)
 {
 	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;
-		}
-	}
 
-	return printed;
+		if (!first)
+			strbuf_addch(sb, ',');
+
+		strbuf_addstr(sb, evsel__name(evsel));
+		first = false;
+	}
 }
 
 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..5fe5cfbbebb1 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);
 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.rc1.451.g8f38331e32-goog

[PATCH v1 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 96cfc7ed1512..b59fa407be44 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -2471,19 +2471,30 @@ struct evsel *evlist__find_evsel(struct evlist *evlist, int idx)
 
 void evlist__format_evsels(struct evlist *evlist, struct strbuf *sb)
 {
-	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, ',');
 
+		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.rc1.451.g8f38331e32-goog

[PATCH v1 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..cb76796bc5c7 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);
+			pr_debug("evlist after sorting/fixing: '%s'\n", sb.buf);
+			strbuf_release(&sb);
+		}
+	}
 	if (!ret) {
 		struct evsel *last;
 
-- 
2.49.0.rc1.451.g8f38331e32-goog

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

[Ian Rogers]

On Mon, Mar 17, 2025 at 9:14 PM Ian Rogers <irogers@google.com> 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.
>
> 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

Ping.

Thanks,
Ian

>  tools/perf/builtin-record.c    |  9 ++++---
>  tools/perf/util/evlist.c       | 32 +++++++++++++++---------
>  tools/perf/util/evlist.h       |  3 ++-
>  tools/perf/util/parse-events.c | 16 +++++++++---
>  tools/perf/util/stat-display.c | 45 ++++++++++------------------------
>  tools/perf/util/stat.h         |  1 -
>  6 files changed, 55 insertions(+), 51 deletions(-)
>
> --
> 2.49.0.rc1.451.g8f38331e32-goog
>

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

[Liang, Kan]

On 2025-03-18 12:14 a.m., Ian Rogers wrote:
> 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.

Nit:
The function name may still bring confusions.
It may be better to change the function name as well, e.g.,
evlist__has_hybrid_pmus()? It implies more than one PMU.

Thanks,
Kan>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/stat-display.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index e852ac0d9847..f311f1960e29 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -825,13 +825,25 @@ static bool is_mixed_hw_group(struct evsel *counter)
>  static bool evlist__has_hybrid(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;

Re: [PATCH v1 3/5] perf evlist: Refactor evlist__scnprintf_evsels

[Liang, Kan]

On 2025-03-18 12:14 a.m., Ian Rogers wrote:
> Switch output to using a strbuf so the storage can be resized. 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    | 19 +++++++++----------
>  tools/perf/util/evlist.h    |  3 ++-
>  3 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index ba20bf7c011d..cea5959adadc 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);
>  
>  		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 49e10d6981ad..96cfc7ed1512 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,21 @@ 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)
>  {
>  	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;
> -		}
> -	}
>  
> -	return printed;
> +		if (!first)
> +			strbuf_addch(sb, ',');
> +> +		strbuf_addstr(sb, evsel__name(evsel));

The evlist may include hundreds of events. The error msg will be too
huge for the case.

Thanks,
Kan

> +		first = false;
> +	}
>  }
>  
>  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..5fe5cfbbebb1 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);
>  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);

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

[Liang, Kan]

On 2025-03-18 12:14 a.m., Ian Rogers wrote:
> 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/}'
> ```
>

Thanks. This is very nice.
> 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..cb76796bc5c7 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);

Ah, I know why you want a full list now.

A full list should be OK for pr_debug. But it may be a problem for
pr_err in patch 3. Maybe add a parameter in evlist__format_evsels() to
control if printing the full list.

Thanks,
Kan

> +			pr_debug("evlist after sorting/fixing: '%s'\n", sb.buf);
> +			strbuf_release(&sb);
> +		}
> +	}
>  	if (!ret) {
>  		struct evsel *last;
>  

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

[Ian Rogers]

On Wed, Apr 2, 2025 at 8:23 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2025-03-18 12:14 a.m., Ian Rogers wrote:
> > 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.
>
> Nit:
> The function name may still bring confusions.
> It may be better to change the function name as well, e.g.,
> evlist__has_hybrid_pmus()? It implies more than one PMU.

Thanks, I'll change in v2.

Ian

> Thanks,
> Kan>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/stat-display.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> > index e852ac0d9847..f311f1960e29 100644
> > --- a/tools/perf/util/stat-display.c
> > +++ b/tools/perf/util/stat-display.c
> > @@ -825,13 +825,25 @@ static bool is_mixed_hw_group(struct evsel *counter)
> >  static bool evlist__has_hybrid(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;
>

Re: [PATCH v1 3/5] perf evlist: Refactor evlist__scnprintf_evsels

[Ian Rogers]

On Wed, Apr 2, 2025 at 8:26 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2025-03-18 12:14 a.m., Ian Rogers wrote:
> > Switch output to using a strbuf so the storage can be resized. 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    | 19 +++++++++----------
> >  tools/perf/util/evlist.h    |  3 ++-
> >  3 files changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index ba20bf7c011d..cea5959adadc 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);
> >
> >               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 49e10d6981ad..96cfc7ed1512 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,21 @@ 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)
> >  {
> >       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;
> > -             }
> > -     }
> >
> > -     return printed;
> > +             if (!first)
> > +                     strbuf_addch(sb, ',');
> > +> +          strbuf_addstr(sb, evsel__name(evsel));
>
> The evlist may include hundreds of events. The error msg will be too
> huge for the case.

I hear you, lots of uncore PMUs :-) I keep wanting to the PMU with an
evsel to be a list. As you suggest in the next feedback, let's add an
option to have some maximum for the perf record use case.

Thanks,
Ian


> Thanks,
> Kan
>
> > +             first = false;
> > +     }
> >  }
> >
> >  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..5fe5cfbbebb1 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);
> >  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);
>