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

[Andrew Kilroy]

Ian, John, thanks for the feedback.

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.

Taking a look.

> In addition we could also add a --topdown arg to force using JSON 
> metricgroups.

What arg do think would be supplied?

> Did you actually test this patch? I have something experimental working 
> from some time ago, and it was more complicated than this. I need to 
> check the code again...

I got stats back from this implementation, yes.  Let me know if there's 
things my patch isn't catering for.

> Thanks,
> John