[PATCH v2] perf pmu: Fix alias events list

[PATCH v2] perf pmu: Fix alias events list

[John Garry]

Commit 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
changes the event list for uncore PMUs or arm64 heterogeneous CPU systems,
such that duplicate aliases are incorrectly listed per PMU (which they
should not be), like:

./perf list
...
unc_cbo_cache_lookup.any_es
[Unit: uncore_cbox L3 Lookup any request that access cache and found
line in E or S-state]
unc_cbo_cache_lookup.any_es
[Unit: uncore_cbox L3 Lookup any request that access cache and found
line in E or S-state]
unc_cbo_cache_lookup.any_i
[Unit: uncore_cbox L3 Lookup any request that access cache and found
line in I-state]
unc_cbo_cache_lookup.any_i
[Unit: uncore_cbox L3 Lookup any request that access cache and found
line in I-state]
...

Notice how the events are listed twice.

The named commit changed how we remove duplicate events, in that events
for different PMUs are not treated as duplicates. I suppose this is to
handle how "Each hybrid pmu event has been assigned with a pmu name".

Fix PMU alias listing by restoring behaviour to remove duplicates for
non-hybrid PMUs.

Fixes: 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
Signed-off-by: John Garry <john.garry@huawei.com>
---

Difference in v2:
- Change duplicate check to explicitly check for hybrid PMU

@Zhengjun Xing, Can you please check this new version? Thanks! 

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 6ae58406f4fc..8dfbba15aeb8 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1659,6 +1659,21 @@ bool is_pmu_core(const char *name)
 	return !strcmp(name, "cpu") || is_arm_pmu_core(name);
 }
 
+static bool pmu_alias_is_duplicate(struct sevent *alias_a,
+				   struct sevent *alias_b)
+{
+	/* Different names -> never duplicates */
+	if (strcmp(alias_a->name, alias_b->name))
+		return false;
+
+	/* Don't remove duplicates for hybrid PMUs */
+	if (perf_pmu__is_hybrid(alias_a->pmu) &&
+	    perf_pmu__is_hybrid(alias_b->pmu))
+		return false;
+
+	return true;
+}
+
 void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
 			bool long_desc, bool details_flag, bool deprecated,
 			const char *pmu_name)
@@ -1744,12 +1759,8 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
 	qsort(aliases, len, sizeof(struct sevent), cmp_sevent);
 	for (j = 0; j < len; j++) {
 		/* Skip duplicates */
-		if (j > 0 && !strcmp(aliases[j].name, aliases[j - 1].name)) {
-			if (!aliases[j].pmu || !aliases[j - 1].pmu ||
-			    !strcmp(aliases[j].pmu, aliases[j - 1].pmu)) {
-				continue;
-			}
-		}
+		if (j > 0 && pmu_alias_is_duplicate(&aliases[j], &aliases[j - 1]))
+			continue;
 
 		if (name_only) {
 			printf("%s ", aliases[j].name);
-- 
2.26.2

Re: [PATCH v2] perf pmu: Fix alias events list

[Xing Zhengjun]

On 12/22/2021 12:11 AM, John Garry wrote:
> Commit 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
> changes the event list for uncore PMUs or arm64 heterogeneous CPU systems,
> such that duplicate aliases are incorrectly listed per PMU (which they
> should not be), like:
> 
> ./perf list
> ...
> unc_cbo_cache_lookup.any_es
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in E or S-state]
> unc_cbo_cache_lookup.any_es
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in E or S-state]
> unc_cbo_cache_lookup.any_i
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in I-state]
> unc_cbo_cache_lookup.any_i
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in I-state]
> ...
> 
> Notice how the events are listed twice.
> 
> The named commit changed how we remove duplicate events, in that events
> for different PMUs are not treated as duplicates. I suppose this is to
> handle how "Each hybrid pmu event has been assigned with a pmu name".
> 
> Fix PMU alias listing by restoring behaviour to remove duplicates for
> non-hybrid PMUs.
> 
> Fixes: 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
> 
> Difference in v2:
> - Change duplicate check to explicitly check for hybrid PMU
> 
> @Zhengjun Xing, Can you please check this new version? Thanks!

Tested this new version patch on both hybrid and non-hybrid PMU x86 
systems, it works OK with no duplicate events for the uncore.

Tested-by: Zhengjun Xing <zhengjun.xing@linux.intel.com>

> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 6ae58406f4fc..8dfbba15aeb8 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1659,6 +1659,21 @@ bool is_pmu_core(const char *name)
>   	return !strcmp(name, "cpu") || is_arm_pmu_core(name);
>   }
>   
> +static bool pmu_alias_is_duplicate(struct sevent *alias_a,
> +				   struct sevent *alias_b)
> +{
> +	/* Different names -> never duplicates */
> +	if (strcmp(alias_a->name, alias_b->name))
> +		return false;
> +
> +	/* Don't remove duplicates for hybrid PMUs */
> +	if (perf_pmu__is_hybrid(alias_a->pmu) &&
> +	    perf_pmu__is_hybrid(alias_b->pmu))
> +		return false;
> +
> +	return true;
> +}
> +
>   void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
>   			bool long_desc, bool details_flag, bool deprecated,
>   			const char *pmu_name)
> @@ -1744,12 +1759,8 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
>   	qsort(aliases, len, sizeof(struct sevent), cmp_sevent);
>   	for (j = 0; j < len; j++) {
>   		/* Skip duplicates */
> -		if (j > 0 && !strcmp(aliases[j].name, aliases[j - 1].name)) {
> -			if (!aliases[j].pmu || !aliases[j - 1].pmu ||
> -			    !strcmp(aliases[j].pmu, aliases[j - 1].pmu)) {
> -				continue;
> -			}
> -		}
> +		if (j > 0 && pmu_alias_is_duplicate(&aliases[j], &aliases[j - 1]))
> +			continue;
>   
>   		if (name_only) {
>   			printf("%s ", aliases[j].name);

-- 
Zhengjun Xing

Re: [PATCH v2] perf pmu: Fix alias events list

[John Garry]

On 21/12/2021 16:11, John Garry wrote:
> Commit 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
> changes the event list for uncore PMUs or arm64 heterogeneous CPU systems,
> such that duplicate aliases are incorrectly listed per PMU (which they
> should not be), like:
> 
> ./perf list
> ...
> unc_cbo_cache_lookup.any_es
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in E or S-state]
> unc_cbo_cache_lookup.any_es
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in E or S-state]
> unc_cbo_cache_lookup.any_i
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in I-state]
> unc_cbo_cache_lookup.any_i
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in I-state]
> ...
> 
> Notice how the events are listed twice.


Hi Arnaldo,

Can you kindly consider picking up this change for v5.16?

Thanks!

Re: [PATCH v2] perf pmu: Fix alias events list

[Arnaldo Carvalho de Melo]

Em Tue, Dec 28, 2021 at 09:00:18PM +0000, John Garry escreveu:
> On 21/12/2021 16:11, John Garry wrote:
> > Commit 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
> > changes the event list for uncore PMUs or arm64 heterogeneous CPU systems,
> > such that duplicate aliases are incorrectly listed per PMU (which they
> > should not be), like:
> > 
> > ./perf list
> > ...
> > unc_cbo_cache_lookup.any_es
> > [Unit: uncore_cbox L3 Lookup any request that access cache and found
> > line in E or S-state]
> > unc_cbo_cache_lookup.any_es
> > [Unit: uncore_cbox L3 Lookup any request that access cache and found
> > line in E or S-state]
> > unc_cbo_cache_lookup.any_i
> > [Unit: uncore_cbox L3 Lookup any request that access cache and found
> > line in I-state]
> > unc_cbo_cache_lookup.any_i
> > [Unit: uncore_cbox L3 Lookup any request that access cache and found
> > line in I-state]
> > ...
> > 
> > Notice how the events are listed twice.
> 
> 
> Hi Arnaldo,
> 
> Can you kindly consider picking up this change for v5.16?

Applied, will send a pull req to Linus soon.

- Arnaldo