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

[Andrew Kilroy]

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@xxxxxxx/

Would be good to get your feedback,

Thanks
Andrew