Re: [PATCH v3 3/5] perf stat: Remove evlist__add_default_attrs use strings

[James Clark]

On 29/05/2024 19:18, Ian Rogers wrote:
> On Wed, May 29, 2024 at 10:39 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>>
>> On Wed, May 29, 2024 at 8:39 AM James Clark <james.clark@xxxxxxx> 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@xxxxxxxxxxxxxx/
>>>
>>> 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