Re: [RFC PATCH 1/1] perf arm64: Implement --topdown with metrics

[Andrew Kilroy <andrew.kilroy@arm.com>]

Hi Andi,


On 21/12/2021 14:03, Andi Kleen wrote:
> 
> On 12/20/2021 9:21 AM, Andrew Kilroy wrote:
>>
>> On 15/12/2021 10:52, John Garry wrote:
>>> Hi Andrew,
>>>
>>>>>   const struct pmu_event *metricgroup__find_metric(const char *metric,
>>>>>                                                   const struct 
>>>>> pmu_events_map *map);
>>>>>   int metricgroup__parse_groups_test(struct evlist *evlist,
>>>>> diff --git a/tools/perf/util/topdown.c b/tools/perf/util/topdown.c
>>>>> index 1081b20f9891..57c0c5f2c6bd 100644
>>>>> --- a/tools/perf/util/topdown.c
>>>>> +++ b/tools/perf/util/topdown.c
>>>>> @@ -56,3 +56,9 @@ __weak bool arch_topdown_sample_read(struct evsel 
>>>>> *leader __maybe_unused)
>>>>>   {
>>>>>          return false;
>>>>>   }
>>>>> +
>>>>> +__weak bool arch_topdown_use_json_metrics(void)
>>>>> +{
>>>
>>> AFAICS, only x86 supports topdown today and that is because they have 
>>> special kernel topdown events exposed for the kernel CPU PMU driver. 
>>> So other architectures - not only arm - would need rely on 
>>> metricgroups for topdown support. So let's make this generic for all 
>>> archs.
>>>
>>>> I like this extension! I've ranted in the past about weak symbols
>>>> breaking with archives due to lazy loading [1]. In this case
>>>> tools/perf/arch/arm64/util/topdown.c has no other symbols within it
>>>> and so the weak symbol has an extra chance of being linked
>>>> incorrectly. We could add a new command line of --topdown-json to
>>>> avoid this, but there seems little difference in doing this over just
>>>> doing '-M TopDownL1'.
>>>
>>>
>>>> Is it possible to use the json metric approach
>>>> for when the CPU version fails?
>>>
>>> I think that's a good idea.
>>>
>>
>>
>> While looking into using the json metrics approach as a fallback to 
>> the original, I noticed  there are two json metricgroups 'TopdownL1' 
>> and 'TopDownL1' (note the case difference) on x86. Not sure if the 
>> case difference is intentional.
>>
>> On skylake, 'TopdownL1' contains the four json metrics Retiring, 
>> Bad_Speculation, Frontend_Bound, and Backend_Bound.  'TopDownL1' has 
>> 'SLOTS', 'CoreIPC', 'CoreIPC_SMT', 'Instructions'.  I think its a 
>> similar situation on other x86 chips.
> 
> 
> There's also SMT metrics.
> 
> 
> We don't want to include CoreIPC etc. by default because it would cause 
> multiplexing in common situations.
> 
>>
>> The search for those metrics by metricgroup name is case insensitive, 
>> so it's picking up all 8 metrics when using the lookup string 
>> 'TopDownL1'.  So the extra 'SLOTS', 'CoreIPC', 'CoreIPC_SMT', 
>> 'Instructions' metrics would be printed as well.
>>
>> Not sure what the significance of the case difference might be.
>>
>> Should we use a different string than 'TopDownL1' as the metric group 
>> name to search for?
> 
> 
> We should probably fix the case (or just make the match case insensitive)
> 
> Can we just keep x86 at using the kernel metrics? On Skylake and earlier 
> it needs different formulas and other options depending whether SMT is 
> on or off, so it's not straight forward to express it as json directly.
> 

I posted a v2 of these patches which keeps x86 only using the kernel 
metrics.

 
https://lore.kernel.org/linux-perf-users/20220111150749.13365-1-andrew.kilroy@arm.com/

Would be good to get your feedback,

Thanks
Andrew