Message-ID: <c672c3a3-64e3-495a-ae61-ae098d30c6b9@linaro.org>
Date: Thu, 13 Feb 2025 12:15:30 +0000
From: James Clark <james.clark@...aro.org>
To: 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>,
 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 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.

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

Powered by blists - more mailing lists