Messages in this thread | | | Date | Thu, 27 Jan 2022 11:11:13 +0000 | From | Andrew Kilroy <> | Subject | Re: [RFC PATCH 1/1] perf arm64: Implement --topdown with metrics |
| |
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
| |