| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAP-5=fVzJHm94T5c2CCPwzLzUweuQuzgppdECNoW4Ju-47m2LQ@mail.gmail.com>
Date: Fri, 18 Jul 2025 06:15:32 -0700
From: Ian Rogers <irogers@...gle.com>
To: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
Cc: Thomas Falcon <thomas.falcon@...el.com>, Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>, "Liang, Kan" <kan.liang@...ux.intel.com>,
Ravi Bangoria <ravi.bangoria@....com>, James Clark <james.clark@...aro.org>,
Weilin Wang <weilin.wang@...el.com>, Andi Kleen <ak@...ux.intel.com>, linux-kernel@...r.kernel.org,
linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v1 1/2] perf topdown: Use attribute to see an event is a
topdown metic or slots
On Fri, Jul 18, 2025 at 1:51 AM Mi, Dapeng <dapeng1.mi@...ux.intel.com> wrote:
>
>
> On 7/18/2025 3:17 PM, 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@...gle.com>
> > ---
> > 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..ba2c163945d8
> > --- /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 topdown_pmu = evsel->pmu->type == PERF_TYPE_RAW;
I think this should be called p_core_pmu as e_cores can have topdown
events, we just don't need to fix their grouping. I'll send a v2 for
this.
> > + 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) != topdown_pmu ||
> > + arch_is_topdown_metrics(evsel))
> > + fail = true;
> > + } else if (strcasestr(name, "topdown")) {
> > + if (arch_is_topdown_slots(evsel) ||
> > + arch_is_topdown_metrics(evsel) != topdown_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;
>
> Do I miss something? But it seems not check if the PMU has slots event here.
topdown_sys_has_perf_metrics checks this and caches the result of
true/false (I also tweaked the comment to mention this).
Thinking about this I spotted the issue above.
Thanks,
Ian
>
> > }
> >
> > 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);
Powered by blists - more mailing lists