| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f2f19e27-907c-4af5-88c6-88e02fbb2e15@linaro.org>
Date: Wed, 26 Feb 2025 14:57:26 +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 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@...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.
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.
Powered by blists - more mailing lists