From:   John Garry <john.garry@huawei.com>
Subject: Re: [RFC PATCH 1/1] perf arm64: Implement --topdown with metrics
To:     Ian Rogers <irogers@google.com>,
        Andrew Kilroy <andrew.kilroy@arm.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>
References: <4c375d34-bf20-496d-22fc-aed8597126e2@huawei.com>
 <20211214184240.24215-1-andrew.kilroy@arm.com>
 <20211214184240.24215-2-andrew.kilroy@arm.com>
 <CAP-5=fXJeH0ZvcHPa20N5KfLwnYSw29rpK3OrnvE0o3u-vGTLA@mail.gmail.com>
Message-ID: <b1640897-10d7-c11e-4a7a-d17633916c8e@huawei.com>
Date:   Wed, 15 Dec 2021 10:52:25 +0000
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.1
MIME-Version: 1.0
In-Reply-To: <CAP-5=fXJeH0ZvcHPa20N5KfLwnYSw29rpK3OrnvE0o3u-vGTLA@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

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

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...

Thanks,
John