Re: [PATCH 00/20] perf vendors events arm64: Multiple Arm CPUs

[John Garry]

On 15/05/2022 23:03, Ian Rogers wrote:
> I'll raise John's "ok" and say this looks great!:-D  Some thoughts:
>
> The mapfile.csv cpuid values don't directly align with:
> https://github.com/ARM-software/data/blob/master/cpus.json
> but this definitely looks deliberate.

Hi Ian,

> The new events lack the PMU "Unit" value.

For arm support we work on the basis that no "Unit" means CPU PMU. I 
assume the same for other archs, but maybe this hybrid PMU support 
changes that.

> The current perf json is
> pretty free form and leads to problems if two PMUs are present.

Can you clarify - for my benefit - exactly what you mean by "two PMUs 
are present"?

> Context is here:
> https://lore.kernel.org/lkml/CAP-5=fWRRZsyJZ-gky-FOFz79zW_3r78d_0APpj5sf66HqTpLw@xxxxxxxxxxxxxx/

We have another problem but I am not sure if exactly the same.

The issue is that if we have an event alias "cycles" for an uncore PMU, 
then if we use "stat" command then perf tool matches "cycles" to CPU 
cycles and not the uncore PMU, which we would not want.

We have ways to work around it, though.

> My idea to rationalize this is to mirror what is already done in
> sysfs, that is the event data is specific to a PMU. As a lot of "Unit"
> values are missing from events on x86 a reasonable guess if the "Unit"
> is missing is to use "cpu". 

This sounds like what I mentioned in the reply to 1/20:

"I had a patch series which makes perf read the armv8 pmu
sysfs event file to learn all the events which the core supports and
create the aliases from that. So, in this, we don't require the JSONs to
list these events explicitly. "

Is this like what Andi was talking about in terms of runtime loading?

> Poking a Google Pixel 4a, I see that all
> PMU data is in "armv8_pmuv3". So for ARM I could guess this is always
> the case, ie all events should belong to armv8_pmuv3. This may not be
> right and could lead to confusion like an event BR_COND_MIS_PRED
> having an alias of "armv8_pmuv3/BR_COND_MIS_PRED/" but it really
> should have some other PMU name in there. I just raise this in case
> there is a fix for this we could incorporate into this patch series,
> maybe "armv8_pmuv3" is always the PMU and my life is easy.

Thanks,
John