perf topdown: Use attribute to see an event is a topdown metic or slots

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>
Link: https://lore.kernel.org/r/20250719030517.1990983-14-irogers@google.com
Signed-off-by: Namhyung Kim <namhyung@kernel.org>

diff --git a/tools/perf/arch/x86/include/arch-tests.h b/tools/perf/arch/x86/include/arch-tests.h
index 4fd425157d7d4f5c4582e87e197bf1899861afb3..8713e9122d4c66c7c366be04153dba81b32c1837 100644 (file)
--- 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 01d5527f38c788c689974a2dc5497d1057f1e690..311b6b53d3d84d8db98ed7d7056226ec8079e97a 100644 (file)
--- 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 bfee2432515b1709086ed4a5050d02ad0d3f1e9c..29ec1861ccefd3b9e8fb37a58efd8cc451a5e39c 100644 (file)
--- 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 (file)
index 0000000..8d0ea7a
--- /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 3dd29ba2c23b06b70c7fe96c701965b65b24a78c..9bc80fff3aa0e26294b071423c59a3aeb15f4d22 100644 (file)
--- 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 d1c654839049696ec21730f558878ac018d68c31..66b231fbf52efa7952b51d16143074bc3b017578 100644 (file)
--- 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 1bae9b1822d71b8d6c69aa3fe81aab3444238c97..2349536cf882586a44df4875cf3021ffcf2423bf 100644 (file)
--- 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);