Re: [PATCH 0/2] perf vendor events arm64: Add A720/A520 events/metrics

[James Clark <james.clark@linaro.org>]

On 20/02/2025 3:33 am, Yangyu Chen wrote:
> 
> 
>> On 19 Feb 2025, at 06:33, Ian Rogers <irogers@google.com> wrote:
>>
>> On Tue, Feb 18, 2025 at 2:19 PM Namhyung Kim <namhyung@kernel.org <mailto:namhyung@kernel.org>> wrote:
>>>
>>> On Tue, Feb 18, 2025 at 09:30:23AM +0000, James Clark wrote:
>>>>
>>>>
>>>> On 18/02/2025 12:41 am, Ian Rogers wrote:
>>>>> On Fri, Feb 14, 2025 at 2:02 AM James Clark <james.clark@linaro.org> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 14/02/2025 5:49 am, Yangyu Chen wrote:
>>>>>>>
>>>>>>>
>>>>>>>> On 14 Feb 2025, at 09:12, Namhyung Kim <namhyung@kernel.org> wrote:
>>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> On Thu, Feb 13, 2025 at 11:11:01PM +0800, Yangyu Chen wrote:
>>>>>>>>> This patchset adds the perf JSON files for the Cortex-A720 and Cortex-A520
>>>>>>>>> processors. Some events have been tested on Raxda Orion 6 with Cix P1 SoC
>>>>>>>>> (8xA720 + 4xA520) running mainline Kernel with ACPI mode.
>>>>>>>>
>>>>>>>> I'm curious how the name of PMUs look like.  It is cortex_a720 (or a520)?
>>>>>>>
>>>>>>> The name of PMUs comes from Arm's documentation. I have included these
>>>>>>> links in each patch.
>>>>>>>
>>>>>>>> I remember there's a logic to check the length of hex digits at the end.
>>>>>>>>
>>>>>>>
>>>>>>> Could you provide more details about this?
>>>>>>>
>>>>>>>> Ian, are you ok with this?
>>>>>>>>
>>>>>>
>>>>>> I think they wouldn't be merged because they're core PMUs, so should be
>>>>>> fine? Even though they would otherwise be merged because they're more
>>>>>> than 3 hex digits.
>>>>>
>>>>> Do we know the PMU names? If they are cortex_a520 and cortex_a720 then
>>>>
>>>> It will be "armv9_cortex_a720" from this line:
>>>>
>>>>   PMUV3_INIT_SIMPLE(armv9_cortex_a720)
>>>
>>> I see, thanks!
>>>
>>>>
>>>>> this comment at least reads a little stale:
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmus.c?h=perf-tools-next#n76
>>>>> ```
>>>>> /*
>>>>> * There is a '_{num}' suffix. For decimal suffixes any length
>>>>> * will do, for hexadecimal ensure more than 2 hex digits so
>>>>> * that S390's cpum_cf PMU doesn't match.
>>>>> */
>>>>> ```
>>>>> James is right that core PMUs aren't put on the same list as uncore/other PMUs.
>>>
>>> Ok, then I guess we're good.
>>
>> I think you may be able to do things that look odd, like today the
>> "i915" PMU can be called just "i", I think the a520/a720 naming will
>> allow "armv9_cortex/cycles/" as an event name, then open it on two
>> PMUs if they are present. We may only show one PMU in perf list as
>> that code I think assumes they're the same PMU as they only differ by
>> suffix:
>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmus.c?h=perf-tools-next#n384
>> I can imagine aggregation possibly being broken, but I think that
>> works off the number of PMUs not the names of the PMUs, so it should
>> be okay. Probably the only thing broken that matter is perf list when
>> you have a BIG.little system with a520 and a720, this may be broken
>> with say a a53 and a72 today as both of those suffix lengths are >2,
>> but maybe they use the "armv8._pmuv3_0", "armv8._pmuv3_1", etc. naming
>> convention. I suspect the >2 here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmus.c?h=perf-tools-next#n80
>> would still work and be correct if it were >4. If that changes then
>> this will also need to change:
>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/Documentation/ABI/testing/sysfs-bus-event_source-devices?h=perf-tools-next#n12
>>
>> Thanks,
>> Ian
>>
> 
> On my system, the names of PMUs are `armv8_pmuv3_0` and
> `armv8_pmuv3_1`:
> 
> ```
> $ ls /sys/bus/event_source/devices/
> armv8_pmuv3_0  armv8_pmuv3_1  breakpoint  kprobe  software  tracepoint  uprobe
> ```
> 
> I searched for ACPI DSDT on my platform, but there's no mention of
> a720 or a520. I haven't delved into the PMU kernel driver yet.

Ah yeah, with ACPI you get those names instead.

> 
> Additionally, there's a more significant problem for aarch64
> BIG.little platforms when two or more types of cores don't have the
> same PMUs. The perf list can only display the core PMUs on core0
> unless we use the PERF_CPUID env to override it. This is because
> perf will only probe the first MIDR here:
> https://github.com/torvalds/linux/blob/87a132e73910e8689902aed7f2fc229d6908383b/tools/perf/arch/arm64/util/header.c#L60
> 
> However, I think this doesn't block this patch for adding events and metrics?
> 
> 
> Thanks,
> Yangyu Chen
> 

I don't think that's an issue because events are listed per PMU rather 
than per CPU and that MIDR function does take a CPU struct. From my 
testing the only thing stopping all PMUs from being listed was the 
numeric suffix de-duplication.

Either way, no, it shouldn't affect your patch. But I'm also looking 
into Ian's suggestion to improve it anyway.

>>>
>>> Thanks,
>>> Namhyung
> 
>