[<prev] [next>] [day] [month] [year] [list]
Message-ID: <4e2422e8-f082-4ce1-8b2b-63721fda2c74@arm.com>
Date: Tue, 18 Jun 2024 11:51:43 +0100
From: James Clark <james.clark@....com>
To: Ian Rogers <irogers@...gle.com>
Cc: linux-perf-users <linux-perf-users@...r.kernel.org>,
Robin Murphy <robin.murphy@....com>, Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
"Liang, Kan" <kan.liang@...ux.intel.com>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] perf pmu: Don't de-duplicate core PMUs
On 17/06/2024 22:30, Ian Rogers wrote:
> On Mon, Jun 17, 2024, 6:44 AM James Clark <james.clark@....com> wrote:
>
>> Arm PMUs have a suffix, either a single decimal (armv8_pmuv3_0) or 3 hex
>> digits which (armv8_cortex_a53) which Perf assumes are both strippable
>> suffixes for the purposes of deduplication. S390 "cpum_cf" is a
>> similarly suffixed core PMU but is only two characters so is not treated
>> as strippable because the rules are a minimum of 3 hex characters or 1
>> decimal character.
>>
>> There are two paths involved in listing PMU events:
>>
>> * HW/cache event printing assumes core PMUs don't have suffixes so
>> doesn't try to strip.
>> * Sysfs PMU events share the printing function with uncore PMUs which
>> strips.
>>
>> This results in slightly inconsistent Perf list behavior if a core PMU
>> has a suffix:
>>
>> # perf list
>> ...
>> armv8_pmuv3_0/branch-load-misses/
>> armv8_pmuv3/l3d_cache_wb/ [Kernel PMU event]
>> ...
>>
>> Fix it by partially reverting back to the old list behavior where
>> stripping was only done for uncore PMUs. For example commit 8d9f5146f5da
>> ("perf pmus: Sort pmus by name then suffix") mentions that only PMUs
>> starting 'uncore_' are considered to have a potential suffix. This
>> change doesn't go back that far, but does only strip PMUs that are
>> !is_core. This keeps the desirable behavior where the many possibly
>> duplicated uncore PMUs aren't repeated, but it doesn't break listing for
>> core PMUs.
>>
>> Searching for a PMU continues to use the new stripped comparison
>> functions, meaning that it's still possible to request an event by
>> specifying the common part of a PMU name, or even open events on
>> multiple similarly named PMUs. For example:
>>
>> # perf stat -e armv8_cortex/inst_retired/
>>
>> 5777173628 armv8_cortex_a53/inst_retired/ (99.93%)
>> 7469626951 armv8_cortex_a57/inst_retired/ (49.88%)
>>
>> Fixes: 3241d46f5f54 ("perf pmus: Sort/merge/aggregate PMUs like
>> mrvl_ddr_pmu")
>> Signed-off-by: James Clark <james.clark@....com>
>>
>
> Should this have my suggested by tag?
>
> Thanks,
> Ian
Yep, you're right I can add it
>
> ---
>> tools/perf/util/pmu.c | 24 ++++++++++++++++++------
>> 1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 97d74fe6d816..b73946ba9d05 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -847,6 +847,22 @@ __weak const struct pmu_metrics_table
>> *pmu_metrics_table__find(void)
>> return perf_pmu__find_metrics_table(NULL);
>> }
>>
>> +/**
>> + * Return the length of the PMU name not including the suffix for uncore
>> PMUs.
>> + *
>> + * We want to deduplicate many similar uncore PMUs by stripping their
>> suffixes,
>> + * but there are never going to be too many core PMUs and the suffixes
>> might be
>> + * interesting. "arm_cortex_a53" vs "arm_cortex_a57" or "cpum_cf" for
>> example.
>> + *
>> + * @skip_duplicate_pmus: False in verbose mode so all uncore PMUs are
>> visible
>> + */
>> +static size_t pmu_deduped_name_len(const struct perf_pmu *pmu, bool
>> skip_duplicate_pmus)
>> +{
>> + return skip_duplicate_pmus && !pmu->is_core
>> + ? pmu_name_len_no_suffix(pmu->name)
>> + : strlen(pmu->name);
>> +}
>> +
>> /**
>> * perf_pmu__match_ignoring_suffix - Does the pmu_name match tok ignoring
>> any
>> * trailing suffix? The Suffix must be
>> in form
>> @@ -1796,9 +1812,7 @@ static char *format_alias(char *buf, int len, const
>> struct perf_pmu *pmu,
>> const struct perf_pmu_alias *alias, bool
>> skip_duplicate_pmus)
>> {
>> struct parse_events_term *term;
>> - size_t pmu_name_len = skip_duplicate_pmus
>> - ? pmu_name_len_no_suffix(pmu->name)
>> - : strlen(pmu->name);
>> + size_t pmu_name_len = pmu_deduped_name_len(pmu,
>> skip_duplicate_pmus);
>> int used = snprintf(buf, len, "%.*s/%s", (int)pmu_name_len,
>> pmu->name, alias->name);
>>
>> list_for_each_entry(term, &alias->terms.terms, list) {
>> @@ -1839,9 +1853,7 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu,
>> bool skip_duplicate_pmus,
>> size_t buf_used, pmu_name_len;
>>
>> info.pmu_name = event->pmu_name ?: pmu->name;
>> - pmu_name_len = skip_duplicate_pmus
>> - ? pmu_name_len_no_suffix(info.pmu_name)
>> - : strlen(info.pmu_name);
>> + pmu_name_len = pmu_deduped_name_len(pmu,
>> skip_duplicate_pmus);
>> info.alias = NULL;
>> if (event->desc) {
>> info.name = event->name;
>> --
>> 2.34.1
>>
>>
>
Powered by blists - more mailing lists