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

[James Clark <james.clark@linaro.org>]

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.