From: Ian Rogers <irogers@google.com>
To: Thomas Falcon <thomas.falcon@intel.com>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Namhyung Kim <namhyung@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>,
Adrian Hunter <adrian.hunter@intel.com>,
"Liang, Kan" <kan.liang@linux.intel.com>,
Ravi Bangoria <ravi.bangoria@amd.com>,
James Clark <james.clark@linaro.org>,
Dapeng Mi <dapeng1.mi@linux.intel.com>,
Weilin Wang <weilin.wang@intel.com>,
Andi Kleen <ak@linux.intel.com>,
linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org
Subject: [PATCH v3 13/15] perf topdown: Use attribute to see an event is a topdown metic or slots
Date: Fri, 18 Jul 2025 20:05:15 -0700 [thread overview]
Message-ID: <20250719030517.1990983-14-irogers@google.com> (raw)
In-Reply-To: <20250719030517.1990983-1-irogers@google.com>
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
next prev parent reply other threads:[~2025-07-19 3:05 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-07-19 3:05 [PATCH v3 00/15] Fixes for Intel TMA, particularly for hybrid Ian Rogers
2025-07-19 3:05 ` [PATCH v3 01/15] perf parse-events: Warn if a cpu term is unsupported by a CPU Ian Rogers
2025-07-19 3:05 ` [PATCH v3 02/15] perf stat: Avoid buffer overflow to the aggregation map Ian Rogers
2025-07-19 3:05 ` [PATCH v3 03/15] perf stat: Don't size aggregation ids from user_requested_cpus Ian Rogers
2025-07-19 3:05 ` [PATCH v3 04/15] perf parse-events: Allow the cpu term to be a PMU or CPU range Ian Rogers
2025-07-19 3:05 ` [PATCH v3 05/15] perf tool_pmu: Allow num_cpus(_online) to be specific to a cpumask Ian Rogers
2025-07-19 3:05 ` [PATCH v3 06/15] libperf evsel: Rename own_cpus to pmu_cpus Ian Rogers
2025-07-19 3:05 ` [PATCH v3 07/15] libperf evsel: Factor perf_evsel__exit out of perf_evsel__delete Ian Rogers
2025-07-19 3:05 ` [PATCH v3 08/15] perf evsel: Use libperf perf_evsel__exit Ian Rogers
2025-07-19 3:05 ` [PATCH v3 09/15] perf pmus: Factor perf_pmus__find_by_attr out of evsel__find_pmu Ian Rogers
2025-07-19 3:05 ` [PATCH v3 10/15] perf parse-events: Minor __add_event refactoring Ian Rogers
2025-07-19 3:05 ` [PATCH v3 11/15] perf evsel: Add evsel__open_per_cpu_and_thread Ian Rogers
2025-07-19 3:05 ` [PATCH v3 12/15] perf parse-events: Support user CPUs mixed with threads/processes Ian Rogers
2025-07-24 15:12 ` Ian Rogers
2025-07-19 3:05 ` Ian Rogers [this message]
2025-07-19 3:05 ` [PATCH v3 14/15] perf parse-events: Fix missing slots for Intel topdown metric events Ian Rogers
2025-07-19 3:05 ` [PATCH v3 15/15] perf metricgroups: Add NO_THRESHOLD_AND_NMI constraint Ian Rogers
2025-07-25 18:48 ` [PATCH v3 00/15] Fixes for Intel TMA, particularly for hybrid Namhyung Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250719030517.1990983-14-irogers@google.com \
--to=irogers@google.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=dapeng1.mi@linux.intel.com \
--cc=james.clark@linaro.org \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=ravi.bangoria@amd.com \
--cc=thomas.falcon@intel.com \
--cc=weilin.wang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®