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

[PATCH] 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.
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@linaro.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) {
+		/* 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

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

[Ian Rogers]

On Wed, Feb 12, 2025 at 4:24 AM James Clark <james.clark@linaro.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@linaro.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
>

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

[Ian Rogers]

On Wed, Feb 12, 2025 at 9:48 AM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Feb 12, 2025 at 4:24 AM James Clark <james.clark@linaro.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@linaro.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.

I wonder we should just hoist the hybrid test earlier:
```
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 per
f_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 per
f_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.

```

The hybrid test is unfortunately expensive at it needs to search for
>1 core PMU, which means loading all sysfs PMUs. I think we're already
paying the cost though.

Could you check this works James?

Thanks,
Ian

> 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
> >

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

[James Clark]

On 12/02/2025 9:38 pm, Ian Rogers wrote:
> On Wed, Feb 12, 2025 at 9:48 AM Ian Rogers <irogers@google.com> wrote:
>>
>> On Wed, Feb 12, 2025 at 4:24 AM James Clark <james.clark@linaro.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@linaro.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.

As it stands evsel__uniquify_counter() unconditionally dereferences 
evsel->pmu which is why I thought it made sense to check that first. But 
if that might change then fair enough.

>>
>> Your commit message gives me enough to think about what the issue is,
>> so let me give it some thought.
> 
> I wonder we should just hoist the hybrid test earlier:
> ```
> 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 per
> f_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 per
> f_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.
> 
> ```
> 
> The hybrid test is unfortunately expensive at it needs to search for
>> 1 core PMU, which means loading all sysfs PMUs. I think we're already
> paying the cost though.
> 
> Could you check this works James?
> 
> Thanks,
> Ian
> 

Yep that works too.

>> 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
>>>

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

[Namhyung Kim]

Hello,

On Thu, Feb 13, 2025 at 12:15:30PM +0000, James Clark wrote:
> 
> 
> On 12/02/2025 9:38 pm, Ian Rogers wrote:
> > On Wed, Feb 12, 2025 at 9:48 AM Ian Rogers <irogers@google.com> wrote:
> > > 
> > > On Wed, Feb 12, 2025 at 4:24 AM James Clark <james.clark@linaro.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@linaro.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.
> 
> As it stands evsel__uniquify_counter() unconditionally dereferences
> evsel->pmu which is why I thought it made sense to check that first. But if
> that might change then fair enough.
> 
> > > 
> > > Your commit message gives me enough to think about what the issue is,
> > > so let me give it some thought.
> > 
> > I wonder we should just hoist the hybrid test earlier:
> > ```
> > 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 per
> > f_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 per
> > f_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.
> > 
> > ```
> > 
> > The hybrid test is unfortunately expensive at it needs to search for
> > > 1 core PMU, which means loading all sysfs PMUs. I think we're already
> > paying the cost though.
> > 
> > Could you check this works James?
> > 
> > Thanks,
> > Ian
> > 
> 
> Yep that works too.

James, can I take it as your Tested-by?

Ian, can you please send a formal patch with that?

Thanks,
Namhyung

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

[James Clark]

On 14/02/2025 1:27 am, Namhyung Kim wrote:
> Hello,
> 
> On Thu, Feb 13, 2025 at 12:15:30PM +0000, James Clark wrote:
>>
>>
>> On 12/02/2025 9:38 pm, Ian Rogers wrote:
>>> On Wed, Feb 12, 2025 at 9:48 AM Ian Rogers <irogers@google.com> wrote:
>>>>
>>>> On Wed, Feb 12, 2025 at 4:24 AM James Clark <james.clark@linaro.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@linaro.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.
>>
>> As it stands evsel__uniquify_counter() unconditionally dereferences
>> evsel->pmu which is why I thought it made sense to check that first. But if
>> that might change then fair enough.
>>
>>>>
>>>> Your commit message gives me enough to think about what the issue is,
>>>> so let me give it some thought.
>>>
>>> I wonder we should just hoist the hybrid test earlier:
>>> ```
>>> 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 per
>>> f_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 per
>>> f_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.
>>>
>>> ```
>>>
>>> The hybrid test is unfortunately expensive at it needs to search for
>>>> 1 core PMU, which means loading all sysfs PMUs. I think we're already
>>> paying the cost though.
>>>
>>> Could you check this works James?
>>>
>>> Thanks,
>>> Ian
>>>
>>
>> Yep that works too.
> 
> James, can I take it as your Tested-by?
> 

Yep sure

> Ian, can you please send a formal patch with that?
> 
> Thanks,
> Namhyung
> 

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

[James Clark]

On 14/02/2025 10:45 am, James Clark wrote:
> 
> 
> On 14/02/2025 1:27 am, Namhyung Kim wrote:
>> Hello,
>>
>> On Thu, Feb 13, 2025 at 12:15:30PM +0000, James Clark wrote:
>>>
>>>
>>> On 12/02/2025 9:38 pm, Ian Rogers wrote:
>>>> On Wed, Feb 12, 2025 at 9:48 AM Ian Rogers <irogers@google.com> wrote:
>>>>>
>>>>> On Wed, Feb 12, 2025 at 4:24 AM James Clark 
>>>>> <james.clark@linaro.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@linaro.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.
>>>
>>> As it stands evsel__uniquify_counter() unconditionally dereferences
>>> evsel->pmu which is why I thought it made sense to check that first. 
>>> But if
>>> that might change then fair enough.
>>>
>>>>>
>>>>> Your commit message gives me enough to think about what the issue is,
>>>>> so let me give it some thought.
>>>>
>>>> I wonder we should just hoist the hybrid test earlier:
>>>> ```
>>>> 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 per
>>>> f_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 per
>>>> f_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.
>>>>
>>>> ```
>>>>
>>>> The hybrid test is unfortunately expensive at it needs to search for
>>>>> 1 core PMU, which means loading all sysfs PMUs. I think we're already
>>>> paying the cost though.

Yeah perf_pmus__num_core_pmus() has already been called by this point, 
even if doing something like opening the event on a single PMU so I 
think it's fine.

>>>>
>>>> Could you check this works James?
>>>>
>>>> Thanks,
>>>> Ian
>>>>
>>>
>>> Yep that works too.
>>
>> James, can I take it as your Tested-by?
>>
> 
> Yep sure
> 
>> Ian, can you please send a formal patch with that?
>>
>> Thanks,
>> Namhyung
>>
> 

I just noticed that there wasn't another patch so I sent the V2.