linux-kernel - Re: [PATCH] perf stat: Fix non-uniquified hybrid legacy events

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fWywDB40-RgV8LaPqsoffOLdDcYkUB_LHoPvV=R8yas4w@mail.gmail.com>
Date: Wed, 12 Feb 2025 09:48:05 -0800
From: Ian Rogers <irogers@...gle.com>
To: James Clark <james.clark@...aro.org>
Cc: linux-perf-users@...r.kernel.org, Robin.Murphy@....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>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf stat: Fix non-uniquified hybrid legacy events

On Wed, Feb 12, 2025 at 4:24 AM James Clark <james.clark@...aro.org> wrote:
>
> Legacy hybrid events have attr.type == PERF_TYPE_HARDWARE, so they look
> like plain legacy events if we only look at attr.type. But legacy events
> should still be uniquified if they were opened on a non-legacy PMU.
> Previously we looked at the PMU type to determine legacy vs hybrid
> events here so revert this particular check to how it was before the
> linked fixes commit.
>
> counter->pmu doesn't need to be null checked twice, in fact it is
> required for any kind of uniquification so make that a separate check.
>
> This restores PMU names on hybrid systems and also changes "perf stat
> metrics (shadow stat) test" from a FAIL back to a SKIP (on hybrid). The
> test was gated on "cycles" appearing alone which doesn't happen on
> here.
>
> Before:
>
>   $ perf stat -- true
>   ...
>      <not counted>      instructions:u                           (0.00%)
>            162,536      instructions:u            # 0.58  insn per cycle
>   ...
>
> After:
>
>  $ perf stat -- true
>  ...
>      <not counted>      cpu_atom/instructions/u                  (0.00%)
>            162,541      cpu_core/instructions/u   # 0.62  insn per cycle
>  ...
>
> Fixes: 357b965deba9 ("perf stat: Changes to event name uniquification")
> Signed-off-by: James Clark <james.clark@...aro.org>
> ---
>  tools/perf/util/stat-display.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index e65c7e9f15d1..eae34ba95f59 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -1688,12 +1688,17 @@ static void evsel__set_needs_uniquify(struct evsel *counter, const struct perf_s
>                 return;
>         }
>
> -       if  (counter->core.attr.type < PERF_TYPE_MAX && counter->core.attr.type != PERF_TYPE_RAW) {
> +       if (!counter->pmu) {

Thanks James, I wish I had a hybrid laptop so I didn't keep breaking
things like this. I'm uncomfortable using an evsel having/not-having a
PMU as an indication of whether uniquification is necessary. It is
kind of a side-effect of parsing whether the PMU variable is non-NULL,
it'd kind of be nice to stop things using `evsel->pmu` directly and
switch them to `evsel__find_pmu(evsel)`, in the future maybe legacy
events will get the core PMU, a tracepoint PMU, etc. so we'd never
expect this variable to be NULL.

Your commit message gives me enough to think about what the issue is,
so let me give it some thought.

Thanks!
Ian

> +               /* evsel__uniquify_counter() uses counter->pmu for the name */
> +               return;
> +       }
> +
> +       if (counter->pmu->type < PERF_TYPE_MAX && counter->pmu->type != PERF_TYPE_RAW) {
>                 /* Legacy event, don't uniquify. */
>                 return;
>         }
>
> -       if (counter->pmu && counter->pmu->is_core &&
> +       if (counter->pmu->is_core &&
>             counter->alternate_hw_config != PERF_COUNT_HW_MAX) {
>                 /* A sysfs or json event replacing a legacy event, don't uniquify. */
>                 return;
> --
> 2.34.1
>