Message-ID: <6c9d8c3a-d5f7-4b88-9dda-74b9b3d4484c@linaro.org>
Date: Fri, 14 Feb 2025 10:45:08 +0000
From: James Clark <james.clark@...aro.org>
To: Namhyung Kim <namhyung@...nel.org>, Ian Rogers <irogers@...gle.com>
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>,
 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 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@...gle.com> wrote:
>>>>
>>>> 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.
>>
>> 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
> 

Powered by blists - more mailing lists