Commit 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
changed the list for uncore PMUs, such that duplicate aliases are now
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 uncore PMU alias list by also checking if events with PMU name are not
cpu PMUs.

Fixes: 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
Signed-off-by: John Garry <john.garry@huawei.com>
---
It would be helpful if someone with some of these hybrid CPU systems could
test this change, thanks!

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 6ae58406f4fc..392f6a36418b 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1659,6 +1659,24 @@ 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;
+	if (!alias_a->pmu)
+		return true;
+	if (!alias_b->pmu)
+		return true;
+	if (!strcmp(alias_a->pmu, alias_b->pmu))
+		return true;
+	/* uncore PMUs */
+	if (!alias_a->is_cpu && !alias_b->is_cpu)
+		return true;
+	return false;
+}
+
 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 +1762,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

On Thu, Dec 16, 2021 at 11:53:37PM +0800, John Garry wrote:
> Commit 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
> changed the list for uncore PMUs, such that duplicate aliases are now
> 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 uncore PMU alias list by also checking if events with PMU name are not
> cpu PMUs.
> 
> Fixes: 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
> It would be helpful if someone with some of these hybrid CPU systems could
> test this change, thanks!
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 6ae58406f4fc..392f6a36418b 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1659,6 +1659,24 @@ 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;
> +	if (!alias_a->pmu)
> +		return true;
> +	if (!alias_b->pmu)
> +		return true;

nit could be:

	if (!alias_a->pmu || !alias_b->pmu)
		return true;

would be great to have more comments explaining the check

thanks,
jirka

> +	if (!strcmp(alias_a->pmu, alias_b->pmu))
> +		return true;
> +	/* uncore PMUs */
> +	if (!alias_a->is_cpu && !alias_b->is_cpu)
> +		return true;
> +	return false;
> +}
> +
>  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 +1762,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
> 

On 21/12/2021 07:58, Jiri Olsa wrote:
>> +	/* Different names -> never duplicates */
>> +	if (strcmp(alias_a->name, alias_b->name))
>> +		return false;
>> +	if (!alias_a->pmu)
>> +		return true;
>> +	if (!alias_b->pmu)
>> +		return true;
> nit could be:
> 
> 	if (!alias_a->pmu || !alias_b->pmu)
> 		return true;
> 
> would be great to have more comments explaining the check
> 

This is just a sanity check that both strings are non-NULL as we do a 
strcmp() next. So would this be better:

if (!alias_a->pmu || !alias_b->pmu || !strcmp(alias_a->pmu, alias_b->pmu))
	return true

?

It will spill a line.

Thanks,
John

> thanks,
> jirka
> 
>> +	if (!strcmp(alias_a->pmu, alias_b->pmu))
>> +		return true;
>> +	/* uncore PMUs */