linux-kernel - Re: [PATCH v3 3/5] perf stat: Remove evlist__add_default

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Thu, 30 May 2024 13:22:25 +0100
From: James Clark <james.clark@....com>
To: Ian Rogers <irogers@...gle.com>
Cc: 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>,
 Kan Liang <kan.liang@...ux.intel.com>, Ravi Bangoria
 <ravi.bangoria@....com>, Yang Jihong <yangjihong@...edance.com>,
 Ze Gao <zegao2021@...il.com>, Leo Yan <leo.yan@...ux.dev>,
 Song Liu <song@...nel.org>, K Prateek Nayak <kprateek.nayak@....com>,
 Kaige Ye <ye@...ge.org>, Yicong Yang <yangyicong@...ilicon.com>,
 linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/5] perf stat: Remove evlist__add_default_attrs use
 strings



On 29/05/2024 19:18, Ian Rogers wrote:
> On Wed, May 29, 2024 at 10:39 AM Ian Rogers <irogers@...gle.com> wrote:
>>
>> On Wed, May 29, 2024 at 8:39 AM James Clark <james.clark@....com> wrote:
>>>
>>>
>>>
>>> On 10/05/2024 06:37, Ian Rogers wrote:
>>>> add_default_atttributes would add evsels by having pre-created
>>>> perf_event_attr, however, this needed fixing for hybrid as the
>>>> extended PMU type was necessary for each core PMU. The logic for this
>>>> was in an arch specific x86 function and wasn't present for ARM,
>>>> meaning that default events weren't being opened on all PMUs on
>>>> ARM. Change the creation of the default events to use parse_events and
>>>> strings as that will open the events on all PMUs.
>>>>
>>>> Rather than try to detect events on PMUs before parsing, parse the
>>>> event but skip its output in stat-display.
>>>>
>>>> The previous order of hardware events was: cycles,
>>>> stalled-cycles-frontend, stalled-cycles-backend, instructions. As
>>>> instructions is a more fundamental concept the order is changed to:
>>>> instructions, cycles, stalled-cycles-frontend, stalled-cycles-backend.
>>>>
>>>> Closes: https://lore.kernel.org/lkml/CAP-5=fVABSBZnsmtRn1uF-k-G1GWM-L5SgiinhPTfHbQsKXb_g@mail.gmail.com/
>>>
>>> Taking a look at this one now. I think some example commands and outputs
>>> in the commit message would be helpful because there are quite a few
>>> different things mentioned in the closes link.
>>>
>>> But I'm assuming this is just for the command without specifying an event:
>>>
>>>   $ perf stat
>>>
>>> I didn't realise that wasn't working properly and I'd missed that Mark
>>> spotted it in that link.
>>
>> Hi James,
>>
>> Do you want to take on owning fixing this? My concern is that by using
>> event parsing:
>>
>> +                               "context-switches,"
>> +                               "cpu-migrations,"
>> +                               "page-faults,"
>> +                               "instructions,"
>> +                               "cycles,"
>> +                               "stalled-cycles-frontend,"
>> +                               "stalled-cycles-backend,"
>> +                               "branches,"
>> +                               "branch-misses",
>>
>> any of the names could conflict with something advertised on an ARM
>> PMU somewhere. Clearly cycles has already proved to be controversial
>> and broken for perf record on Neoverse.
> 
> Oh and these events are also broken on ARM M? PMUs with the revert
> that's in v6.10, so I don't know how to go about fixing this.
> 
> Thanks,
> Ian

I'll take it yep. But the fix probably needs to take into context
whatever else we decide with the other issue. Conflicting names might
not actually be an issue if we work around that with the other change.
But I'm still getting up to speed with the other one.

James