[PATCH v2] perf stat: Fix non-uniquified hybrid legacy events

[PATCH v2] perf stat: Fix non-uniquified hybrid legacy events

[James Clark]

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. Fix
it by checking if the evsel is hybrid and forcing needs_uniquify
before looking at the attr.type.

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")
Suggested-by: Ian Rogers <irogers@google.com>
Signed-off-by: James Clark <james.clark@linaro.org>
---

Changes since V1:
 * Move the existing evsel__is_hybrid() test earlier instead of looking
   at the PMU of the evsel. Looking at the PMU creates a dependency that
   the PMU is assigned which may prevent refactors in the future.

 tools/perf/util/stat-display.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index e65c7e9f15d1..e852ac0d9847 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -1688,6 +1688,12 @@ static void evsel__set_needs_uniquify(struct evsel *counter, const struct perf_s
 		return;
 	}
 
+	if (!config->hybrid_merge && evsel__is_hybrid(counter)) {
+		/* Unique hybrid counters necessary. */
+		counter->needs_uniquify = true;
+		return;
+	}
+
 	if  (counter->core.attr.type < PERF_TYPE_MAX && counter->core.attr.type != PERF_TYPE_RAW) {
 		/* Legacy event, don't uniquify. */
 		return;
@@ -1705,12 +1711,6 @@ static void evsel__set_needs_uniquify(struct evsel *counter, const struct perf_s
 		return;
 	}
 
-	if (!config->hybrid_merge && evsel__is_hybrid(counter)) {
-		/* Unique hybrid counters necessary. */
-		counter->needs_uniquify = true;
-		return;
-	}
-
 	/*
 	 * Do other non-merged events in the evlist have the same name? If so
 	 * uniquify is necessary.
-- 
2.34.1

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

[Falcon, Thomas]

On Wed, 2025-02-26 at 14:55 +0000, James Clark 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.
> Fix
> it by checking if the evsel is hybrid and forcing needs_uniquify
> before looking at the attr.type.
> 
> 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")
> Suggested-by: Ian Rogers <irogers@google.com>
> Signed-off-by: James Clark <james.clark@linaro.org>

Works for me on a i9-12900. Thanks!

Tested-by: Thomas Falcon <thomas.falcon@intel.com>

> ---
> 
> Changes since V1:
>  * Move the existing evsel__is_hybrid() test earlier instead of
> looking
>    at the PMU of the evsel. Looking at the PMU creates a dependency
> that
>    the PMU is assigned which may prevent refactors in the future.
> 
>  tools/perf/util/stat-display.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-
> display.c
> index e65c7e9f15d1..e852ac0d9847 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -1688,6 +1688,12 @@ static void evsel__set_needs_uniquify(struct
> evsel *counter, const struct perf_s
>  		return;
>  	}
>  
> +	if (!config->hybrid_merge && evsel__is_hybrid(counter)) {
> +		/* Unique hybrid counters necessary. */
> +		counter->needs_uniquify = true;
> +		return;
> +	}
> +
>  	if  (counter->core.attr.type < PERF_TYPE_MAX && counter-
> >core.attr.type != PERF_TYPE_RAW) {
>  		/* Legacy event, don't uniquify. */
>  		return;
> @@ -1705,12 +1711,6 @@ static void evsel__set_needs_uniquify(struct
> evsel *counter, const struct perf_s
>  		return;
>  	}
>  
> -	if (!config->hybrid_merge && evsel__is_hybrid(counter)) {
> -		/* Unique hybrid counters necessary. */
> -		counter->needs_uniquify = true;
> -		return;
> -	}
> -
>  	/*
>  	 * Do other non-merged events in the evlist have the same
> name? If so
>  	 * uniquify is necessary.

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

[Namhyung Kim]

On Wed, 26 Feb 2025 14:55:25 +0000, James Clark 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. Fix
> it by checking if the evsel is hybrid and forcing needs_uniquify
> before looking at the attr.type.
> 
> 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.
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung