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

From: Andrew Kilroy <andrew.kilroy@arm.com>
To: John Garry <john.garry@huawei.com>, Ian Rogers <irogers@google.com>
Cc: linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
	acme@kernel.org, Will Deacon <will@kernel.org>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Leo Yan <leo.yan@linaro.org>, Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC PATCH 1/1] perf arm64: Implement --topdown with metrics
Date: Wed, 15 Dec 2021 12:38:47 +0000	[thread overview]
Message-ID: <d91a1b98-2c2b-a961-543f-d540b3c7a146@arm.com> (raw)
In-Reply-To: <b1640897-10d7-c11e-4a7a-d17633916c8e@huawei.com>

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