[PATCH v2 0/3] Fix incorrect counts when count the same uncore event multiple times

[PATCH v2 0/3] Fix incorrect counts when count the same uncore event multiple times

[Chun-Tse Shao]

Let's take a look an example, the machine is SKX with 6 IMC devices.

  perf stat -e clockticks,clockticks -I 1000
  #           time             counts unit events
       1.001127430      6,901,503,174      uncore_imc_0/clockticks/
       1.001127430      3,940,896,301      uncore_imc_0/clockticks/
       2.002649722        988,376,876      uncore_imc_0/clockticks/
       2.002649722        988,376,141      uncore_imc_0/clockticks/
       3.004071319      1,000,292,675      uncore_imc_0/clockticks/
       3.004071319      1,000,294,160      uncore_imc_0/clockticks/

1) The events name should not be uniquified.
2) The initial count for the first `clockticks` is doubled.
3) Subsequent count only report for the first IMC device.

The first patch fixes 1) and 3), and the second patch fixes 2).

After these fix:

  perf stat -e clockticks,clockticks -I 1000
  #           time             counts unit events
       1.001127586      4,126,938,857      clockticks
       1.001127586      4,121,564,277      clockticks
       2.001686014      3,953,806,350      clockticks
       2.001686014      3,953,809,541      clockticks
       3.003121403      4,137,750,252      clockticks
       3.003121403      4,137,749,048      clockticks

I also tested `-A`, `--per-socket`, `--per-die` and `--per-core`, all
looks good.

Ian tested `hybrid-merge` and `hwmon`, all looks good as well.

Chun-Tse Shao (1):
  perf test: Add stat uniquifying test

Ian Rogers (2):
  perf evlist: Make uniquifying counter names consistent
  perf parse-events: Use wildcard processing to set an event to merge
    into

 tools/perf/builtin-record.c                   |   7 +-
 tools/perf/builtin-top.c                      |   7 +-
 .../tests/shell/stat+event_uniquifying.sh     |  69 ++++++++
 tools/perf/util/evlist.c                      |  66 +++++---
 tools/perf/util/evlist.h                      |   3 +-
 tools/perf/util/evsel.c                       | 119 ++++++++++++-
 tools/perf/util/evsel.h                       |  11 +-
 tools/perf/util/parse-events.c                |  86 +++++++---
 tools/perf/util/stat-display.c                | 160 ++----------------
 tools/perf/util/stat.c                        |  40 +----
 10 files changed, 329 insertions(+), 239 deletions(-)
 create mode 100755 tools/perf/tests/shell/stat+event_uniquifying.sh

--
v2:
  - Fixes for `hwmon` and `--hybrid-merge`.
  - Add a test for event uniquifying.

v1: lore.kernel.org/20250326234758.480431-1-ctshao@google.com
2.49.0.472.ge94155a9ec-goog

[PATCH v2 1/3] perf evlist: Make uniquifying counter names consistent

[Chun-Tse Shao]

From: Ian Rogers <irogers@google.com>

perf stat has different uniquification logic to perf record and perf
top. In the case of perf record and perf top all hybrid event
names are uniquified. Perf stat is more disciplined respecting
name config terms, libpfm4 events, etc. Perf stat will uniquify
hybrid events and the non-core PMU cases shouldn't apply to perf
record or perf top. For consistency, remove the uniquification for
perf record and perf top and reuse the perf stat uniquification,
making the code more globally visible for this.

Fix the detection of cross-PMU for disabling uniquify by correctly
setting last_pmu. When setting uniquify on an evsel, make sure the
PMUs between the 2 considered events differ otherwise the uniquify
isn't adding value.

Signed-off-by: Ian Rogers <irogers@google.com>
Tested-by: Chun-Tse Shao <ctshao@google.com>
---
 tools/perf/builtin-record.c    |   7 +-
 tools/perf/builtin-top.c       |   7 +-
 tools/perf/util/evlist.c       |  66 ++++++++++-----
 tools/perf/util/evlist.h       |   3 +-
 tools/perf/util/evsel.c        | 113 +++++++++++++++++++++++++
 tools/perf/util/evsel.h        |   4 +
 tools/perf/util/stat-display.c | 149 +--------------------------------
 7 files changed, 176 insertions(+), 173 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ba20bf7c011d..83f8f9728e12 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -26,6 +26,7 @@
 #include "util/target.h"
 #include "util/session.h"
 #include "util/tool.h"
+#include "util/stat.h"
 #include "util/symbol.h"
 #include "util/record.h"
 #include "util/cpumap.h"
@@ -2483,7 +2484,11 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 		pr_warning("WARNING: --timestamp-filename option is not available in pipe mode.\n");
 	}
 
-	evlist__uniquify_name(rec->evlist);
+	/*
+	 * Use global stat_config that is zero meaning aggr_mode is AGGR_NONE
+	 * and hybrid_merge is false.
+	 */
+	evlist__uniquify_evsel_names(rec->evlist, &stat_config);
 
 	evlist__config(rec->evlist, opts, &callchain_param);
 
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 1061f4eebc3f..b7c8ecafa2f2 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -35,6 +35,7 @@
 #include "util/mmap.h"
 #include "util/session.h"
 #include "util/thread.h"
+#include "util/stat.h"
 #include "util/symbol.h"
 #include "util/synthetic-events.h"
 #include "util/top.h"
@@ -1309,7 +1310,11 @@ static int __cmd_top(struct perf_top *top)
 		}
 	}
 
-	evlist__uniquify_name(top->evlist);
+	/*
+	 * Use global stat_config that is zero meaning aggr_mode is AGGR_NONE
+	 * and hybrid_merge is false.
+	 */
+	evlist__uniquify_evsel_names(top->evlist, &stat_config);
 	ret = perf_top__start_counters(top);
 	if (ret)
 		return ret;
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index c1a04141aed0..a28ab52e7d85 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -2552,34 +2552,56 @@ void evlist__warn_user_requested_cpus(struct evlist *evlist, const char *cpu_lis
 	perf_cpu_map__put(user_requested_cpus);
 }
 
-void evlist__uniquify_name(struct evlist *evlist)
+/* Should uniquify be disabled for the evlist? */
+static bool evlist__disable_uniquify(const struct evlist *evlist)
 {
-	char *new_name, empty_attributes[2] = ":", *attributes;
-	struct evsel *pos;
+	struct evsel *counter;
+	struct perf_pmu *last_pmu = NULL;
+	bool first = true;
 
-	if (perf_pmus__num_core_pmus() == 1)
-		return;
+	evlist__for_each_entry(evlist, counter) {
+		/* If PMUs vary then uniquify can be useful. */
+		if (!first && counter->pmu != last_pmu)
+			return false;
+		first = false;
+		if (counter->pmu) {
+			/* Allow uniquify for uncore PMUs. */
+			if (!counter->pmu->is_core)
+				return false;
+			/* Keep hybrid event names uniquified for clarity. */
+			if (perf_pmus__num_core_pmus() > 1)
+				return false;
+		}
+		last_pmu = counter->pmu;
+	}
+	return true;
+}
 
-	evlist__for_each_entry(evlist, pos) {
-		if (!evsel__is_hybrid(pos))
-			continue;
+static bool evlist__set_needs_uniquify(struct evlist *evlist, const struct perf_stat_config *config)
+{
+	struct evsel *counter;
+	bool needs_uniquify = false;
 
-		if (strchr(pos->name, '/'))
-			continue;
+	if (evlist__disable_uniquify(evlist)) {
+		evlist__for_each_entry(evlist, counter)
+			counter->uniquified_name = true;
+		return false;
+	}
+
+	evlist__for_each_entry(evlist, counter) {
+		if (evsel__set_needs_uniquify(counter, config))
+			needs_uniquify = true;
+	}
+	return needs_uniquify;
+}
 
-		attributes = strchr(pos->name, ':');
-		if (attributes)
-			*attributes = '\0';
-		else
-			attributes = empty_attributes;
+void evlist__uniquify_evsel_names(struct evlist *evlist, const struct perf_stat_config *config)
+{
+	if (evlist__set_needs_uniquify(evlist, config)) {
+		struct evsel *pos;
 
-		if (asprintf(&new_name, "%s/%s/%s", pos->pmu ? pos->pmu->name : "",
-			     pos->name, attributes + 1)) {
-			free(pos->name);
-			pos->name = new_name;
-		} else {
-			*attributes = ':';
-		}
+		evlist__for_each_entry(evlist, pos)
+			evsel__uniquify_counter(pos);
 	}
 }
 
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index edcbf1c10e92..7cfdaf543214 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -19,6 +19,7 @@
 struct pollfd;
 struct thread_map;
 struct perf_cpu_map;
+struct perf_stat_config;
 struct record_opts;
 struct target;
 
@@ -433,7 +434,7 @@ struct evsel *evlist__find_evsel(struct evlist *evlist, int idx);
 int evlist__scnprintf_evsels(struct evlist *evlist, size_t size, char *bf);
 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);
+void evlist__uniquify_evsel_names(struct evlist *evlist, const struct perf_stat_config *config);
 bool evlist__has_bpf_output(struct evlist *evlist);
 bool evlist__needs_bpf_sb_event(struct evlist *evlist);
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 1974395492d7..f00cfff119aa 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3939,3 +3939,116 @@ void evsel__remove_from_group(struct evsel *evsel, struct evsel *leader)
 		leader->core.nr_members--;
 	}
 }
+
+bool evsel__set_needs_uniquify(struct evsel *counter, const struct perf_stat_config *config)
+{
+	struct evsel *evsel;
+
+	if (counter->needs_uniquify) {
+		/* Already set. */
+		return true;
+	}
+
+	if (counter->merged_stat) {
+		/* Counter won't be shown. */
+		return false;
+	}
+
+	if (counter->use_config_name || counter->is_libpfm_event) {
+		/* Original name will be used. */
+		return false;
+	}
+
+	if (!config->hybrid_merge && evsel__is_hybrid(counter)) {
+		/* Unique hybrid counters necessary. */
+		counter->needs_uniquify = true;
+		return true;
+	}
+
+	if  (counter->core.attr.type < PERF_TYPE_MAX && counter->core.attr.type != PERF_TYPE_RAW) {
+		/* Legacy event, don't uniquify. */
+		return false;
+	}
+
+	if (counter->pmu && counter->pmu->is_core &&
+	    counter->alternate_hw_config != PERF_COUNT_HW_MAX) {
+		/* A sysfs or json event replacing a legacy event, don't uniquify. */
+		return false;
+	}
+
+	if (config->aggr_mode == AGGR_NONE) {
+		/* Always unique with no aggregation. */
+		counter->needs_uniquify = true;
+		return true;
+	}
+
+	/*
+	 * Do other non-merged events in the evlist have the same name? If so
+	 * uniquify is necessary.
+	 */
+	evlist__for_each_entry(counter->evlist, evsel) {
+		if (evsel == counter || evsel->merged_stat || evsel->pmu == counter->pmu)
+			continue;
+
+		if (evsel__name_is(counter, evsel__name(evsel))) {
+			counter->needs_uniquify = true;
+			return true;
+		}
+	}
+	return false;
+}
+
+void evsel__uniquify_counter(struct evsel *counter)
+{
+	const char *name, *pmu_name;
+	char *new_name, *config;
+	int ret;
+
+	/* No uniquification necessary. */
+	if (!counter->needs_uniquify)
+		return;
+
+	/* The evsel was already uniquified. */
+	if (counter->uniquified_name)
+		return;
+
+	/* Avoid checking to uniquify twice. */
+	counter->uniquified_name = true;
+
+	name = evsel__name(counter);
+	pmu_name = counter->pmu->name;
+	/* Already prefixed by the PMU name. */
+	if (!strncmp(name, pmu_name, strlen(pmu_name)))
+		return;
+
+	config = strchr(name, '/');
+	if (config) {
+		int len = config - name;
+
+		if (config[1] == '/') {
+			/* case: event// */
+			ret = asprintf(&new_name, "%s/%.*s/%s", pmu_name, len, name, config + 2);
+		} else {
+			/* case: event/.../ */
+			ret = asprintf(&new_name, "%s/%.*s,%s", pmu_name, len, name, config + 1);
+		}
+	} else {
+		config = strchr(name, ':');
+		if (config) {
+			/* case: event:.. */
+			int len = config - name;
+
+			ret = asprintf(&new_name, "%s/%.*s/%s", pmu_name, len, name, config + 1);
+		} else {
+			/* case: event */
+			ret = asprintf(&new_name, "%s/%s/", pmu_name, name);
+		}
+	}
+	if (ret > 0) {
+		free(counter->name);
+		counter->name = new_name;
+	} else {
+		/* ENOMEM from asprintf. */
+		counter->uniquified_name = false;
+	}
+}
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index aae431d63d64..76ccb7c7e8c2 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -16,6 +16,7 @@
 struct bpf_object;
 struct cgroup;
 struct perf_counts;
+struct perf_stat_config;
 struct perf_stat_evsel;
 union perf_event;
 struct bpf_counter_ops;
@@ -542,6 +543,9 @@ void evsel__remove_from_group(struct evsel *evsel, struct evsel *leader);
 
 bool arch_evsel__must_be_in_group(const struct evsel *evsel);
 
+bool evsel__set_needs_uniquify(struct evsel *counter, const struct perf_stat_config *config);
+void evsel__uniquify_counter(struct evsel *counter);
+
 /*
  * Macro to swap the bit-field postition and size.
  * Used when,
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index e852ac0d9847..d427e0ca98a1 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -929,61 +929,6 @@ static void printout(struct perf_stat_config *config, struct outstate *os,
 	}
 }
 
-static void evsel__uniquify_counter(struct evsel *counter)
-{
-	const char *name, *pmu_name;
-	char *new_name, *config;
-	int ret;
-
-	/* No uniquification necessary. */
-	if (!counter->needs_uniquify)
-		return;
-
-	/* The evsel was already uniquified. */
-	if (counter->uniquified_name)
-		return;
-
-	/* Avoid checking to uniquify twice. */
-	counter->uniquified_name = true;
-
-	name = evsel__name(counter);
-	pmu_name = counter->pmu->name;
-	/* Already prefixed by the PMU name. */
-	if (!strncmp(name, pmu_name, strlen(pmu_name)))
-		return;
-
-	config = strchr(name, '/');
-	if (config) {
-		int len = config - name;
-
-		if (config[1] == '/') {
-			/* case: event// */
-			ret = asprintf(&new_name, "%s/%.*s/%s", pmu_name, len, name, config + 2);
-		} else {
-			/* case: event/.../ */
-			ret = asprintf(&new_name, "%s/%.*s,%s", pmu_name, len, name, config + 1);
-		}
-	} else {
-		config = strchr(name, ':');
-		if (config) {
-			/* case: event:.. */
-			int len = config - name;
-
-			ret = asprintf(&new_name, "%s/%.*s/%s", pmu_name, len, name, config + 1);
-		} else {
-			/* case: event */
-			ret = asprintf(&new_name, "%s/%s/", pmu_name, name);
-		}
-	}
-	if (ret > 0) {
-		free(counter->name);
-		counter->name = new_name;
-	} else {
-		/* ENOMEM from asprintf. */
-		counter->uniquified_name = false;
-	}
-}
-
 /**
  * should_skip_zero_count() - Check if the event should print 0 values.
  * @config: The perf stat configuration (including aggregation mode).
@@ -1069,8 +1014,6 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 	if (counter->merged_stat)
 		return;
 
-	evsel__uniquify_counter(counter);
-
 	val = aggr->counts.val;
 	ena = aggr->counts.ena;
 	run = aggr->counts.run;
@@ -1650,96 +1593,6 @@ static void print_cgroup_counter(struct perf_stat_config *config, struct evlist
 		print_metric_end(config, os);
 }
 
-/* Should uniquify be disabled for the evlist? */
-static bool evlist__disable_uniquify(const struct evlist *evlist)
-{
-	struct evsel *counter;
-	struct perf_pmu *last_pmu = NULL;
-	bool first = true;
-
-	evlist__for_each_entry(evlist, counter) {
-		/* If PMUs vary then uniquify can be useful. */
-		if (!first && counter->pmu != last_pmu)
-			return false;
-		first = false;
-		if (counter->pmu) {
-			/* Allow uniquify for uncore PMUs. */
-			if (!counter->pmu->is_core)
-				return false;
-			/* Keep hybrid event names uniquified for clarity. */
-			if (perf_pmus__num_core_pmus() > 1)
-				return false;
-		}
-	}
-	return true;
-}
-
-static void evsel__set_needs_uniquify(struct evsel *counter, const struct perf_stat_config *config)
-{
-	struct evsel *evsel;
-
-	if (counter->merged_stat) {
-		/* Counter won't be shown. */
-		return;
-	}
-
-	if (counter->use_config_name || counter->is_libpfm_event) {
-		/* Original name will be used. */
-		return;
-	}
-
-	if (!config->hybrid_merge && evsel__is_hybrid(counter)) {
-		/* Unique hybrid counters necessary. */
-		counter->needs_uniquify = true;
-		return;
-	}
-
-	if  (counter->core.attr.type < PERF_TYPE_MAX && counter->core.attr.type != PERF_TYPE_RAW) {
-		/* Legacy event, don't uniquify. */
-		return;
-	}
-
-	if (counter->pmu && counter->pmu->is_core &&
-	    counter->alternate_hw_config != PERF_COUNT_HW_MAX) {
-		/* A sysfs or json event replacing a legacy event, don't uniquify. */
-		return;
-	}
-
-	if (config->aggr_mode == AGGR_NONE) {
-		/* Always unique with no aggregation. */
-		counter->needs_uniquify = true;
-		return;
-	}
-
-	/*
-	 * Do other non-merged events in the evlist have the same name? If so
-	 * uniquify is necessary.
-	 */
-	evlist__for_each_entry(counter->evlist, evsel) {
-		if (evsel == counter || evsel->merged_stat)
-			continue;
-
-		if (evsel__name_is(counter, evsel__name(evsel))) {
-			counter->needs_uniquify = true;
-			return;
-		}
-	}
-}
-
-static void evlist__set_needs_uniquify(struct evlist *evlist, const struct perf_stat_config *config)
-{
-	struct evsel *counter;
-
-	if (evlist__disable_uniquify(evlist)) {
-		evlist__for_each_entry(evlist, counter)
-			counter->uniquified_name = true;
-		return;
-	}
-
-	evlist__for_each_entry(evlist, counter)
-		evsel__set_needs_uniquify(counter, config);
-}
-
 void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *config,
 			    struct target *_target, struct timespec *ts,
 			    int argc, const char **argv)
@@ -1751,7 +1604,7 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 		.first = true,
 	};
 
-	evlist__set_needs_uniquify(evlist, config);
+	evlist__uniquify_evsel_names(evlist, config);
 
 	if (config->iostat_run)
 		evlist->selected = evlist__first(evlist);
-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v2 2/3] perf parse-events: Use wildcard processing to set an event to merge into

[Chun-Tse Shao]

From: Ian Rogers <irogers@google.com>

The merge stat code fails for uncore events if they are repeated
twice, for example `perf stat -e clockticks,clockticks -I 1000` as the
counts of the second set of uncore events will be merged into the
first counter. Reimplement the logic to have a first_wildcard_match so
that merged later events correctly merge into the first wildcard event
that they will be aggregated into.

Signed-off-by: Ian Rogers <irogers@google.com>
Tested-by: Chun-Tse Shao <ctshao@google.com>
---
 tools/perf/util/evsel.c        | 18 ++++---
 tools/perf/util/evsel.h        |  7 ++-
 tools/perf/util/parse-events.c | 86 ++++++++++++++++++++++++----------
 tools/perf/util/stat-display.c | 11 ++++-
 tools/perf/util/stat.c         | 40 ++--------------
 5 files changed, 90 insertions(+), 72 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index f00cfff119aa..e360071c870d 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -552,11 +552,11 @@ struct evsel *evsel__clone(struct evsel *dest, struct evsel *orig)
 
 	evsel->exclude_GH = orig->exclude_GH;
 	evsel->sample_read = orig->sample_read;
-	evsel->auto_merge_stats = orig->auto_merge_stats;
 	evsel->collect_stat = orig->collect_stat;
 	evsel->weak_group = orig->weak_group;
 	evsel->use_config_name = orig->use_config_name;
 	evsel->pmu = orig->pmu;
+	evsel->first_wildcard_match = orig->first_wildcard_match;
 
 	if (evsel__copy_config_terms(evsel, orig) < 0)
 		goto out_err;
@@ -3949,11 +3949,6 @@ bool evsel__set_needs_uniquify(struct evsel *counter, const struct perf_stat_con
 		return true;
 	}
 
-	if (counter->merged_stat) {
-		/* Counter won't be shown. */
-		return false;
-	}
-
 	if (counter->use_config_name || counter->is_libpfm_event) {
 		/* Original name will be used. */
 		return false;
@@ -3982,12 +3977,21 @@ bool evsel__set_needs_uniquify(struct evsel *counter, const struct perf_stat_con
 		return true;
 	}
 
+	if (counter->first_wildcard_match != NULL) {
+		/*
+		 * If stats are merged then only the first_wildcard_match is
+		 * displayed, there is no need to uniquify this evsel as the
+		 * name won't be shown.
+		 */
+		return false;
+	}
+
 	/*
 	 * Do other non-merged events in the evlist have the same name? If so
 	 * uniquify is necessary.
 	 */
 	evlist__for_each_entry(counter->evlist, evsel) {
-		if (evsel == counter || evsel->merged_stat || evsel->pmu == counter->pmu)
+		if (evsel == counter || evsel->first_wildcard_match || evsel->pmu == counter->pmu)
 			continue;
 
 		if (evsel__name_is(counter, evsel__name(evsel))) {
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 76ccb7c7e8c2..2cc046894e30 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -70,6 +70,11 @@ struct evsel {
 		const char		*unit;
 		struct cgroup		*cgrp;
 		const char		*metric_id;
+		/*
+		 * This point to the first evsel with the same name, intended to store the
+		 * aggregated counts in aggregation mode.
+		 */
+		struct evsel		*first_wildcard_match;
 		/* parse modifier helper */
 		int			exclude_GH;
 		int			sample_read;
@@ -78,7 +83,6 @@ struct evsel {
 		bool			percore;
 		bool			precise_max;
 		bool			is_libpfm_event;
-		bool			auto_merge_stats;
 		bool			collect_stat;
 		bool			weak_group;
 		bool			bpf_counter;
@@ -115,7 +119,6 @@ struct evsel {
 	bool			ignore_missing_thread;
 	bool			forced_leader;
 	bool			cmdline_group_boundary;
-	bool			merged_stat;
 	bool			reset_group;
 	bool			errored;
 	bool			needs_auxtrace_mmap;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 5152fd5a6ead..233836c37e41 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -228,12 +228,32 @@ __add_event(struct list_head *list, int *idx,
 	    struct perf_event_attr *attr,
 	    bool init_attr,
 	    const char *name, const char *metric_id, struct perf_pmu *pmu,
-	    struct list_head *config_terms, bool auto_merge_stats,
+	    struct list_head *config_terms, struct evsel *first_wildcard_match,
 	    struct perf_cpu_map *cpu_list, u64 alternate_hw_config)
 {
 	struct evsel *evsel;
 	struct perf_cpu_map *cpus = perf_cpu_map__is_empty(cpu_list) && pmu ? pmu->cpus : cpu_list;
 
+	/*
+	 * Ensure the first_wildcard_match's PMU matches that of the new event
+	 * being added. Otherwise try to match with another event further down
+	 * the evlist.
+	 */
+	if (first_wildcard_match) {
+		struct evsel *pos = list_prev_entry(first_wildcard_match, core.node);
+
+		first_wildcard_match = NULL;
+		list_for_each_entry_continue(pos, list, core.node) {
+			if (perf_pmu__name_no_suffix_match(pos->pmu, pmu->name)) {
+				first_wildcard_match = pos;
+				break;
+			}
+			if (pos->pmu->is_core && (!pmu || pmu->is_core)) {
+				first_wildcard_match = pos;
+				break;
+			}
+		}
+	}
 	cpus = perf_cpu_map__get(cpus);
 	if (pmu)
 		perf_pmu__warn_invalid_formats(pmu);
@@ -262,9 +282,9 @@ __add_event(struct list_head *list, int *idx,
 	evsel->core.own_cpus = perf_cpu_map__get(cpus);
 	evsel->core.requires_cpu = pmu ? pmu->is_uncore : false;
 	evsel->core.is_pmu_core = pmu ? pmu->is_core : false;
-	evsel->auto_merge_stats = auto_merge_stats;
 	evsel->pmu = pmu;
 	evsel->alternate_hw_config = alternate_hw_config;
+	evsel->first_wildcard_match = first_wildcard_match;
 
 	if (name)
 		evsel->name = strdup(name);
@@ -287,7 +307,7 @@ struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
 {
 	return __add_event(/*list=*/NULL, &idx, attr, /*init_attr=*/false, name,
 			   metric_id, pmu, /*config_terms=*/NULL,
-			   /*auto_merge_stats=*/false, /*cpu_list=*/NULL,
+			   /*first_wildcard_match=*/NULL, /*cpu_list=*/NULL,
 			   /*alternate_hw_config=*/PERF_COUNT_HW_MAX);
 }
 
@@ -298,7 +318,7 @@ static int add_event(struct list_head *list, int *idx,
 {
 	return __add_event(list, idx, attr, /*init_attr*/true, name, metric_id,
 			   /*pmu=*/NULL, config_terms,
-			   /*auto_merge_stats=*/false, /*cpu_list=*/NULL,
+			   /*first_wildcard_match=*/NULL, /*cpu_list=*/NULL,
 			   alternate_hw_config) ? 0 : -ENOMEM;
 }
 
@@ -423,7 +443,7 @@ bool parse_events__filter_pmu(const struct parse_events_state *parse_state,
 static int parse_events_add_pmu(struct parse_events_state *parse_state,
 				struct list_head *list, struct perf_pmu *pmu,
 				const struct parse_events_terms *const_parsed_terms,
-				bool auto_merge_stats, u64 alternate_hw_config);
+				struct evsel *first_wildcard_match, u64 alternate_hw_config);
 
 int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
 			   struct parse_events_state *parse_state,
@@ -433,6 +453,7 @@ int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
 	bool found_supported = false;
 	const char *config_name = get_config_name(parsed_terms);
 	const char *metric_id = get_config_metric_id(parsed_terms);
+	struct evsel *first_wildcard_match = NULL;
 
 	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
 		LIST_HEAD(config_terms);
@@ -449,10 +470,14 @@ int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
 			 */
 			ret = parse_events_add_pmu(parse_state, list, pmu,
 						   parsed_terms,
-						   perf_pmu__auto_merge_stats(pmu),
+						   first_wildcard_match,
 						   /*alternate_hw_config=*/PERF_COUNT_HW_MAX);
 			if (ret)
 				return ret;
+
+			if (first_wildcard_match == NULL)
+				first_wildcard_match =
+					container_of(list->prev, struct evsel, core.node);
 			continue;
 		}
 
@@ -480,11 +505,13 @@ int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
 		}
 
 		if (__add_event(list, idx, &attr, /*init_attr*/true, config_name ?: name,
-				metric_id, pmu, &config_terms, /*auto_merge_stats=*/false,
+				metric_id, pmu, &config_terms, first_wildcard_match,
 				/*cpu_list=*/NULL,
 				/*alternate_hw_config=*/PERF_COUNT_HW_MAX) == NULL)
 			return -ENOMEM;
 
+		if (first_wildcard_match == NULL)
+			first_wildcard_match = container_of(list->prev, struct evsel, core.node);
 		free_config_terms(&config_terms);
 	}
 	return found_supported ? 0 : -EINVAL;
@@ -1335,7 +1362,8 @@ int parse_events_add_tracepoint(struct parse_events_state *parse_state,
 static int __parse_events_add_numeric(struct parse_events_state *parse_state,
 				struct list_head *list,
 				struct perf_pmu *pmu, u32 type, u32 extended_type,
-				u64 config, const struct parse_events_terms *head_config)
+				u64 config, const struct parse_events_terms *head_config,
+				struct evsel *first_wildcard_match)
 {
 	struct perf_event_attr attr;
 	LIST_HEAD(config_terms);
@@ -1362,7 +1390,7 @@ static int __parse_events_add_numeric(struct parse_events_state *parse_state,
 	name = get_config_name(head_config);
 	metric_id = get_config_metric_id(head_config);
 	ret = __add_event(list, &parse_state->idx, &attr, /*init_attr*/true, name,
-			  metric_id, pmu, &config_terms, /*auto_merge_stats=*/false,
+			  metric_id, pmu, &config_terms, first_wildcard_match,
 			  /*cpu_list=*/NULL, /*alternate_hw_config=*/PERF_COUNT_HW_MAX
 		) == NULL ? -ENOMEM : 0;
 	free_config_terms(&config_terms);
@@ -1380,6 +1408,8 @@ int parse_events_add_numeric(struct parse_events_state *parse_state,
 
 	/* Wildcards on numeric values are only supported by core PMUs. */
 	if (wildcard && perf_pmus__supports_extended_type()) {
+		struct evsel *first_wildcard_match = NULL;
+
 		while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
 			int ret;
 
@@ -1389,15 +1419,21 @@ int parse_events_add_numeric(struct parse_events_state *parse_state,
 
 			ret = __parse_events_add_numeric(parse_state, list, pmu,
 							 type, pmu->type,
-							 config, head_config);
+							 config, head_config,
+							 first_wildcard_match);
 			if (ret)
 				return ret;
+
+			if (first_wildcard_match == NULL)
+				first_wildcard_match =
+					container_of(list->prev, struct evsel, core.node);
 		}
 		if (found_supported)
 			return 0;
 	}
 	return __parse_events_add_numeric(parse_state, list, perf_pmus__find_by_type(type),
-					type, /*extended_type=*/0, config, head_config);
+					type, /*extended_type=*/0, config, head_config,
+					/*first_wildcard_match=*/NULL);
 }
 
 static bool config_term_percore(struct list_head *config_terms)
@@ -1415,7 +1451,7 @@ static bool config_term_percore(struct list_head *config_terms)
 static int parse_events_add_pmu(struct parse_events_state *parse_state,
 				struct list_head *list, struct perf_pmu *pmu,
 				const struct parse_events_terms *const_parsed_terms,
-				bool auto_merge_stats, u64 alternate_hw_config)
+				struct evsel *first_wildcard_match, u64 alternate_hw_config)
 {
 	struct perf_event_attr attr;
 	struct perf_pmu_info info;
@@ -1451,7 +1487,7 @@ static int parse_events_add_pmu(struct parse_events_state *parse_state,
 		evsel = __add_event(list, &parse_state->idx, &attr,
 				    /*init_attr=*/true, /*name=*/NULL,
 				    /*metric_id=*/NULL, pmu,
-				    /*config_terms=*/NULL, auto_merge_stats,
+				    /*config_terms=*/NULL, first_wildcard_match,
 				    /*cpu_list=*/NULL, alternate_hw_config);
 		return evsel ? 0 : -ENOMEM;
 	}
@@ -1521,7 +1557,7 @@ static int parse_events_add_pmu(struct parse_events_state *parse_state,
 	evsel = __add_event(list, &parse_state->idx, &attr, /*init_attr=*/true,
 			    get_config_name(&parsed_terms),
 			    get_config_metric_id(&parsed_terms), pmu,
-			    &config_terms, auto_merge_stats, /*cpu_list=*/NULL,
+			    &config_terms, first_wildcard_match, /*cpu_list=*/NULL,
 			    alternate_hw_config);
 	if (!evsel) {
 		parse_events_terms__exit(&parsed_terms);
@@ -1554,6 +1590,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 	int ok = 0;
 	const char *config;
 	struct parse_events_terms parsed_terms;
+	struct evsel *first_wildcard_match = NULL;
 
 	*listp = NULL;
 
@@ -1586,17 +1623,14 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 	INIT_LIST_HEAD(list);
 
 	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
-		bool auto_merge_stats;
-
 		if (parse_events__filter_pmu(parse_state, pmu))
 			continue;
 
 		if (!perf_pmu__have_event(pmu, event_name))
 			continue;
 
-		auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
 		if (!parse_events_add_pmu(parse_state, list, pmu,
-					  &parsed_terms, auto_merge_stats, hw_config)) {
+					  &parsed_terms, first_wildcard_match, hw_config)) {
 			struct strbuf sb;
 
 			strbuf_init(&sb, /*hint=*/ 0);
@@ -1605,11 +1639,13 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			strbuf_release(&sb);
 			ok++;
 		}
+		if (first_wildcard_match == NULL)
+			first_wildcard_match = container_of(list->prev, struct evsel, core.node);
 	}
 
 	if (parse_state->fake_pmu) {
 		if (!parse_events_add_pmu(parse_state, list, perf_pmus__fake_pmu(), &parsed_terms,
-					  /*auto_merge_stats=*/true, hw_config)) {
+					 first_wildcard_match, hw_config)) {
 			struct strbuf sb;
 
 			strbuf_init(&sb, /*hint=*/ 0);
@@ -1640,6 +1676,7 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 	struct perf_pmu *pmu;
 	int ok = 0;
 	char *help;
+	struct evsel *first_wildcard_match = NULL;
 
 	*listp = malloc(sizeof(**listp));
 	if (!*listp)
@@ -1650,14 +1687,14 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 	/* Attempt to add to list assuming event_or_pmu is a PMU name. */
 	pmu = perf_pmus__find(event_or_pmu);
 	if (pmu && !parse_events_add_pmu(parse_state, *listp, pmu, const_parsed_terms,
-					 /*auto_merge_stats=*/false,
+					 first_wildcard_match,
 					 /*alternate_hw_config=*/PERF_COUNT_HW_MAX))
 		return 0;
 
 	if (parse_state->fake_pmu) {
 		if (!parse_events_add_pmu(parse_state, *listp, perf_pmus__fake_pmu(),
 					  const_parsed_terms,
-					  /*auto_merge_stats=*/false,
+					  first_wildcard_match,
 					  /*alternate_hw_config=*/PERF_COUNT_HW_MAX))
 			return 0;
 	}
@@ -1667,15 +1704,16 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
 		if (!parse_events__filter_pmu(parse_state, pmu) &&
 		    perf_pmu__wildcard_match(pmu, event_or_pmu)) {
-			bool auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
-
 			if (!parse_events_add_pmu(parse_state, *listp, pmu,
 						  const_parsed_terms,
-						  auto_merge_stats,
+						  first_wildcard_match,
 						  /*alternate_hw_config=*/PERF_COUNT_HW_MAX)) {
 				ok++;
 				parse_state->wild_card_pmus = true;
 			}
+			if (first_wildcard_match == NULL)
+				first_wildcard_match =
+					container_of((*listp)->prev, struct evsel, core.node);
 		}
 	}
 	if (ok)
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index d427e0ca98a1..91386429115d 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -1011,8 +1011,15 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 	os->evsel = counter;
 
 	/* Skip already merged uncore/hybrid events */
-	if (counter->merged_stat)
-		return;
+	if (config->aggr_mode != AGGR_NONE) {
+		if (evsel__is_hybrid(counter)) {
+			if (config->hybrid_merge && counter->first_wildcard_match != NULL)
+				return;
+		} else {
+			if (counter->first_wildcard_match != NULL)
+				return;
+		}
+	}
 
 	val = aggr->counts.val;
 	ena = aggr->counts.ena;
diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
index 1f7abd8754c7..355a7d5c8ab8 100644
--- a/tools/perf/util/stat.c
+++ b/tools/perf/util/stat.c
@@ -535,35 +535,6 @@ static int evsel__merge_aggr_counters(struct evsel *evsel, struct evsel *alias)
 
 	return 0;
 }
-/*
- * Events should have the same name, scale, unit, cgroup but on different core
- * PMUs or on different but matching uncore PMUs.
- */
-static bool evsel__is_alias(struct evsel *evsel_a, struct evsel *evsel_b)
-{
-	if (strcmp(evsel__name(evsel_a), evsel__name(evsel_b)))
-		return false;
-
-	if (evsel_a->scale != evsel_b->scale)
-		return false;
-
-	if (evsel_a->cgrp != evsel_b->cgrp)
-		return false;
-
-	if (strcmp(evsel_a->unit, evsel_b->unit))
-		return false;
-
-	if (evsel__is_clock(evsel_a) != evsel__is_clock(evsel_b))
-		return false;
-
-	if (evsel_a->pmu == evsel_b->pmu || evsel_a->pmu == NULL || evsel_b->pmu == NULL)
-		return false;
-
-	if (evsel_a->pmu->is_core)
-		return evsel_b->pmu->is_core;
-
-	return perf_pmu__name_no_suffix_match(evsel_a->pmu, evsel_b->pmu->name);
-}
 
 static void evsel__merge_aliases(struct evsel *evsel)
 {
@@ -572,10 +543,9 @@ static void evsel__merge_aliases(struct evsel *evsel)
 
 	alias = list_prepare_entry(evsel, &(evlist->core.entries), core.node);
 	list_for_each_entry_continue(alias, &evlist->core.entries, core.node) {
-		/* Merge the same events on different PMUs. */
-		if (evsel__is_alias(evsel, alias)) {
+		if (alias->first_wildcard_match == evsel) {
+			/* Merge the same events on different PMUs. */
 			evsel__merge_aggr_counters(evsel, alias);
-			alias->merged_stat = true;
 		}
 	}
 }
@@ -588,11 +558,7 @@ static bool evsel__should_merge_hybrid(const struct evsel *evsel,
 
 static void evsel__merge_stats(struct evsel *evsel, struct perf_stat_config *config)
 {
-	/* this evsel is already merged */
-	if (evsel->merged_stat)
-		return;
-
-	if (evsel->auto_merge_stats || evsel__should_merge_hybrid(evsel, config))
+	if (!evsel->pmu || !evsel->pmu->is_core || evsel__should_merge_hybrid(evsel, config))
 		evsel__merge_aliases(evsel);
 }
 
-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v2 3/3] perf test: Add stat uniquifying test

[Chun-Tse Shao]

The `stat+uniquify.sh` test retrieves all uniquified `clockticks` events
from `perf list -v clockticks` and check if `perf stat -e clockticks -A`
contains all of them.

Signed-off-by: Chun-Tse Shao <ctshao@google.com>
---
 .../tests/shell/stat+event_uniquifying.sh     | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100755 tools/perf/tests/shell/stat+event_uniquifying.sh

diff --git a/tools/perf/tests/shell/stat+event_uniquifying.sh b/tools/perf/tests/shell/stat+event_uniquifying.sh
new file mode 100755
index 000000000000..5ec35c52b7d9
--- /dev/null
+++ b/tools/perf/tests/shell/stat+event_uniquifying.sh
@@ -0,0 +1,69 @@
+#!/bin/bash
+# perf stat events uniquifying
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+
+stat_output=$(mktemp /tmp/__perf_test.stat_output.XXXXX)
+perf_tool=perf
+err=0
+
+test_event_uniquifying() {
+  # We use `clockticks` to verify the uniquify behavior.
+  event="clockticks"
+
+  # If the `-A` option is added, the event should be uniquified.
+  #
+  # $perf list -v clockticks
+  #
+  # List of pre-defined events (to be used in -e or -M):
+  #
+  #   uncore_imc_0/clockticks/                           [Kernel PMU event]
+  #   uncore_imc_1/clockticks/                           [Kernel PMU event]
+  #   uncore_imc_2/clockticks/                           [Kernel PMU event]
+  #   uncore_imc_3/clockticks/                           [Kernel PMU event]
+  #   uncore_imc_4/clockticks/                           [Kernel PMU event]
+  #   uncore_imc_5/clockticks/                           [Kernel PMU event]
+  #
+  #   ...
+  #
+  # $perf stat -e clockticks -A -- true
+  #
+  #  Performance counter stats for 'system wide':
+  #
+  # CPU0            3,773,018      uncore_imc_0/clockticks/
+  # CPU0            3,609,025      uncore_imc_1/clockticks/
+  # CPU0                    0      uncore_imc_2/clockticks/
+  # CPU0            3,230,009      uncore_imc_3/clockticks/
+  # CPU0            3,049,897      uncore_imc_4/clockticks/
+  # CPU0                    0      uncore_imc_5/clockticks/
+  #
+  #        0.002029828 seconds time elapsed
+
+  echo "stat event uniquifying test"
+  uniquified_event_array=()
+
+  # Check how many uniquified events.
+  while IFS= read -r line; do
+    uniquified_event=$(echo "$line" | awk '{print $1}')
+    uniquified_event_array+=("${uniquified_event}")
+  done < <(${perf_tool} list -v ${event} | grep "\[Kernel PMU event\]")
+
+  perf_command="${perf_tool} stat -e $event -A -o ${stat_output} -- true"
+  $perf_command
+
+  # Check the output contains all uniquified events.
+  for uniquified_event in "${uniquified_event_array[@]}"; do
+    if ! cat "${stat_output}" | grep -q "${uniquified_event}"; then
+      echo "Event is not uniquified [Failed]"
+      echo "${perf_command}"
+      cat "${stat_output}"
+      err=1
+      break
+    fi
+  done
+}
+
+test_event_uniquifying
+rm -f "${stat_output}"
+exit $err
-- 
2.49.0.472.ge94155a9ec-goog

Re: [PATCH v2 0/3] Fix incorrect counts when count the same uncore event multiple times

[Ian Rogers]

On Thu, Mar 27, 2025 at 3:56 PM Chun-Tse Shao <ctshao@google.com> wrote:
>
> Let's take a look an example, the machine is SKX with 6 IMC devices.
>
>   perf stat -e clockticks,clockticks -I 1000
>   #           time             counts unit events
>        1.001127430      6,901,503,174      uncore_imc_0/clockticks/
>        1.001127430      3,940,896,301      uncore_imc_0/clockticks/
>        2.002649722        988,376,876      uncore_imc_0/clockticks/
>        2.002649722        988,376,141      uncore_imc_0/clockticks/
>        3.004071319      1,000,292,675      uncore_imc_0/clockticks/
>        3.004071319      1,000,294,160      uncore_imc_0/clockticks/
>
> 1) The events name should not be uniquified.
> 2) The initial count for the first `clockticks` is doubled.
> 3) Subsequent count only report for the first IMC device.
>
> The first patch fixes 1) and 3), and the second patch fixes 2).
>
> After these fix:
>
>   perf stat -e clockticks,clockticks -I 1000
>   #           time             counts unit events
>        1.001127586      4,126,938,857      clockticks
>        1.001127586      4,121,564,277      clockticks
>        2.001686014      3,953,806,350      clockticks
>        2.001686014      3,953,809,541      clockticks
>        3.003121403      4,137,750,252      clockticks
>        3.003121403      4,137,749,048      clockticks
>
> I also tested `-A`, `--per-socket`, `--per-die` and `--per-core`, all
> looks good.
>
> Ian tested `hybrid-merge` and `hwmon`, all looks good as well.
>
> Chun-Tse Shao (1):
>   perf test: Add stat uniquifying test
>
> Ian Rogers (2):
>   perf evlist: Make uniquifying counter names consistent
>   perf parse-events: Use wildcard processing to set an event to merge
>     into

Tested-by: Ian Rogers <irogers@google.com>

There could be minor conflict with this unreviewed series:
https://lore.kernel.org/lkml/20250318041442.321230-1-irogers@google.com/

Thanks,
Ian

>  tools/perf/builtin-record.c                   |   7 +-
>  tools/perf/builtin-top.c                      |   7 +-
>  .../tests/shell/stat+event_uniquifying.sh     |  69 ++++++++
>  tools/perf/util/evlist.c                      |  66 +++++---
>  tools/perf/util/evlist.h                      |   3 +-
>  tools/perf/util/evsel.c                       | 119 ++++++++++++-
>  tools/perf/util/evsel.h                       |  11 +-
>  tools/perf/util/parse-events.c                |  86 +++++++---
>  tools/perf/util/stat-display.c                | 160 ++----------------
>  tools/perf/util/stat.c                        |  40 +----
>  10 files changed, 329 insertions(+), 239 deletions(-)
>  create mode 100755 tools/perf/tests/shell/stat+event_uniquifying.sh
>
> --
> v2:
>   - Fixes for `hwmon` and `--hybrid-merge`.
>   - Add a test for event uniquifying.
>
> v1: lore.kernel.org/20250326234758.480431-1-ctshao@google.com
> 2.49.0.472.ge94155a9ec-goog
>

Re: [PATCH v2 3/3] perf test: Add stat uniquifying test

[Ian Rogers]

On Thu, Mar 27, 2025 at 4:01 PM Chun-Tse Shao <ctshao@google.com> wrote:
>
> The `stat+uniquify.sh` test retrieves all uniquified `clockticks` events
> from `perf list -v clockticks` and check if `perf stat -e clockticks -A`
> contains all of them.
>
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> ---
>  .../tests/shell/stat+event_uniquifying.sh     | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100755 tools/perf/tests/shell/stat+event_uniquifying.sh
>
> diff --git a/tools/perf/tests/shell/stat+event_uniquifying.sh b/tools/perf/tests/shell/stat+event_uniquifying.sh
> new file mode 100755
> index 000000000000..5ec35c52b7d9
> --- /dev/null
> +++ b/tools/perf/tests/shell/stat+event_uniquifying.sh
> @@ -0,0 +1,69 @@
> +#!/bin/bash
> +# perf stat events uniquifying
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +
> +stat_output=$(mktemp /tmp/__perf_test.stat_output.XXXXX)
> +perf_tool=perf
> +err=0
> +
> +test_event_uniquifying() {
> +  # We use `clockticks` to verify the uniquify behavior.
> +  event="clockticks"

This event is generally only available on Intel, not AMD or ARM, so we
will need to skip if it isn't present.

> +  # If the `-A` option is added, the event should be uniquified.
> +  #
> +  # $perf list -v clockticks
> +  #
> +  # List of pre-defined events (to be used in -e or -M):
> +  #
> +  #   uncore_imc_0/clockticks/                           [Kernel PMU event]
> +  #   uncore_imc_1/clockticks/                           [Kernel PMU event]
> +  #   uncore_imc_2/clockticks/                           [Kernel PMU event]
> +  #   uncore_imc_3/clockticks/                           [Kernel PMU event]
> +  #   uncore_imc_4/clockticks/                           [Kernel PMU event]
> +  #   uncore_imc_5/clockticks/                           [Kernel PMU event]
> +  #
> +  #   ...
> +  #
> +  # $perf stat -e clockticks -A -- true
> +  #
> +  #  Performance counter stats for 'system wide':
> +  #
> +  # CPU0            3,773,018      uncore_imc_0/clockticks/
> +  # CPU0            3,609,025      uncore_imc_1/clockticks/
> +  # CPU0                    0      uncore_imc_2/clockticks/
> +  # CPU0            3,230,009      uncore_imc_3/clockticks/
> +  # CPU0            3,049,897      uncore_imc_4/clockticks/
> +  # CPU0                    0      uncore_imc_5/clockticks/
> +  #
> +  #        0.002029828 seconds time elapsed
> +
> +  echo "stat event uniquifying test"
> +  uniquified_event_array=()
> +
> +  # Check how many uniquified events.
> +  while IFS= read -r line; do
> +    uniquified_event=$(echo "$line" | awk '{print $1}')
> +    uniquified_event_array+=("${uniquified_event}")
> +  done < <(${perf_tool} list -v ${event} | grep "\[Kernel PMU event\]")

Shouldn't the array contain every sysfs event that doesn't have a json
component? They may or may not be uniquified so I think the array name
is misleading.

> +  perf_command="${perf_tool} stat -e $event -A -o ${stat_output} -- true"
> +  $perf_command
> +
> +  # Check the output contains all uniquified events.
> +  for uniquified_event in "${uniquified_event_array[@]}"; do
> +    if ! cat "${stat_output}" | grep -q "${uniquified_event}"; then

Why not pass the file directly to grep?
Should some of the events not show in the stat output as you only
asked for the clockticks event? I'm not sure how this test can pass
currently.

Thanks,
Ian

> +      echo "Event is not uniquified [Failed]"
> +      echo "${perf_command}"
> +      cat "${stat_output}"
> +      err=1
> +      break
> +    fi
> +  done
> +}
> +
> +test_event_uniquifying
> +rm -f "${stat_output}"
> +exit $err
> --
> 2.49.0.472.ge94155a9ec-goog
>

Re: [PATCH v2 3/3] perf test: Add stat uniquifying test

[Chun-Tse Shao]

On Thu, Apr 3, 2025 at 12:25 PM Ian Rogers <irogers@google.com> wrote:
>
> On Thu, Mar 27, 2025 at 4:01 PM Chun-Tse Shao <ctshao@google.com> wrote:
> >
> > The `stat+uniquify.sh` test retrieves all uniquified `clockticks` events
> > from `perf list -v clockticks` and check if `perf stat -e clockticks -A`
> > contains all of them.
> >
> > Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> > ---
> >  .../tests/shell/stat+event_uniquifying.sh     | 69 +++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >  create mode 100755 tools/perf/tests/shell/stat+event_uniquifying.sh
> >
> > diff --git a/tools/perf/tests/shell/stat+event_uniquifying.sh b/tools/perf/tests/shell/stat+event_uniquifying.sh
> > new file mode 100755
> > index 000000000000..5ec35c52b7d9
> > --- /dev/null
> > +++ b/tools/perf/tests/shell/stat+event_uniquifying.sh
> > @@ -0,0 +1,69 @@
> > +#!/bin/bash
> > +# perf stat events uniquifying
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +set -e
> > +
> > +stat_output=$(mktemp /tmp/__perf_test.stat_output.XXXXX)
> > +perf_tool=perf
> > +err=0
> > +
> > +test_event_uniquifying() {
> > +  # We use `clockticks` to verify the uniquify behavior.
> > +  event="clockticks"
>
> This event is generally only available on Intel, not AMD or ARM, so we
> will need to skip if it isn't present.
>
> > +  # If the `-A` option is added, the event should be uniquified.
> > +  #
> > +  # $perf list -v clockticks
> > +  #
> > +  # List of pre-defined events (to be used in -e or -M):
> > +  #
> > +  #   uncore_imc_0/clockticks/                           [Kernel PMU event]
> > +  #   uncore_imc_1/clockticks/                           [Kernel PMU event]
> > +  #   uncore_imc_2/clockticks/                           [Kernel PMU event]
> > +  #   uncore_imc_3/clockticks/                           [Kernel PMU event]
> > +  #   uncore_imc_4/clockticks/                           [Kernel PMU event]
> > +  #   uncore_imc_5/clockticks/                           [Kernel PMU event]
> > +  #
> > +  #   ...
> > +  #
> > +  # $perf stat -e clockticks -A -- true
> > +  #
> > +  #  Performance counter stats for 'system wide':
> > +  #
> > +  # CPU0            3,773,018      uncore_imc_0/clockticks/
> > +  # CPU0            3,609,025      uncore_imc_1/clockticks/
> > +  # CPU0                    0      uncore_imc_2/clockticks/
> > +  # CPU0            3,230,009      uncore_imc_3/clockticks/
> > +  # CPU0            3,049,897      uncore_imc_4/clockticks/
> > +  # CPU0                    0      uncore_imc_5/clockticks/
> > +  #
> > +  #        0.002029828 seconds time elapsed
> > +
> > +  echo "stat event uniquifying test"
> > +  uniquified_event_array=()
> > +
> > +  # Check how many uniquified events.
> > +  while IFS= read -r line; do
> > +    uniquified_event=$(echo "$line" | awk '{print $1}')
> > +    uniquified_event_array+=("${uniquified_event}")
> > +  done < <(${perf_tool} list -v ${event} | grep "\[Kernel PMU event\]")
>
> Shouldn't the array contain every sysfs event that doesn't have a json
> component? They may or may not be uniquified so I think the array name
> is misleading.
>
> > +  perf_command="${perf_tool} stat -e $event -A -o ${stat_output} -- true"
> > +  $perf_command
> > +
> > +  # Check the output contains all uniquified events.
> > +  for uniquified_event in "${uniquified_event_array[@]}"; do
> > +    if ! cat "${stat_output}" | grep -q "${uniquified_event}"; then
>
> Why not pass the file directly to grep?
> Should some of the events not show in the stat output as you only
> asked for the clockticks event? I'm not sure how this test can pass
> currently.
>
> Thanks,
> Ian
>
Thanks for your comment, Ian.

I tried to run `perf list -v clockticks` and grep for all events which
have `[Kernel PMU event]` tag.
Ideally, that should retrieve all uniquified clockticks events.
Then I ran `perf stat -e clockticks` to check if all events from `perf
list` are showing.

I randomly picked some machines from different arches and it seems
work. Let me know if you have a better idea.

-CT

> > +      echo "Event is not uniquified [Failed]"
> > +      echo "${perf_command}"
> > +      cat "${stat_output}"
> > +      err=1
> > +      break
> > +    fi
> > +  done
> > +}
> > +
> > +test_event_uniquifying
> > +rm -f "${stat_output}"
> > +exit $err
> > --
> > 2.49.0.472.ge94155a9ec-goog
> >

Re: [PATCH v2 0/3] Fix incorrect counts when count the same uncore event multiple times

[Chun-Tse Shao]

Ping.

Thanks,
CT

On Thu, Mar 27, 2025 at 9:20 PM Ian Rogers <irogers@google.com> wrote:
>
> On Thu, Mar 27, 2025 at 3:56 PM Chun-Tse Shao <ctshao@google.com> wrote:
> >
> > Let's take a look an example, the machine is SKX with 6 IMC devices.
> >
> >   perf stat -e clockticks,clockticks -I 1000
> >   #           time             counts unit events
> >        1.001127430      6,901,503,174      uncore_imc_0/clockticks/
> >        1.001127430      3,940,896,301      uncore_imc_0/clockticks/
> >        2.002649722        988,376,876      uncore_imc_0/clockticks/
> >        2.002649722        988,376,141      uncore_imc_0/clockticks/
> >        3.004071319      1,000,292,675      uncore_imc_0/clockticks/
> >        3.004071319      1,000,294,160      uncore_imc_0/clockticks/
> >
> > 1) The events name should not be uniquified.
> > 2) The initial count for the first `clockticks` is doubled.
> > 3) Subsequent count only report for the first IMC device.
> >
> > The first patch fixes 1) and 3), and the second patch fixes 2).
> >
> > After these fix:
> >
> >   perf stat -e clockticks,clockticks -I 1000
> >   #           time             counts unit events
> >        1.001127586      4,126,938,857      clockticks
> >        1.001127586      4,121,564,277      clockticks
> >        2.001686014      3,953,806,350      clockticks
> >        2.001686014      3,953,809,541      clockticks
> >        3.003121403      4,137,750,252      clockticks
> >        3.003121403      4,137,749,048      clockticks
> >
> > I also tested `-A`, `--per-socket`, `--per-die` and `--per-core`, all
> > looks good.
> >
> > Ian tested `hybrid-merge` and `hwmon`, all looks good as well.
> >
> > Chun-Tse Shao (1):
> >   perf test: Add stat uniquifying test
> >
> > Ian Rogers (2):
> >   perf evlist: Make uniquifying counter names consistent
> >   perf parse-events: Use wildcard processing to set an event to merge
> >     into
>
> Tested-by: Ian Rogers <irogers@google.com>
>
> There could be minor conflict with this unreviewed series:
> https://lore.kernel.org/lkml/20250318041442.321230-1-irogers@google.com/
>
> Thanks,
> Ian
>
> >  tools/perf/builtin-record.c                   |   7 +-
> >  tools/perf/builtin-top.c                      |   7 +-
> >  .../tests/shell/stat+event_uniquifying.sh     |  69 ++++++++
> >  tools/perf/util/evlist.c                      |  66 +++++---
> >  tools/perf/util/evlist.h                      |   3 +-
> >  tools/perf/util/evsel.c                       | 119 ++++++++++++-
> >  tools/perf/util/evsel.h                       |  11 +-
> >  tools/perf/util/parse-events.c                |  86 +++++++---
> >  tools/perf/util/stat-display.c                | 160 ++----------------
> >  tools/perf/util/stat.c                        |  40 +----
> >  10 files changed, 329 insertions(+), 239 deletions(-)
> >  create mode 100755 tools/perf/tests/shell/stat+event_uniquifying.sh
> >
> > --
> > v2:
> >   - Fixes for `hwmon` and `--hybrid-merge`.
> >   - Add a test for event uniquifying.
> >
> > v1: lore.kernel.org/20250326234758.480431-1-ctshao@google.com
> > 2.49.0.472.ge94155a9ec-goog
> >

Re: [PATCH v2 1/3] perf evlist: Make uniquifying counter names consistent

[Arnaldo Carvalho de Melo]

On Thu, Mar 27, 2025 at 03:48:16PM -0700, Chun-Tse Shao wrote:
> From: Ian Rogers <irogers@google.com>
> 
> perf stat has different uniquification logic to perf record and perf
> top. In the case of perf record and perf top all hybrid event
> names are uniquified. Perf stat is more disciplined respecting
> name config terms, libpfm4 events, etc. Perf stat will uniquify
> hybrid events and the non-core PMU cases shouldn't apply to perf
> record or perf top. For consistency, remove the uniquification for
> perf record and perf top and reuse the perf stat uniquification,
> making the code more globally visible for this.
> 
> Fix the detection of cross-PMU for disabling uniquify by correctly
> setting last_pmu. When setting uniquify on an evsel, make sure the
> PMUs between the 2 considered events differ otherwise the uniquify
> isn't adding value.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> Tested-by: Chun-Tse Shao <ctshao@google.com>

Can you please refresh this series?

Total patches: 3
---
Cover: ./v2_20250327_ctshao_fix_incorrect_counts_when_count_the_same_uncore_event_multiple_times.cover
 Link: https://lore.kernel.org/r/20250327225651.642965-1-ctshao@google.com
 Base: not specified
       git am ./v2_20250327_ctshao_fix_incorrect_counts_when_count_the_same_uncore_event_multiple_times.mbx
⬢ [acme@toolbx perf-tools-next]$        git am ./v2_20250327_ctshao_fix_incorrect_counts_when_count_the_same_uncore_event_multiple_times.mbx
Applying: perf evlist: Make uniquifying counter names consistent
error: patch failed: tools/perf/util/evlist.h:19
error: tools/perf/util/evlist.h: patch does not apply
Patch failed at 0001 perf evlist: Make uniquifying counter names consistent
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
⬢ [acme@toolbx perf-tools-next]$ git am --abort
⬢ [acme@toolbx perf-tools-next]$ patch -p1 < ./v2_20250327_ctshao_fix_incorrect_counts_when_count_the_same_uncore_event_multiple_times.mbx
patching file tools/perf/builtin-record.c
Hunk #2 succeeded at 2485 (offset 1 line).
patching file tools/perf/builtin-top.c
patching file tools/perf/util/evlist.c
Hunk #1 succeeded at 2565 (offset 13 lines).
patching file tools/perf/util/evlist.h
Hunk #1 succeeded at 19 with fuzz 2.
Hunk #2 succeeded at 435 with fuzz 1 (offset 1 line).
patching file tools/perf/util/evsel.c
Hunk #1 succeeded at 3954 (offset 15 lines).
patching file tools/perf/util/evsel.h
Hunk #2 succeeded at 549 (offset 6 lines).
patching file tools/perf/util/stat-display.c
Hunk #1 succeeded at 915 (offset -14 lines).
Hunk #2 succeeded at 1005 (offset -9 lines).
Hunk #3 succeeded at 1579 (offset -14 lines).
Hunk #4 succeeded at 1590 (offset -14 lines).
patching file tools/perf/util/evsel.c
Hunk #2 succeeded at 3964 (offset 15 lines).
Hunk #3 succeeded at 3992 (offset 15 lines).
patching file tools/perf/util/evsel.h
patching file tools/perf/util/parse-events.c
Hunk #1 FAILED at 228.
Hunk #2 FAILED at 262.
Hunk #3 succeeded at 318 (offset 31 lines).
Hunk #4 succeeded at 329 (offset 31 lines).
Hunk #5 succeeded at 454 (offset 31 lines).
Hunk #6 FAILED at 433.
Hunk #7 FAILED at 449.
Hunk #8 FAILED at 480.
Hunk #9 succeeded at 1387 (offset 52 lines).
Hunk #10 FAILED at 1363.
Hunk #11 succeeded at 1435 (offset 54 lines).
Hunk #12 succeeded at 1446 (offset 54 lines).
Hunk #13 succeeded at 1478 (offset 54 lines).
Hunk #14 succeeded at 1515 (offset 55 lines).
Hunk #15 FAILED at 1530.
Hunk #16 succeeded at 1623 (offset 60 lines).
Hunk #17 succeeded at 1656 (offset 60 lines).
Hunk #18 succeeded at 1672 (offset 60 lines).
Hunk #19 succeeded at 1709 (offset 60 lines).
Hunk #20 succeeded at 1720 (offset 60 lines).
Hunk #21 succeeded at 1737 (offset 60 lines).
7 out of 21 hunks FAILED -- saving rejects to file tools/perf/util/parse-events.c.rej
patching file tools/perf/util/stat-display.c
Hunk #1 succeeded at 1002 (offset -9 lines).
patching file tools/perf/util/stat.c
patching file tools/perf/tests/shell/stat+event_uniquifying.sh
⬢ [acme@toolbx perf-tools-next]$ 

Re: [PATCH v2 1/3] perf evlist: Make uniquifying counter names consistent

[Arnaldo Carvalho de Melo]

On Mon, May 12, 2025 at 02:51:37PM -0300, Arnaldo Carvalho de Melo wrote:
> Can you please refresh this series?

I mean to what is in:

https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git

branch tmp.perf-tools-next

- Arnaldo