[PATCH v2 1/2] perf topdown: Use attribute to see an event is a topdown metic or slots

[PATCH v2 1/2] perf topdown: Use attribute to see an event is a topdown metic or slots

[Ian Rogers]

The string comparisons were overly broad and could fire for the
incorrect PMU and events. Switch to using the config in the attribute
then add a perf test to confirm the attribute config values match
those of parsed events of that name and don't match others. This
exposed matches for slots events that shouldn't have matched as the
slots fixed counter event, such as topdown.slots_p.

Fixes: fbc798316bef ("perf x86/topdown: Refine helper arch_is_topdown_metrics()")
Signed-off-by: Ian Rogers <irogers@google.com>
---
v2: In test rename topdown_pmu to p_core_pmu for clarity.
---
 tools/perf/arch/x86/include/arch-tests.h |  4 ++
 tools/perf/arch/x86/tests/Build          |  1 +
 tools/perf/arch/x86/tests/arch-tests.c   |  1 +
 tools/perf/arch/x86/tests/topdown.c      | 76 ++++++++++++++++++++++++
 tools/perf/arch/x86/util/evsel.c         | 46 ++++----------
 tools/perf/arch/x86/util/topdown.c       | 31 ++++------
 tools/perf/arch/x86/util/topdown.h       |  4 ++
 7 files changed, 108 insertions(+), 55 deletions(-)
 create mode 100644 tools/perf/arch/x86/tests/topdown.c

diff --git a/tools/perf/arch/x86/include/arch-tests.h b/tools/perf/arch/x86/include/arch-tests.h
index 4fd425157d7d..8713e9122d4c 100644
--- a/tools/perf/arch/x86/include/arch-tests.h
+++ b/tools/perf/arch/x86/include/arch-tests.h
@@ -2,6 +2,8 @@
 #ifndef ARCH_TESTS_H
 #define ARCH_TESTS_H
 
+#include "tests/tests.h"
+
 struct test_suite;
 
 /* Tests */
@@ -17,6 +19,8 @@ int test__amd_ibs_via_core_pmu(struct test_suite *test, int subtest);
 int test__amd_ibs_period(struct test_suite *test, int subtest);
 int test__hybrid(struct test_suite *test, int subtest);
 
+DECLARE_SUITE(x86_topdown);
+
 extern struct test_suite *arch_tests[];
 
 #endif
diff --git a/tools/perf/arch/x86/tests/Build b/tools/perf/arch/x86/tests/Build
index 01d5527f38c7..311b6b53d3d8 100644
--- a/tools/perf/arch/x86/tests/Build
+++ b/tools/perf/arch/x86/tests/Build
@@ -11,6 +11,7 @@ endif
 perf-test-$(CONFIG_X86_64) += bp-modify.o
 perf-test-y += amd-ibs-via-core-pmu.o
 perf-test-y += amd-ibs-period.o
+perf-test-y += topdown.o
 
 ifdef SHELLCHECK
   SHELL_TESTS := gen-insn-x86-dat.sh
diff --git a/tools/perf/arch/x86/tests/arch-tests.c b/tools/perf/arch/x86/tests/arch-tests.c
index bfee2432515b..29ec1861ccef 100644
--- a/tools/perf/arch/x86/tests/arch-tests.c
+++ b/tools/perf/arch/x86/tests/arch-tests.c
@@ -53,5 +53,6 @@ struct test_suite *arch_tests[] = {
 	&suite__amd_ibs_via_core_pmu,
 	&suite__amd_ibs_period,
 	&suite__hybrid,
+	&suite__x86_topdown,
 	NULL,
 };
diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
new file mode 100644
index 000000000000..8d0ea7a4bbc1
--- /dev/null
+++ b/tools/perf/arch/x86/tests/topdown.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "arch-tests.h"
+#include "../util/topdown.h"
+#include "evlist.h"
+#include "parse-events.h"
+#include "pmu.h"
+#include "pmus.h"
+
+static int event_cb(void *state, struct pmu_event_info *info)
+{
+	char buf[256];
+	struct parse_events_error parse_err;
+	int *ret = state, err;
+	struct evlist *evlist = evlist__new();
+	struct evsel *evsel;
+
+	if (!evlist)
+		return -ENOMEM;
+
+	parse_events_error__init(&parse_err);
+	snprintf(buf, sizeof(buf), "%s/%s/", info->pmu->name, info->name);
+	err = parse_events(evlist, buf, &parse_err);
+	if (err) {
+		parse_events_error__print(&parse_err, buf);
+		*ret = TEST_FAIL;
+	}
+	parse_events_error__exit(&parse_err);
+	evlist__for_each_entry(evlist, evsel) {
+		bool fail = false;
+		bool p_core_pmu = evsel->pmu->type == PERF_TYPE_RAW;
+		const char *name = evsel__name(evsel);
+
+		if (strcasestr(name, "uops_retired.slots") ||
+		    strcasestr(name, "topdown.backend_bound_slots") ||
+		    strcasestr(name, "topdown.br_mispredict_slots") ||
+		    strcasestr(name, "topdown.memory_bound_slots") ||
+		    strcasestr(name, "topdown.bad_spec_slots") ||
+		    strcasestr(name, "topdown.slots_p")) {
+			if (arch_is_topdown_slots(evsel) || arch_is_topdown_metrics(evsel))
+				fail = true;
+		} else if (strcasestr(name, "slots")) {
+			if (arch_is_topdown_slots(evsel) != p_core_pmu ||
+			    arch_is_topdown_metrics(evsel))
+				fail = true;
+		} else if (strcasestr(name, "topdown")) {
+			if (arch_is_topdown_slots(evsel) ||
+			    arch_is_topdown_metrics(evsel) != p_core_pmu)
+				fail = true;
+		} else if (arch_is_topdown_slots(evsel) || arch_is_topdown_metrics(evsel)) {
+			fail = true;
+		}
+		if (fail) {
+			pr_debug("Broken topdown information for '%s'\n", evsel__name(evsel));
+			*ret = TEST_FAIL;
+		}
+	}
+	evlist__delete(evlist);
+	return 0;
+}
+
+static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
+{
+	int ret = TEST_OK;
+	struct perf_pmu *pmu = NULL;
+
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
+		if (perf_pmu__for_each_event(pmu, /*skip_duplicate_pmus=*/false, &ret, event_cb))
+			break;
+	}
+	return ret;
+}
+
+DEFINE_SUITE("x86 topdown", x86_topdown);
diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
index 3dd29ba2c23b..9bc80fff3aa0 100644
--- a/tools/perf/arch/x86/util/evsel.c
+++ b/tools/perf/arch/x86/util/evsel.c
@@ -23,47 +23,25 @@ void arch_evsel__set_sample_weight(struct evsel *evsel)
 bool evsel__sys_has_perf_metrics(const struct evsel *evsel)
 {
 	struct perf_pmu *pmu;
-	u32 type = evsel->core.attr.type;
 
-	/*
-	 * The PERF_TYPE_RAW type is the core PMU type, e.g., "cpu" PMU
-	 * on a non-hybrid machine, "cpu_core" PMU on a hybrid machine.
-	 * The slots event is only available for the core PMU, which
-	 * supports the perf metrics feature.
-	 * Checking both the PERF_TYPE_RAW type and the slots event
-	 * should be good enough to detect the perf metrics feature.
-	 */
-again:
-	switch (type) {
-	case PERF_TYPE_HARDWARE:
-	case PERF_TYPE_HW_CACHE:
-		type = evsel->core.attr.config >> PERF_PMU_TYPE_SHIFT;
-		if (type)
-			goto again;
-		break;
-	case PERF_TYPE_RAW:
-		break;
-	default:
+	if (!topdown_sys_has_perf_metrics())
 		return false;
-	}
-
-	pmu = evsel->pmu;
-	if (pmu && perf_pmu__is_fake(pmu))
-		pmu = NULL;
 
-	if (!pmu) {
-		while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
-			if (pmu->type == PERF_TYPE_RAW)
-				break;
-		}
-	}
-	return pmu && perf_pmu__have_event(pmu, "slots");
+	/*
+	 * The PERF_TYPE_RAW type is the core PMU type, e.g., "cpu" PMU on a
+	 * non-hybrid machine, "cpu_core" PMU on a hybrid machine.  The
+	 * topdown_sys_has_perf_metrics checks the slots event is only available
+	 * for the core PMU, which supports the perf metrics feature. Checking
+	 * both the PERF_TYPE_RAW type and the slots event should be good enough
+	 * to detect the perf metrics feature.
+	 */
+	pmu = evsel__find_pmu(evsel);
+	return pmu && pmu->type == PERF_TYPE_RAW;
 }
 
 bool arch_evsel__must_be_in_group(const struct evsel *evsel)
 {
-	if (!evsel__sys_has_perf_metrics(evsel) || !evsel->name ||
-	    strcasestr(evsel->name, "uops_retired.slots"))
+	if (!evsel__sys_has_perf_metrics(evsel))
 		return false;
 
 	return arch_is_topdown_metrics(evsel) || arch_is_topdown_slots(evsel);
diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
index d1c654839049..66b231fbf52e 100644
--- a/tools/perf/arch/x86/util/topdown.c
+++ b/tools/perf/arch/x86/util/topdown.c
@@ -1,6 +1,4 @@
 // SPDX-License-Identifier: GPL-2.0
-#include "api/fs/fs.h"
-#include "util/evsel.h"
 #include "util/evlist.h"
 #include "util/pmu.h"
 #include "util/pmus.h"
@@ -8,6 +6,9 @@
 #include "topdown.h"
 #include "evsel.h"
 
+// cmask=0, inv=0, pc=0, edge=0, umask=4, event=0
+#define TOPDOWN_SLOTS		0x0400
+
 /* Check whether there is a PMU which supports the perf metrics. */
 bool topdown_sys_has_perf_metrics(void)
 {
@@ -32,31 +33,19 @@ bool topdown_sys_has_perf_metrics(void)
 	return has_perf_metrics;
 }
 
-#define TOPDOWN_SLOTS		0x0400
 bool arch_is_topdown_slots(const struct evsel *evsel)
 {
-	if (evsel->core.attr.config == TOPDOWN_SLOTS)
-		return true;
-
-	return false;
+	return evsel->core.attr.type == PERF_TYPE_RAW &&
+	       evsel->core.attr.config == TOPDOWN_SLOTS &&
+	       evsel->core.attr.config1 == 0;
 }
 
 bool arch_is_topdown_metrics(const struct evsel *evsel)
 {
-	int config = evsel->core.attr.config;
-	const char *name_from_config;
-	struct perf_pmu *pmu;
-
-	/* All topdown events have an event code of 0. */
-	if ((config & 0xFF) != 0)
-		return false;
-
-	pmu = evsel__find_pmu(evsel);
-	if (!pmu || !pmu->is_core)
-		return false;
-
-	name_from_config = perf_pmu__name_from_config(pmu, config);
-	return name_from_config && strcasestr(name_from_config, "topdown");
+	// cmask=0, inv=0, pc=0, edge=0, umask=0x80-0x87, event=0
+	return evsel->core.attr.type == PERF_TYPE_RAW &&
+		(evsel->core.attr.config & 0xFFFFF8FF) == 0x8000 &&
+		evsel->core.attr.config1 == 0;
 }
 
 /*
diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
index 1bae9b1822d7..2349536cf882 100644
--- a/tools/perf/arch/x86/util/topdown.h
+++ b/tools/perf/arch/x86/util/topdown.h
@@ -2,6 +2,10 @@
 #ifndef _TOPDOWN_H
 #define _TOPDOWN_H 1
 
+#include <stdbool.h>
+
+struct evsel;
+
 bool topdown_sys_has_perf_metrics(void);
 bool arch_is_topdown_slots(const struct evsel *evsel);
 bool arch_is_topdown_metrics(const struct evsel *evsel);
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v2 2/2] perf parse-events: Fix missing slots for Intel topdown metric events

[Ian Rogers]

Topdown metric events require grouping with a slots event. In perf
metrics this is currently achieved by metrics adding an unnecessary
"0 * tma_info_thread_slots". New TMA metrics trigger optimizations of
the metric expression that removes the event and breaks the metric due
to the missing but required event. Add a pass immediately before
sorting and fixing parsed events, that insert a slots event if one is
missing. Update test expectations to match this.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/x86/util/evlist.c  | 24 ++++++++++++++++++++++++
 tools/perf/arch/x86/util/topdown.c | 27 +++++++++++++++++++++++++++
 tools/perf/arch/x86/util/topdown.h |  2 ++
 tools/perf/tests/parse-events.c    | 24 ++++++++++++------------
 tools/perf/util/evlist.h           |  1 +
 tools/perf/util/parse-events.c     | 10 ++++++++++
 6 files changed, 76 insertions(+), 12 deletions(-)

diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
index 1969758cc8c1..75e9d00a1494 100644
--- a/tools/perf/arch/x86/util/evlist.c
+++ b/tools/perf/arch/x86/util/evlist.c
@@ -81,3 +81,27 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
 	/* Default ordering by insertion index. */
 	return lhs->core.idx - rhs->core.idx;
 }
+
+int arch_evlist__add_required_events(struct list_head *list)
+{
+	struct evsel *pos, *metric_event = NULL;
+	int idx = 0;
+
+	if (!topdown_sys_has_perf_metrics())
+		return 0;
+
+	list_for_each_entry(pos, list, core.node) {
+		if (arch_is_topdown_slots(pos)) {
+			/* Slots event already present, nothing to do. */
+			return 0;
+		}
+		if (metric_event == NULL && arch_is_topdown_metrics(pos))
+			metric_event = pos;
+		idx++;
+	}
+	if (metric_event == NULL) {
+		/* No topdown metric events, nothing to do. */
+		return 0;
+	}
+	return topdown_insert_slots_event(list, idx + 1, metric_event);
+}
diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
index 66b231fbf52e..5d53e3d59c0c 100644
--- a/tools/perf/arch/x86/util/topdown.c
+++ b/tools/perf/arch/x86/util/topdown.c
@@ -77,3 +77,30 @@ bool arch_topdown_sample_read(struct evsel *leader)
 
 	return false;
 }
+
+/*
+ * Make a copy of the topdown metric event metric_event with the given index but
+ * change its configuration to be a topdown slots event. Copying from
+ * metric_event ensures modifiers are the same.
+ */
+int topdown_insert_slots_event(struct list_head *list, int idx, struct evsel *metric_event)
+{
+	struct evsel *evsel = evsel__new_idx(&metric_event->core.attr, idx);
+
+	if (!evsel)
+		return -ENOMEM;
+
+	evsel->core.attr.config = TOPDOWN_SLOTS;
+	evsel->core.cpus = perf_cpu_map__get(metric_event->core.cpus);
+	evsel->core.own_cpus = perf_cpu_map__get(metric_event->core.own_cpus);
+	evsel->core.is_pmu_core = true;
+	evsel->pmu = metric_event->pmu;
+	evsel->name = strdup("slots");
+	evsel->precise_max = metric_event->precise_max;
+	evsel->sample_read = metric_event->sample_read;
+	evsel->weak_group = metric_event->weak_group;
+	evsel->bpf_counter = metric_event->bpf_counter;
+	evsel->retire_lat = metric_event->retire_lat;
+	list_add_tail(&evsel->core.node, list);
+	return 0;
+}
diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
index 2349536cf882..69035565e649 100644
--- a/tools/perf/arch/x86/util/topdown.h
+++ b/tools/perf/arch/x86/util/topdown.h
@@ -5,9 +5,11 @@
 #include <stdbool.h>
 
 struct evsel;
+struct list_head;
 
 bool topdown_sys_has_perf_metrics(void);
 bool arch_is_topdown_slots(const struct evsel *evsel);
 bool arch_is_topdown_metrics(const struct evsel *evsel);
+int topdown_insert_slots_event(struct list_head *list, int idx, struct evsel *metric_event);
 
 #endif
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 5ec2e5607987..bb8004397650 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -719,20 +719,20 @@ static int test__checkevent_pmu_partial_time_callgraph(struct evlist *evlist)
 
 static int test__checkevent_pmu_events(struct evlist *evlist)
 {
-	struct evsel *evsel = evlist__first(evlist);
+	struct evsel *evsel;
 
-	TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->core.nr_entries);
-	TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type ||
-				      strcmp(evsel->pmu->name, "cpu"));
-	TEST_ASSERT_VAL("wrong exclude_user",
-			!evsel->core.attr.exclude_user);
-	TEST_ASSERT_VAL("wrong exclude_kernel",
-			evsel->core.attr.exclude_kernel);
-	TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
-	TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
-	TEST_ASSERT_VAL("wrong pinned", !evsel->core.attr.pinned);
-	TEST_ASSERT_VAL("wrong exclusive", !evsel->core.attr.exclusive);
+	TEST_ASSERT_VAL("wrong number of entries", 1 <= evlist->core.nr_entries);
 
+	evlist__for_each_entry(evlist, evsel) {
+		TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type ||
+					      strcmp(evsel->pmu->name, "cpu"));
+		TEST_ASSERT_VAL("wrong exclude_user", !evsel->core.attr.exclude_user);
+		TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
+		TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
+		TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
+		TEST_ASSERT_VAL("wrong pinned", !evsel->core.attr.pinned);
+		TEST_ASSERT_VAL("wrong exclusive", !evsel->core.attr.exclusive);
+	}
 	return TEST_OK;
 }
 
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index fac1a01ba13f..1472d2179be1 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -111,6 +111,7 @@ void evlist__add(struct evlist *evlist, struct evsel *entry);
 void evlist__remove(struct evlist *evlist, struct evsel *evsel);
 
 int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs);
+int arch_evlist__add_required_events(struct list_head *list);
 
 int evlist__add_dummy(struct evlist *evlist);
 struct evsel *evlist__add_aux_dummy(struct evlist *evlist, bool system_wide);
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index a59ae5ca0f89..01dab5dd3540 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2132,6 +2132,11 @@ static int evlist__cmp(void *_fg_idx, const struct list_head *l, const struct li
 	return arch_evlist__cmp(lhs, rhs);
 }
 
+int __weak arch_evlist__add_required_events(struct list_head *list __always_unused)
+{
+	return 0;
+}
+
 static int parse_events__sort_events_and_fix_groups(struct list_head *list)
 {
 	int idx = 0, force_grouped_idx = -1;
@@ -2143,6 +2148,11 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
 	struct evsel *force_grouped_leader = NULL;
 	bool last_event_was_forced_leader = false;
 
+	/* On x86 topdown metrics events require a slots event. */
+	ret = arch_evlist__add_required_events(list);
+	if (ret)
+		return ret;
+
 	/*
 	 * Compute index to insert ungrouped events at. Place them where the
 	 * first ungrouped event appears.
-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH v2 2/2] perf parse-events: Fix missing slots for Intel topdown metric events

[Ian Rogers]

On Fri, Jul 18, 2025 at 6:27 AM Ian Rogers <irogers@google.com> wrote:
>
> Topdown metric events require grouping with a slots event. In perf
> metrics this is currently achieved by metrics adding an unnecessary
> "0 * tma_info_thread_slots". New TMA metrics trigger optimizations of
> the metric expression that removes the event and breaks the metric due
> to the missing but required event. Add a pass immediately before
> sorting and fixing parsed events, that insert a slots event if one is
> missing. Update test expectations to match this.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/x86/util/evlist.c  | 24 ++++++++++++++++++++++++
>  tools/perf/arch/x86/util/topdown.c | 27 +++++++++++++++++++++++++++
>  tools/perf/arch/x86/util/topdown.h |  2 ++
>  tools/perf/tests/parse-events.c    | 24 ++++++++++++------------
>  tools/perf/util/evlist.h           |  1 +
>  tools/perf/util/parse-events.c     | 10 ++++++++++
>  6 files changed, 76 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index 1969758cc8c1..75e9d00a1494 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -81,3 +81,27 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>         /* Default ordering by insertion index. */
>         return lhs->core.idx - rhs->core.idx;
>  }
> +
> +int arch_evlist__add_required_events(struct list_head *list)
> +{
> +       struct evsel *pos, *metric_event = NULL;
> +       int idx = 0;
> +
> +       if (!topdown_sys_has_perf_metrics())
> +               return 0;
> +
> +       list_for_each_entry(pos, list, core.node) {
> +               if (arch_is_topdown_slots(pos)) {
> +                       /* Slots event already present, nothing to do. */
> +                       return 0;
> +               }
> +               if (metric_event == NULL && arch_is_topdown_metrics(pos))
> +                       metric_event = pos;
> +               idx++;
> +       }
> +       if (metric_event == NULL) {
> +               /* No topdown metric events, nothing to do. */
> +               return 0;
> +       }
> +       return topdown_insert_slots_event(list, idx + 1, metric_event);
> +}
> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> index 66b231fbf52e..5d53e3d59c0c 100644
> --- a/tools/perf/arch/x86/util/topdown.c
> +++ b/tools/perf/arch/x86/util/topdown.c
> @@ -77,3 +77,30 @@ bool arch_topdown_sample_read(struct evsel *leader)
>
>         return false;
>  }
> +
> +/*
> + * Make a copy of the topdown metric event metric_event with the given index but
> + * change its configuration to be a topdown slots event. Copying from
> + * metric_event ensures modifiers are the same.
> + */
> +int topdown_insert_slots_event(struct list_head *list, int idx, struct evsel *metric_event)
> +{
> +       struct evsel *evsel = evsel__new_idx(&metric_event->core.attr, idx);
> +
> +       if (!evsel)
> +               return -ENOMEM;
> +
> +       evsel->core.attr.config = TOPDOWN_SLOTS;
> +       evsel->core.cpus = perf_cpu_map__get(metric_event->core.cpus);
> +       evsel->core.own_cpus = perf_cpu_map__get(metric_event->core.own_cpus);

Note, this change conflicts with:
https://lore.kernel.org/lkml/20250717210233.1143622-1-irogers@google.com/
(reviewed but not merged) due to own_cpus being renamed to pmu_cpus. The fix is:
```
--- a/tools/perf/arch/x86/util/topdown.c
+++ b/tools/perf/arch/x86/util/topdown.c
@@ -92,7 +92,7 @@ int topdown_insert_slots_event(struct list_head
*list, int idx, struct evsel *me

       evsel->core.attr.config = TOPDOWN_SLOTS;
       evsel->core.cpus = perf_cpu_map__get(metric_event->core.cpus);
-       evsel->core.own_cpus = perf_cpu_map__get(metric_event->core.own_cpus);
+       evsel->core.pmu_cpus = perf_cpu_map__get(metric_event->core.pmu_cpus);
       evsel->core.is_pmu_core = true;
       evsel->pmu = metric_event->pmu;
       evsel->name = strdup("slots");
```
and I can send this fix when:
https://lore.kernel.org/lkml/20250717210233.1143622-1-irogers@google.com/
lands.

Thanks,
Ian

> +       evsel->core.is_pmu_core = true;
> +       evsel->pmu = metric_event->pmu;
> +       evsel->name = strdup("slots");
> +       evsel->precise_max = metric_event->precise_max;
> +       evsel->sample_read = metric_event->sample_read;
> +       evsel->weak_group = metric_event->weak_group;
> +       evsel->bpf_counter = metric_event->bpf_counter;
> +       evsel->retire_lat = metric_event->retire_lat;
> +       list_add_tail(&evsel->core.node, list);
> +       return 0;
> +}
> diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
> index 2349536cf882..69035565e649 100644
> --- a/tools/perf/arch/x86/util/topdown.h
> +++ b/tools/perf/arch/x86/util/topdown.h
> @@ -5,9 +5,11 @@
>  #include <stdbool.h>
>
>  struct evsel;
> +struct list_head;
>
>  bool topdown_sys_has_perf_metrics(void);
>  bool arch_is_topdown_slots(const struct evsel *evsel);
>  bool arch_is_topdown_metrics(const struct evsel *evsel);
> +int topdown_insert_slots_event(struct list_head *list, int idx, struct evsel *metric_event);
>
>  #endif
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 5ec2e5607987..bb8004397650 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -719,20 +719,20 @@ static int test__checkevent_pmu_partial_time_callgraph(struct evlist *evlist)
>
>  static int test__checkevent_pmu_events(struct evlist *evlist)
>  {
> -       struct evsel *evsel = evlist__first(evlist);
> +       struct evsel *evsel;
>
> -       TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->core.nr_entries);
> -       TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type ||
> -                                     strcmp(evsel->pmu->name, "cpu"));
> -       TEST_ASSERT_VAL("wrong exclude_user",
> -                       !evsel->core.attr.exclude_user);
> -       TEST_ASSERT_VAL("wrong exclude_kernel",
> -                       evsel->core.attr.exclude_kernel);
> -       TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> -       TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
> -       TEST_ASSERT_VAL("wrong pinned", !evsel->core.attr.pinned);
> -       TEST_ASSERT_VAL("wrong exclusive", !evsel->core.attr.exclusive);
> +       TEST_ASSERT_VAL("wrong number of entries", 1 <= evlist->core.nr_entries);
>
> +       evlist__for_each_entry(evlist, evsel) {
> +               TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type ||
> +                                             strcmp(evsel->pmu->name, "cpu"));
> +               TEST_ASSERT_VAL("wrong exclude_user", !evsel->core.attr.exclude_user);
> +               TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
> +               TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> +               TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
> +               TEST_ASSERT_VAL("wrong pinned", !evsel->core.attr.pinned);
> +               TEST_ASSERT_VAL("wrong exclusive", !evsel->core.attr.exclusive);
> +       }
>         return TEST_OK;
>  }
>
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index fac1a01ba13f..1472d2179be1 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -111,6 +111,7 @@ void evlist__add(struct evlist *evlist, struct evsel *entry);
>  void evlist__remove(struct evlist *evlist, struct evsel *evsel);
>
>  int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs);
> +int arch_evlist__add_required_events(struct list_head *list);
>
>  int evlist__add_dummy(struct evlist *evlist);
>  struct evsel *evlist__add_aux_dummy(struct evlist *evlist, bool system_wide);
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index a59ae5ca0f89..01dab5dd3540 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2132,6 +2132,11 @@ static int evlist__cmp(void *_fg_idx, const struct list_head *l, const struct li
>         return arch_evlist__cmp(lhs, rhs);
>  }
>
> +int __weak arch_evlist__add_required_events(struct list_head *list __always_unused)
> +{
> +       return 0;
> +}
> +
>  static int parse_events__sort_events_and_fix_groups(struct list_head *list)
>  {
>         int idx = 0, force_grouped_idx = -1;
> @@ -2143,6 +2148,11 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
>         struct evsel *force_grouped_leader = NULL;
>         bool last_event_was_forced_leader = false;
>
> +       /* On x86 topdown metrics events require a slots event. */
> +       ret = arch_evlist__add_required_events(list);
> +       if (ret)
> +               return ret;
> +
>         /*
>          * Compute index to insert ungrouped events at. Place them where the
>          * first ungrouped event appears.
> --
> 2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCH v2 1/2] perf topdown: Use attribute to see an event is a topdown metic or slots

[Namhyung Kim]

On Fri, Jul 18, 2025 at 06:27:49AM -0700, Ian Rogers wrote:
> The string comparisons were overly broad and could fire for the
> incorrect PMU and events. Switch to using the config in the attribute
> then add a perf test to confirm the attribute config values match
> those of parsed events of that name and don't match others. This
> exposed matches for slots events that shouldn't have matched as the
> slots fixed counter event, such as topdown.slots_p.
> 
> Fixes: fbc798316bef ("perf x86/topdown: Refine helper arch_is_topdown_metrics()")
> Signed-off-by: Ian Rogers <irogers@google.com>

Dapeng, are you ok with this (or Ian's TMA fix v3 below)?

https://lore.kernel.org/r/20250719030517.1990983-1-irogers@google.com

Thanks,
Namhyung


> ---
> v2: In test rename topdown_pmu to p_core_pmu for clarity.
> ---
>  tools/perf/arch/x86/include/arch-tests.h |  4 ++
>  tools/perf/arch/x86/tests/Build          |  1 +
>  tools/perf/arch/x86/tests/arch-tests.c   |  1 +
>  tools/perf/arch/x86/tests/topdown.c      | 76 ++++++++++++++++++++++++
>  tools/perf/arch/x86/util/evsel.c         | 46 ++++----------
>  tools/perf/arch/x86/util/topdown.c       | 31 ++++------
>  tools/perf/arch/x86/util/topdown.h       |  4 ++
>  7 files changed, 108 insertions(+), 55 deletions(-)
>  create mode 100644 tools/perf/arch/x86/tests/topdown.c
> 
> diff --git a/tools/perf/arch/x86/include/arch-tests.h b/tools/perf/arch/x86/include/arch-tests.h
> index 4fd425157d7d..8713e9122d4c 100644
> --- a/tools/perf/arch/x86/include/arch-tests.h
> +++ b/tools/perf/arch/x86/include/arch-tests.h
> @@ -2,6 +2,8 @@
>  #ifndef ARCH_TESTS_H
>  #define ARCH_TESTS_H
>  
> +#include "tests/tests.h"
> +
>  struct test_suite;
>  
>  /* Tests */
> @@ -17,6 +19,8 @@ int test__amd_ibs_via_core_pmu(struct test_suite *test, int subtest);
>  int test__amd_ibs_period(struct test_suite *test, int subtest);
>  int test__hybrid(struct test_suite *test, int subtest);
>  
> +DECLARE_SUITE(x86_topdown);
> +
>  extern struct test_suite *arch_tests[];
>  
>  #endif
> diff --git a/tools/perf/arch/x86/tests/Build b/tools/perf/arch/x86/tests/Build
> index 01d5527f38c7..311b6b53d3d8 100644
> --- a/tools/perf/arch/x86/tests/Build
> +++ b/tools/perf/arch/x86/tests/Build
> @@ -11,6 +11,7 @@ endif
>  perf-test-$(CONFIG_X86_64) += bp-modify.o
>  perf-test-y += amd-ibs-via-core-pmu.o
>  perf-test-y += amd-ibs-period.o
> +perf-test-y += topdown.o
>  
>  ifdef SHELLCHECK
>    SHELL_TESTS := gen-insn-x86-dat.sh
> diff --git a/tools/perf/arch/x86/tests/arch-tests.c b/tools/perf/arch/x86/tests/arch-tests.c
> index bfee2432515b..29ec1861ccef 100644
> --- a/tools/perf/arch/x86/tests/arch-tests.c
> +++ b/tools/perf/arch/x86/tests/arch-tests.c
> @@ -53,5 +53,6 @@ struct test_suite *arch_tests[] = {
>  	&suite__amd_ibs_via_core_pmu,
>  	&suite__amd_ibs_period,
>  	&suite__hybrid,
> +	&suite__x86_topdown,
>  	NULL,
>  };
> diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
> new file mode 100644
> index 000000000000..8d0ea7a4bbc1
> --- /dev/null
> +++ b/tools/perf/arch/x86/tests/topdown.c
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "arch-tests.h"
> +#include "../util/topdown.h"
> +#include "evlist.h"
> +#include "parse-events.h"
> +#include "pmu.h"
> +#include "pmus.h"
> +
> +static int event_cb(void *state, struct pmu_event_info *info)
> +{
> +	char buf[256];
> +	struct parse_events_error parse_err;
> +	int *ret = state, err;
> +	struct evlist *evlist = evlist__new();
> +	struct evsel *evsel;
> +
> +	if (!evlist)
> +		return -ENOMEM;
> +
> +	parse_events_error__init(&parse_err);
> +	snprintf(buf, sizeof(buf), "%s/%s/", info->pmu->name, info->name);
> +	err = parse_events(evlist, buf, &parse_err);
> +	if (err) {
> +		parse_events_error__print(&parse_err, buf);
> +		*ret = TEST_FAIL;
> +	}
> +	parse_events_error__exit(&parse_err);
> +	evlist__for_each_entry(evlist, evsel) {
> +		bool fail = false;
> +		bool p_core_pmu = evsel->pmu->type == PERF_TYPE_RAW;
> +		const char *name = evsel__name(evsel);
> +
> +		if (strcasestr(name, "uops_retired.slots") ||
> +		    strcasestr(name, "topdown.backend_bound_slots") ||
> +		    strcasestr(name, "topdown.br_mispredict_slots") ||
> +		    strcasestr(name, "topdown.memory_bound_slots") ||
> +		    strcasestr(name, "topdown.bad_spec_slots") ||
> +		    strcasestr(name, "topdown.slots_p")) {
> +			if (arch_is_topdown_slots(evsel) || arch_is_topdown_metrics(evsel))
> +				fail = true;
> +		} else if (strcasestr(name, "slots")) {
> +			if (arch_is_topdown_slots(evsel) != p_core_pmu ||
> +			    arch_is_topdown_metrics(evsel))
> +				fail = true;
> +		} else if (strcasestr(name, "topdown")) {
> +			if (arch_is_topdown_slots(evsel) ||
> +			    arch_is_topdown_metrics(evsel) != p_core_pmu)
> +				fail = true;
> +		} else if (arch_is_topdown_slots(evsel) || arch_is_topdown_metrics(evsel)) {
> +			fail = true;
> +		}
> +		if (fail) {
> +			pr_debug("Broken topdown information for '%s'\n", evsel__name(evsel));
> +			*ret = TEST_FAIL;
> +		}
> +	}
> +	evlist__delete(evlist);
> +	return 0;
> +}
> +
> +static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> +{
> +	int ret = TEST_OK;
> +	struct perf_pmu *pmu = NULL;
> +
> +	if (!topdown_sys_has_perf_metrics())
> +		return TEST_OK;
> +
> +	while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
> +		if (perf_pmu__for_each_event(pmu, /*skip_duplicate_pmus=*/false, &ret, event_cb))
> +			break;
> +	}
> +	return ret;
> +}
> +
> +DEFINE_SUITE("x86 topdown", x86_topdown);
> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> index 3dd29ba2c23b..9bc80fff3aa0 100644
> --- a/tools/perf/arch/x86/util/evsel.c
> +++ b/tools/perf/arch/x86/util/evsel.c
> @@ -23,47 +23,25 @@ void arch_evsel__set_sample_weight(struct evsel *evsel)
>  bool evsel__sys_has_perf_metrics(const struct evsel *evsel)
>  {
>  	struct perf_pmu *pmu;
> -	u32 type = evsel->core.attr.type;
>  
> -	/*
> -	 * The PERF_TYPE_RAW type is the core PMU type, e.g., "cpu" PMU
> -	 * on a non-hybrid machine, "cpu_core" PMU on a hybrid machine.
> -	 * The slots event is only available for the core PMU, which
> -	 * supports the perf metrics feature.
> -	 * Checking both the PERF_TYPE_RAW type and the slots event
> -	 * should be good enough to detect the perf metrics feature.
> -	 */
> -again:
> -	switch (type) {
> -	case PERF_TYPE_HARDWARE:
> -	case PERF_TYPE_HW_CACHE:
> -		type = evsel->core.attr.config >> PERF_PMU_TYPE_SHIFT;
> -		if (type)
> -			goto again;
> -		break;
> -	case PERF_TYPE_RAW:
> -		break;
> -	default:
> +	if (!topdown_sys_has_perf_metrics())
>  		return false;
> -	}
> -
> -	pmu = evsel->pmu;
> -	if (pmu && perf_pmu__is_fake(pmu))
> -		pmu = NULL;
>  
> -	if (!pmu) {
> -		while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
> -			if (pmu->type == PERF_TYPE_RAW)
> -				break;
> -		}
> -	}
> -	return pmu && perf_pmu__have_event(pmu, "slots");
> +	/*
> +	 * The PERF_TYPE_RAW type is the core PMU type, e.g., "cpu" PMU on a
> +	 * non-hybrid machine, "cpu_core" PMU on a hybrid machine.  The
> +	 * topdown_sys_has_perf_metrics checks the slots event is only available
> +	 * for the core PMU, which supports the perf metrics feature. Checking
> +	 * both the PERF_TYPE_RAW type and the slots event should be good enough
> +	 * to detect the perf metrics feature.
> +	 */
> +	pmu = evsel__find_pmu(evsel);
> +	return pmu && pmu->type == PERF_TYPE_RAW;
>  }
>  
>  bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>  {
> -	if (!evsel__sys_has_perf_metrics(evsel) || !evsel->name ||
> -	    strcasestr(evsel->name, "uops_retired.slots"))
> +	if (!evsel__sys_has_perf_metrics(evsel))
>  		return false;
>  
>  	return arch_is_topdown_metrics(evsel) || arch_is_topdown_slots(evsel);
> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> index d1c654839049..66b231fbf52e 100644
> --- a/tools/perf/arch/x86/util/topdown.c
> +++ b/tools/perf/arch/x86/util/topdown.c
> @@ -1,6 +1,4 @@
>  // SPDX-License-Identifier: GPL-2.0
> -#include "api/fs/fs.h"
> -#include "util/evsel.h"
>  #include "util/evlist.h"
>  #include "util/pmu.h"
>  #include "util/pmus.h"
> @@ -8,6 +6,9 @@
>  #include "topdown.h"
>  #include "evsel.h"
>  
> +// cmask=0, inv=0, pc=0, edge=0, umask=4, event=0
> +#define TOPDOWN_SLOTS		0x0400
> +
>  /* Check whether there is a PMU which supports the perf metrics. */
>  bool topdown_sys_has_perf_metrics(void)
>  {
> @@ -32,31 +33,19 @@ bool topdown_sys_has_perf_metrics(void)
>  	return has_perf_metrics;
>  }
>  
> -#define TOPDOWN_SLOTS		0x0400
>  bool arch_is_topdown_slots(const struct evsel *evsel)
>  {
> -	if (evsel->core.attr.config == TOPDOWN_SLOTS)
> -		return true;
> -
> -	return false;
> +	return evsel->core.attr.type == PERF_TYPE_RAW &&
> +	       evsel->core.attr.config == TOPDOWN_SLOTS &&
> +	       evsel->core.attr.config1 == 0;
>  }
>  
>  bool arch_is_topdown_metrics(const struct evsel *evsel)
>  {
> -	int config = evsel->core.attr.config;
> -	const char *name_from_config;
> -	struct perf_pmu *pmu;
> -
> -	/* All topdown events have an event code of 0. */
> -	if ((config & 0xFF) != 0)
> -		return false;
> -
> -	pmu = evsel__find_pmu(evsel);
> -	if (!pmu || !pmu->is_core)
> -		return false;
> -
> -	name_from_config = perf_pmu__name_from_config(pmu, config);
> -	return name_from_config && strcasestr(name_from_config, "topdown");
> +	// cmask=0, inv=0, pc=0, edge=0, umask=0x80-0x87, event=0
> +	return evsel->core.attr.type == PERF_TYPE_RAW &&
> +		(evsel->core.attr.config & 0xFFFFF8FF) == 0x8000 &&
> +		evsel->core.attr.config1 == 0;
>  }
>  
>  /*
> diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
> index 1bae9b1822d7..2349536cf882 100644
> --- a/tools/perf/arch/x86/util/topdown.h
> +++ b/tools/perf/arch/x86/util/topdown.h
> @@ -2,6 +2,10 @@
>  #ifndef _TOPDOWN_H
>  #define _TOPDOWN_H 1
>  
> +#include <stdbool.h>
> +
> +struct evsel;
> +
>  bool topdown_sys_has_perf_metrics(void);
>  bool arch_is_topdown_slots(const struct evsel *evsel);
>  bool arch_is_topdown_metrics(const struct evsel *evsel);
> -- 
> 2.50.0.727.gbf7dc18ff4-goog
> 

Re: [PATCH v2 1/2] perf topdown: Use attribute to see an event is a topdown metic or slots

[Namhyung Kim]

On Tue, Jul 22, 2025 at 06:03:49PM -0700, Namhyung Kim wrote:
> On Fri, Jul 18, 2025 at 06:27:49AM -0700, Ian Rogers wrote:
> > The string comparisons were overly broad and could fire for the
> > incorrect PMU and events. Switch to using the config in the attribute
> > then add a perf test to confirm the attribute config values match
> > those of parsed events of that name and don't match others. This
> > exposed matches for slots events that shouldn't have matched as the
> > slots fixed counter event, such as topdown.slots_p.
> > 
> > Fixes: fbc798316bef ("perf x86/topdown: Refine helper arch_is_topdown_metrics()")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Dapeng, are you ok with this (or Ian's TMA fix v3 below)?
> 
> https://lore.kernel.org/r/20250719030517.1990983-1-irogers@google.com
 
AFAICS Ian already replied to the thread and I think it's handled.
I'm gonna take the TMA fix v3, please let me know if you can find
problems later.

Thanks,
Namhyung

> 
> > ---
> > v2: In test rename topdown_pmu to p_core_pmu for clarity.
> > ---
> >  tools/perf/arch/x86/include/arch-tests.h |  4 ++
> >  tools/perf/arch/x86/tests/Build          |  1 +
> >  tools/perf/arch/x86/tests/arch-tests.c   |  1 +
> >  tools/perf/arch/x86/tests/topdown.c      | 76 ++++++++++++++++++++++++
> >  tools/perf/arch/x86/util/evsel.c         | 46 ++++----------
> >  tools/perf/arch/x86/util/topdown.c       | 31 ++++------
> >  tools/perf/arch/x86/util/topdown.h       |  4 ++
> >  7 files changed, 108 insertions(+), 55 deletions(-)
> >  create mode 100644 tools/perf/arch/x86/tests/topdown.c
> > 
> > diff --git a/tools/perf/arch/x86/include/arch-tests.h b/tools/perf/arch/x86/include/arch-tests.h
> > index 4fd425157d7d..8713e9122d4c 100644
> > --- a/tools/perf/arch/x86/include/arch-tests.h
> > +++ b/tools/perf/arch/x86/include/arch-tests.h
> > @@ -2,6 +2,8 @@
> >  #ifndef ARCH_TESTS_H
> >  #define ARCH_TESTS_H
> >  
> > +#include "tests/tests.h"
> > +
> >  struct test_suite;
> >  
> >  /* Tests */
> > @@ -17,6 +19,8 @@ int test__amd_ibs_via_core_pmu(struct test_suite *test, int subtest);
> >  int test__amd_ibs_period(struct test_suite *test, int subtest);
> >  int test__hybrid(struct test_suite *test, int subtest);
> >  
> > +DECLARE_SUITE(x86_topdown);
> > +
> >  extern struct test_suite *arch_tests[];
> >  
> >  #endif
> > diff --git a/tools/perf/arch/x86/tests/Build b/tools/perf/arch/x86/tests/Build
> > index 01d5527f38c7..311b6b53d3d8 100644
> > --- a/tools/perf/arch/x86/tests/Build
> > +++ b/tools/perf/arch/x86/tests/Build
> > @@ -11,6 +11,7 @@ endif
> >  perf-test-$(CONFIG_X86_64) += bp-modify.o
> >  perf-test-y += amd-ibs-via-core-pmu.o
> >  perf-test-y += amd-ibs-period.o
> > +perf-test-y += topdown.o
> >  
> >  ifdef SHELLCHECK
> >    SHELL_TESTS := gen-insn-x86-dat.sh
> > diff --git a/tools/perf/arch/x86/tests/arch-tests.c b/tools/perf/arch/x86/tests/arch-tests.c
> > index bfee2432515b..29ec1861ccef 100644
> > --- a/tools/perf/arch/x86/tests/arch-tests.c
> > +++ b/tools/perf/arch/x86/tests/arch-tests.c
> > @@ -53,5 +53,6 @@ struct test_suite *arch_tests[] = {
> >  	&suite__amd_ibs_via_core_pmu,
> >  	&suite__amd_ibs_period,
> >  	&suite__hybrid,
> > +	&suite__x86_topdown,
> >  	NULL,
> >  };
> > diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
> > new file mode 100644
> > index 000000000000..8d0ea7a4bbc1
> > --- /dev/null
> > +++ b/tools/perf/arch/x86/tests/topdown.c
> > @@ -0,0 +1,76 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "arch-tests.h"
> > +#include "../util/topdown.h"
> > +#include "evlist.h"
> > +#include "parse-events.h"
> > +#include "pmu.h"
> > +#include "pmus.h"
> > +
> > +static int event_cb(void *state, struct pmu_event_info *info)
> > +{
> > +	char buf[256];
> > +	struct parse_events_error parse_err;
> > +	int *ret = state, err;
> > +	struct evlist *evlist = evlist__new();
> > +	struct evsel *evsel;
> > +
> > +	if (!evlist)
> > +		return -ENOMEM;
> > +
> > +	parse_events_error__init(&parse_err);
> > +	snprintf(buf, sizeof(buf), "%s/%s/", info->pmu->name, info->name);
> > +	err = parse_events(evlist, buf, &parse_err);
> > +	if (err) {
> > +		parse_events_error__print(&parse_err, buf);
> > +		*ret = TEST_FAIL;
> > +	}
> > +	parse_events_error__exit(&parse_err);
> > +	evlist__for_each_entry(evlist, evsel) {
> > +		bool fail = false;
> > +		bool p_core_pmu = evsel->pmu->type == PERF_TYPE_RAW;
> > +		const char *name = evsel__name(evsel);
> > +
> > +		if (strcasestr(name, "uops_retired.slots") ||
> > +		    strcasestr(name, "topdown.backend_bound_slots") ||
> > +		    strcasestr(name, "topdown.br_mispredict_slots") ||
> > +		    strcasestr(name, "topdown.memory_bound_slots") ||
> > +		    strcasestr(name, "topdown.bad_spec_slots") ||
> > +		    strcasestr(name, "topdown.slots_p")) {
> > +			if (arch_is_topdown_slots(evsel) || arch_is_topdown_metrics(evsel))
> > +				fail = true;
> > +		} else if (strcasestr(name, "slots")) {
> > +			if (arch_is_topdown_slots(evsel) != p_core_pmu ||
> > +			    arch_is_topdown_metrics(evsel))
> > +				fail = true;
> > +		} else if (strcasestr(name, "topdown")) {
> > +			if (arch_is_topdown_slots(evsel) ||
> > +			    arch_is_topdown_metrics(evsel) != p_core_pmu)
> > +				fail = true;
> > +		} else if (arch_is_topdown_slots(evsel) || arch_is_topdown_metrics(evsel)) {
> > +			fail = true;
> > +		}
> > +		if (fail) {
> > +			pr_debug("Broken topdown information for '%s'\n", evsel__name(evsel));
> > +			*ret = TEST_FAIL;
> > +		}
> > +	}
> > +	evlist__delete(evlist);
> > +	return 0;
> > +}
> > +
> > +static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> > +{
> > +	int ret = TEST_OK;
> > +	struct perf_pmu *pmu = NULL;
> > +
> > +	if (!topdown_sys_has_perf_metrics())
> > +		return TEST_OK;
> > +
> > +	while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
> > +		if (perf_pmu__for_each_event(pmu, /*skip_duplicate_pmus=*/false, &ret, event_cb))
> > +			break;
> > +	}
> > +	return ret;
> > +}
> > +
> > +DEFINE_SUITE("x86 topdown", x86_topdown);
> > diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> > index 3dd29ba2c23b..9bc80fff3aa0 100644
> > --- a/tools/perf/arch/x86/util/evsel.c
> > +++ b/tools/perf/arch/x86/util/evsel.c
> > @@ -23,47 +23,25 @@ void arch_evsel__set_sample_weight(struct evsel *evsel)
> >  bool evsel__sys_has_perf_metrics(const struct evsel *evsel)
> >  {
> >  	struct perf_pmu *pmu;
> > -	u32 type = evsel->core.attr.type;
> >  
> > -	/*
> > -	 * The PERF_TYPE_RAW type is the core PMU type, e.g., "cpu" PMU
> > -	 * on a non-hybrid machine, "cpu_core" PMU on a hybrid machine.
> > -	 * The slots event is only available for the core PMU, which
> > -	 * supports the perf metrics feature.
> > -	 * Checking both the PERF_TYPE_RAW type and the slots event
> > -	 * should be good enough to detect the perf metrics feature.
> > -	 */
> > -again:
> > -	switch (type) {
> > -	case PERF_TYPE_HARDWARE:
> > -	case PERF_TYPE_HW_CACHE:
> > -		type = evsel->core.attr.config >> PERF_PMU_TYPE_SHIFT;
> > -		if (type)
> > -			goto again;
> > -		break;
> > -	case PERF_TYPE_RAW:
> > -		break;
> > -	default:
> > +	if (!topdown_sys_has_perf_metrics())
> >  		return false;
> > -	}
> > -
> > -	pmu = evsel->pmu;
> > -	if (pmu && perf_pmu__is_fake(pmu))
> > -		pmu = NULL;
> >  
> > -	if (!pmu) {
> > -		while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
> > -			if (pmu->type == PERF_TYPE_RAW)
> > -				break;
> > -		}
> > -	}
> > -	return pmu && perf_pmu__have_event(pmu, "slots");
> > +	/*
> > +	 * The PERF_TYPE_RAW type is the core PMU type, e.g., "cpu" PMU on a
> > +	 * non-hybrid machine, "cpu_core" PMU on a hybrid machine.  The
> > +	 * topdown_sys_has_perf_metrics checks the slots event is only available
> > +	 * for the core PMU, which supports the perf metrics feature. Checking
> > +	 * both the PERF_TYPE_RAW type and the slots event should be good enough
> > +	 * to detect the perf metrics feature.
> > +	 */
> > +	pmu = evsel__find_pmu(evsel);
> > +	return pmu && pmu->type == PERF_TYPE_RAW;
> >  }
> >  
> >  bool arch_evsel__must_be_in_group(const struct evsel *evsel)
> >  {
> > -	if (!evsel__sys_has_perf_metrics(evsel) || !evsel->name ||
> > -	    strcasestr(evsel->name, "uops_retired.slots"))
> > +	if (!evsel__sys_has_perf_metrics(evsel))
> >  		return false;
> >  
> >  	return arch_is_topdown_metrics(evsel) || arch_is_topdown_slots(evsel);
> > diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> > index d1c654839049..66b231fbf52e 100644
> > --- a/tools/perf/arch/x86/util/topdown.c
> > +++ b/tools/perf/arch/x86/util/topdown.c
> > @@ -1,6 +1,4 @@
> >  // SPDX-License-Identifier: GPL-2.0
> > -#include "api/fs/fs.h"
> > -#include "util/evsel.h"
> >  #include "util/evlist.h"
> >  #include "util/pmu.h"
> >  #include "util/pmus.h"
> > @@ -8,6 +6,9 @@
> >  #include "topdown.h"
> >  #include "evsel.h"
> >  
> > +// cmask=0, inv=0, pc=0, edge=0, umask=4, event=0
> > +#define TOPDOWN_SLOTS		0x0400
> > +
> >  /* Check whether there is a PMU which supports the perf metrics. */
> >  bool topdown_sys_has_perf_metrics(void)
> >  {
> > @@ -32,31 +33,19 @@ bool topdown_sys_has_perf_metrics(void)
> >  	return has_perf_metrics;
> >  }
> >  
> > -#define TOPDOWN_SLOTS		0x0400
> >  bool arch_is_topdown_slots(const struct evsel *evsel)
> >  {
> > -	if (evsel->core.attr.config == TOPDOWN_SLOTS)
> > -		return true;
> > -
> > -	return false;
> > +	return evsel->core.attr.type == PERF_TYPE_RAW &&
> > +	       evsel->core.attr.config == TOPDOWN_SLOTS &&
> > +	       evsel->core.attr.config1 == 0;
> >  }
> >  
> >  bool arch_is_topdown_metrics(const struct evsel *evsel)
> >  {
> > -	int config = evsel->core.attr.config;
> > -	const char *name_from_config;
> > -	struct perf_pmu *pmu;
> > -
> > -	/* All topdown events have an event code of 0. */
> > -	if ((config & 0xFF) != 0)
> > -		return false;
> > -
> > -	pmu = evsel__find_pmu(evsel);
> > -	if (!pmu || !pmu->is_core)
> > -		return false;
> > -
> > -	name_from_config = perf_pmu__name_from_config(pmu, config);
> > -	return name_from_config && strcasestr(name_from_config, "topdown");
> > +	// cmask=0, inv=0, pc=0, edge=0, umask=0x80-0x87, event=0
> > +	return evsel->core.attr.type == PERF_TYPE_RAW &&
> > +		(evsel->core.attr.config & 0xFFFFF8FF) == 0x8000 &&
> > +		evsel->core.attr.config1 == 0;
> >  }
> >  
> >  /*
> > diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
> > index 1bae9b1822d7..2349536cf882 100644
> > --- a/tools/perf/arch/x86/util/topdown.h
> > +++ b/tools/perf/arch/x86/util/topdown.h
> > @@ -2,6 +2,10 @@
> >  #ifndef _TOPDOWN_H
> >  #define _TOPDOWN_H 1
> >  
> > +#include <stdbool.h>
> > +
> > +struct evsel;
> > +
> >  bool topdown_sys_has_perf_metrics(void);
> >  bool arch_is_topdown_slots(const struct evsel *evsel);
> >  bool arch_is_topdown_metrics(const struct evsel *evsel);
> > -- 
> > 2.50.0.727.gbf7dc18ff4-goog
> > 

Re: [PATCH v2 1/2] perf topdown: Use attribute to see an event is a topdown metic or slots

[Mi, Dapeng]

On 7/23/2025 9:03 AM, Namhyung Kim wrote:
> On Fri, Jul 18, 2025 at 06:27:49AM -0700, Ian Rogers wrote:
>> The string comparisons were overly broad and could fire for the
>> incorrect PMU and events. Switch to using the config in the attribute
>> then add a perf test to confirm the attribute config values match
>> those of parsed events of that name and don't match others. This
>> exposed matches for slots events that shouldn't have matched as the
>> slots fixed counter event, such as topdown.slots_p.
>>
>> Fixes: fbc798316bef ("perf x86/topdown: Refine helper arch_is_topdown_metrics()")
>> Signed-off-by: Ian Rogers <irogers@google.com>
> Dapeng, are you ok with this (or Ian's TMA fix v3 below)?
>
> https://lore.kernel.org/r/20250719030517.1990983-1-irogers@google.com

Sorry for late response (Just back from vacation). The patches look good to
me. I tested these patches on Sapphire Rapids and Panther Lake, no issue is
found. Thanks. 


>
> Thanks,
> Namhyung
>
>
>> ---
>> v2: In test rename topdown_pmu to p_core_pmu for clarity.
>> ---
>>  tools/perf/arch/x86/include/arch-tests.h |  4 ++
>>  tools/perf/arch/x86/tests/Build          |  1 +
>>  tools/perf/arch/x86/tests/arch-tests.c   |  1 +
>>  tools/perf/arch/x86/tests/topdown.c      | 76 ++++++++++++++++++++++++
>>  tools/perf/arch/x86/util/evsel.c         | 46 ++++----------
>>  tools/perf/arch/x86/util/topdown.c       | 31 ++++------
>>  tools/perf/arch/x86/util/topdown.h       |  4 ++
>>  7 files changed, 108 insertions(+), 55 deletions(-)
>>  create mode 100644 tools/perf/arch/x86/tests/topdown.c
>>
>> diff --git a/tools/perf/arch/x86/include/arch-tests.h b/tools/perf/arch/x86/include/arch-tests.h
>> index 4fd425157d7d..8713e9122d4c 100644
>> --- a/tools/perf/arch/x86/include/arch-tests.h
>> +++ b/tools/perf/arch/x86/include/arch-tests.h
>> @@ -2,6 +2,8 @@
>>  #ifndef ARCH_TESTS_H
>>  #define ARCH_TESTS_H
>>  
>> +#include "tests/tests.h"
>> +
>>  struct test_suite;
>>  
>>  /* Tests */
>> @@ -17,6 +19,8 @@ int test__amd_ibs_via_core_pmu(struct test_suite *test, int subtest);
>>  int test__amd_ibs_period(struct test_suite *test, int subtest);
>>  int test__hybrid(struct test_suite *test, int subtest);
>>  
>> +DECLARE_SUITE(x86_topdown);
>> +
>>  extern struct test_suite *arch_tests[];
>>  
>>  #endif
>> diff --git a/tools/perf/arch/x86/tests/Build b/tools/perf/arch/x86/tests/Build
>> index 01d5527f38c7..311b6b53d3d8 100644
>> --- a/tools/perf/arch/x86/tests/Build
>> +++ b/tools/perf/arch/x86/tests/Build
>> @@ -11,6 +11,7 @@ endif
>>  perf-test-$(CONFIG_X86_64) += bp-modify.o
>>  perf-test-y += amd-ibs-via-core-pmu.o
>>  perf-test-y += amd-ibs-period.o
>> +perf-test-y += topdown.o
>>  
>>  ifdef SHELLCHECK
>>    SHELL_TESTS := gen-insn-x86-dat.sh
>> diff --git a/tools/perf/arch/x86/tests/arch-tests.c b/tools/perf/arch/x86/tests/arch-tests.c
>> index bfee2432515b..29ec1861ccef 100644
>> --- a/tools/perf/arch/x86/tests/arch-tests.c
>> +++ b/tools/perf/arch/x86/tests/arch-tests.c
>> @@ -53,5 +53,6 @@ struct test_suite *arch_tests[] = {
>>  	&suite__amd_ibs_via_core_pmu,
>>  	&suite__amd_ibs_period,
>>  	&suite__hybrid,
>> +	&suite__x86_topdown,
>>  	NULL,
>>  };
>> diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
>> new file mode 100644
>> index 000000000000..8d0ea7a4bbc1
>> --- /dev/null
>> +++ b/tools/perf/arch/x86/tests/topdown.c
>> @@ -0,0 +1,76 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#include "arch-tests.h"
>> +#include "../util/topdown.h"
>> +#include "evlist.h"
>> +#include "parse-events.h"
>> +#include "pmu.h"
>> +#include "pmus.h"
>> +
>> +static int event_cb(void *state, struct pmu_event_info *info)
>> +{
>> +	char buf[256];
>> +	struct parse_events_error parse_err;
>> +	int *ret = state, err;
>> +	struct evlist *evlist = evlist__new();
>> +	struct evsel *evsel;
>> +
>> +	if (!evlist)
>> +		return -ENOMEM;
>> +
>> +	parse_events_error__init(&parse_err);
>> +	snprintf(buf, sizeof(buf), "%s/%s/", info->pmu->name, info->name);
>> +	err = parse_events(evlist, buf, &parse_err);
>> +	if (err) {
>> +		parse_events_error__print(&parse_err, buf);
>> +		*ret = TEST_FAIL;
>> +	}
>> +	parse_events_error__exit(&parse_err);
>> +	evlist__for_each_entry(evlist, evsel) {
>> +		bool fail = false;
>> +		bool p_core_pmu = evsel->pmu->type == PERF_TYPE_RAW;
>> +		const char *name = evsel__name(evsel);
>> +
>> +		if (strcasestr(name, "uops_retired.slots") ||
>> +		    strcasestr(name, "topdown.backend_bound_slots") ||
>> +		    strcasestr(name, "topdown.br_mispredict_slots") ||
>> +		    strcasestr(name, "topdown.memory_bound_slots") ||
>> +		    strcasestr(name, "topdown.bad_spec_slots") ||
>> +		    strcasestr(name, "topdown.slots_p")) {
>> +			if (arch_is_topdown_slots(evsel) || arch_is_topdown_metrics(evsel))
>> +				fail = true;
>> +		} else if (strcasestr(name, "slots")) {
>> +			if (arch_is_topdown_slots(evsel) != p_core_pmu ||
>> +			    arch_is_topdown_metrics(evsel))
>> +				fail = true;
>> +		} else if (strcasestr(name, "topdown")) {
>> +			if (arch_is_topdown_slots(evsel) ||
>> +			    arch_is_topdown_metrics(evsel) != p_core_pmu)
>> +				fail = true;
>> +		} else if (arch_is_topdown_slots(evsel) || arch_is_topdown_metrics(evsel)) {
>> +			fail = true;
>> +		}
>> +		if (fail) {
>> +			pr_debug("Broken topdown information for '%s'\n", evsel__name(evsel));
>> +			*ret = TEST_FAIL;
>> +		}
>> +	}
>> +	evlist__delete(evlist);
>> +	return 0;
>> +}
>> +
>> +static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
>> +{
>> +	int ret = TEST_OK;
>> +	struct perf_pmu *pmu = NULL;
>> +
>> +	if (!topdown_sys_has_perf_metrics())
>> +		return TEST_OK;
>> +
>> +	while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
>> +		if (perf_pmu__for_each_event(pmu, /*skip_duplicate_pmus=*/false, &ret, event_cb))
>> +			break;
>> +	}
>> +	return ret;
>> +}
>> +
>> +DEFINE_SUITE("x86 topdown", x86_topdown);
>> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
>> index 3dd29ba2c23b..9bc80fff3aa0 100644
>> --- a/tools/perf/arch/x86/util/evsel.c
>> +++ b/tools/perf/arch/x86/util/evsel.c
>> @@ -23,47 +23,25 @@ void arch_evsel__set_sample_weight(struct evsel *evsel)
>>  bool evsel__sys_has_perf_metrics(const struct evsel *evsel)
>>  {
>>  	struct perf_pmu *pmu;
>> -	u32 type = evsel->core.attr.type;
>>  
>> -	/*
>> -	 * The PERF_TYPE_RAW type is the core PMU type, e.g., "cpu" PMU
>> -	 * on a non-hybrid machine, "cpu_core" PMU on a hybrid machine.
>> -	 * The slots event is only available for the core PMU, which
>> -	 * supports the perf metrics feature.
>> -	 * Checking both the PERF_TYPE_RAW type and the slots event
>> -	 * should be good enough to detect the perf metrics feature.
>> -	 */
>> -again:
>> -	switch (type) {
>> -	case PERF_TYPE_HARDWARE:
>> -	case PERF_TYPE_HW_CACHE:
>> -		type = evsel->core.attr.config >> PERF_PMU_TYPE_SHIFT;
>> -		if (type)
>> -			goto again;
>> -		break;
>> -	case PERF_TYPE_RAW:
>> -		break;
>> -	default:
>> +	if (!topdown_sys_has_perf_metrics())
>>  		return false;
>> -	}
>> -
>> -	pmu = evsel->pmu;
>> -	if (pmu && perf_pmu__is_fake(pmu))
>> -		pmu = NULL;
>>  
>> -	if (!pmu) {
>> -		while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
>> -			if (pmu->type == PERF_TYPE_RAW)
>> -				break;
>> -		}
>> -	}
>> -	return pmu && perf_pmu__have_event(pmu, "slots");
>> +	/*
>> +	 * The PERF_TYPE_RAW type is the core PMU type, e.g., "cpu" PMU on a
>> +	 * non-hybrid machine, "cpu_core" PMU on a hybrid machine.  The
>> +	 * topdown_sys_has_perf_metrics checks the slots event is only available
>> +	 * for the core PMU, which supports the perf metrics feature. Checking
>> +	 * both the PERF_TYPE_RAW type and the slots event should be good enough
>> +	 * to detect the perf metrics feature.
>> +	 */
>> +	pmu = evsel__find_pmu(evsel);
>> +	return pmu && pmu->type == PERF_TYPE_RAW;
>>  }
>>  
>>  bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>>  {
>> -	if (!evsel__sys_has_perf_metrics(evsel) || !evsel->name ||
>> -	    strcasestr(evsel->name, "uops_retired.slots"))
>> +	if (!evsel__sys_has_perf_metrics(evsel))
>>  		return false;
>>  
>>  	return arch_is_topdown_metrics(evsel) || arch_is_topdown_slots(evsel);
>> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
>> index d1c654839049..66b231fbf52e 100644
>> --- a/tools/perf/arch/x86/util/topdown.c
>> +++ b/tools/perf/arch/x86/util/topdown.c
>> @@ -1,6 +1,4 @@
>>  // SPDX-License-Identifier: GPL-2.0
>> -#include "api/fs/fs.h"
>> -#include "util/evsel.h"
>>  #include "util/evlist.h"
>>  #include "util/pmu.h"
>>  #include "util/pmus.h"
>> @@ -8,6 +6,9 @@
>>  #include "topdown.h"
>>  #include "evsel.h"
>>  
>> +// cmask=0, inv=0, pc=0, edge=0, umask=4, event=0
>> +#define TOPDOWN_SLOTS		0x0400
>> +
>>  /* Check whether there is a PMU which supports the perf metrics. */
>>  bool topdown_sys_has_perf_metrics(void)
>>  {
>> @@ -32,31 +33,19 @@ bool topdown_sys_has_perf_metrics(void)
>>  	return has_perf_metrics;
>>  }
>>  
>> -#define TOPDOWN_SLOTS		0x0400
>>  bool arch_is_topdown_slots(const struct evsel *evsel)
>>  {
>> -	if (evsel->core.attr.config == TOPDOWN_SLOTS)
>> -		return true;
>> -
>> -	return false;
>> +	return evsel->core.attr.type == PERF_TYPE_RAW &&
>> +	       evsel->core.attr.config == TOPDOWN_SLOTS &&
>> +	       evsel->core.attr.config1 == 0;
>>  }
>>  
>>  bool arch_is_topdown_metrics(const struct evsel *evsel)
>>  {
>> -	int config = evsel->core.attr.config;
>> -	const char *name_from_config;
>> -	struct perf_pmu *pmu;
>> -
>> -	/* All topdown events have an event code of 0. */
>> -	if ((config & 0xFF) != 0)
>> -		return false;
>> -
>> -	pmu = evsel__find_pmu(evsel);
>> -	if (!pmu || !pmu->is_core)
>> -		return false;
>> -
>> -	name_from_config = perf_pmu__name_from_config(pmu, config);
>> -	return name_from_config && strcasestr(name_from_config, "topdown");
>> +	// cmask=0, inv=0, pc=0, edge=0, umask=0x80-0x87, event=0
>> +	return evsel->core.attr.type == PERF_TYPE_RAW &&
>> +		(evsel->core.attr.config & 0xFFFFF8FF) == 0x8000 &&
>> +		evsel->core.attr.config1 == 0;
>>  }
>>  
>>  /*
>> diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
>> index 1bae9b1822d7..2349536cf882 100644
>> --- a/tools/perf/arch/x86/util/topdown.h
>> +++ b/tools/perf/arch/x86/util/topdown.h
>> @@ -2,6 +2,10 @@
>>  #ifndef _TOPDOWN_H
>>  #define _TOPDOWN_H 1
>>  
>> +#include <stdbool.h>
>> +
>> +struct evsel;
>> +
>>  bool topdown_sys_has_perf_metrics(void);
>>  bool arch_is_topdown_slots(const struct evsel *evsel);
>>  bool arch_is_topdown_metrics(const struct evsel *evsel);
>> -- 
>> 2.50.0.727.gbf7dc18ff4-goog
>>

Re: [PATCH v2 1/2] perf topdown: Use attribute to see an event is a topdown metic or slots

[Namhyung Kim]

Hello,

On Tue, Jul 29, 2025 at 11:21:32AM +0800, Mi, Dapeng wrote:
> 
> On 7/23/2025 9:03 AM, Namhyung Kim wrote:
> > On Fri, Jul 18, 2025 at 06:27:49AM -0700, Ian Rogers wrote:
> >> The string comparisons were overly broad and could fire for the
> >> incorrect PMU and events. Switch to using the config in the attribute
> >> then add a perf test to confirm the attribute config values match
> >> those of parsed events of that name and don't match others. This
> >> exposed matches for slots events that shouldn't have matched as the
> >> slots fixed counter event, such as topdown.slots_p.
> >>
> >> Fixes: fbc798316bef ("perf x86/topdown: Refine helper arch_is_topdown_metrics()")
> >> Signed-off-by: Ian Rogers <irogers@google.com>
> > Dapeng, are you ok with this (or Ian's TMA fix v3 below)?
> >
> > https://lore.kernel.org/r/20250719030517.1990983-1-irogers@google.com
> 
> Sorry for late response (Just back from vacation). The patches look good to
> me. I tested these patches on Sapphire Rapids and Panther Lake, no issue is
> found. Thanks. 

Thanks for testing this!
Namhyung