Messages in this thread | | | Date | Wed, 15 Dec 2021 12:38:47 +0000 | Subject | Re: [RFC PATCH 1/1] perf arm64: Implement --topdown with metrics | From | 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
| |