Re: [PATCH v9 46/48] perf jevents: Add collection of topdown like metrics for arm64

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

On 02/12/2025 5:50 pm, Ian Rogers wrote:
> Metrics are created using legacy, common and recommended events. As
> events may be missing a TryEvent function will give None if an event
> is missing. To workaround missing JSON events for cortex-a53, sysfs
> encodings are used.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> An earlier review of this patch by Leo Yan is here:
> https://lore.kernel.org/lkml/8168c713-005c-4fd9-a928-66763dab746a@arm.com/
> Hopefully all corrections were made.
> ---
>   tools/perf/pmu-events/arm64_metrics.py | 145 ++++++++++++++++++++++++-
>   1 file changed, 142 insertions(+), 3 deletions(-)
> 
[...]
> +        MetricGroup("lpm_topdown_be_bound", [
> +            MetricGroup("lpm_topdown_be_dtlb", [
> +                Metric("lpm_topdown_be_dtlb_walks", "Dtlb walks per instruction",
> +                       d_ratio(dtlb_walk, ins_ret), "walk/insn"),
> +                Metric("lpm_topdown_be_dtlb_walk_rate", "Dtlb walks per L1D TLB access",
> +                       d_ratio(dtlb_walk, l1d_tlb) if l1d_tlb else None, "100%"),
> +            ]) if dtlb_walk else None,
> +            MetricGroup("lpm_topdown_be_mix", [
> +                Metric("lpm_topdown_be_mix_ld", "Percentage of load instructions",
> +                       d_ratio(ld_spec, inst_spec), "100%") if ld_spec else None,
> +                Metric("lpm_topdown_be_mix_st", "Percentage of store instructions",
> +                       d_ratio(st_spec, inst_spec), "100%") if st_spec else None,
> +                Metric("lpm_topdown_be_mix_simd", "Percentage of SIMD instructions",
> +                       d_ratio(ase_spec, inst_spec), "100%") if ase_spec else None,
> +                Metric("lpm_topdown_be_mix_fp",
> +                       "Percentage of floating point instructions",
> +                       d_ratio(vfp_spec, inst_spec), "100%") if vfp_spec else None,
> +                Metric("lpm_topdown_be_mix_dp",
> +                       "Percentage of data processing instructions",
> +                       d_ratio(dp_spec, inst_spec), "100%") if dp_spec else None,
> +                Metric("lpm_topdown_be_mix_crypto",
> +                       "Percentage of data processing instructions",
> +                       d_ratio(crypto_spec, inst_spec), "100%") if crypto_spec else None,
> +                Metric(
> +                    "lpm_topdown_be_mix_br", "Percentage of branch instructions",
> +                    d_ratio(br_immed_spec + br_indirect_spec + br_ret_spec,
> +                            inst_spec), "100%") if br_immed_spec and br_indirect_spec and br_ret_spec else None,

Hi Ian,

I've been trying to engage with the team that's publishing the metrics 
in Arm [1] to see if there was any chance in getting some unity between 
these new metrics and their existing json ones. The feedback from them 
was that the decision to only publish metrics for certain cores is 
deliberate and there is no plan to change anything. The metrics there 
are well tested, known to be working, and usually contain workarounds 
for specific issues. They don't want to do "Arm wide" common metrics for 
existing cores as they believe it has more potential to mislead people 
than help.

I'm commenting on this "lpm_topdown_be_mix_br" as one example, that the 
equivalent Arm metric "branch_percentage" excludes br_ret_spec because 
br_indirect_spec also counts returns. Or on neoverse-n3 it's 
"PC_WRITE_SPEC / INST_SPEC".

I see that you've prefixed all the metrics so the names won't clash from 
Kan's feedback [2]. But it makes me wonder if at some point some kind of 
alias list could be implemented to override the generated metrics with 
hand written json ones. But by that point why not just use the same 
names? The Arm metric team's feedback was that there isn't really an 
industry standard for naming, and that differences between architectures 
would make it almost impossible to standardise anyway in their opinion. 
But here we're adding duplicate metrics with different names, where the 
new ones are known to have issues. It's not a great user experience IMO, 
but at the same time missing old cores from the Arm metrics isn't a 
great user experience either. I actually don't have a solution, other 
than to say I tried to get them to consider more unified naming.

I also have to say that I do still agree with Andi's old feedback [3] 
that the existing json was good enough, and maybe this isn't the right 
direction, although it's not very useful feedback at this point. I 
thought I had replied to that thread long ago, but must not have pressed 
send, sorry about that.

[1]: 
https://gitlab.arm.com/telemetry-solution/telemetry-solution/-/tree/main/data
[2]: 
https://lore.kernel.org/lkml/43548903-b7c8-47c4-b1da-0258293ecbd4@linux.intel.com
[3]: https://lore.kernel.org/lkml/ZeJJyCmXO9GxpDiF@tassilo/