[PATCH] perf pmu: Fix event list for uncore PMUs

[PATCH] perf pmu: Fix event list for uncore PMUs

[John Garry]

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

Re: [PATCH] perf pmu: Fix event list for uncore PMUs

[Arnaldo Carvalho de Melo]

Em Thu, Dec 16, 2021 at 11:53:37PM +0800, John Garry escreveu:
> 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.

Hi Jin,

	Can I have your acked-by/tested-by?

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

-- 

- Arnaldo

Re: [PATCH] perf pmu: Fix event list for uncore PMUs

[John Garry]

On 18/12/2021 01:47, Arnaldo Carvalho de Melo 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.
> Hi Jin,
> 
> 	Can I have your acked-by/tested-by?

Hi Arnaldo,

I assume that address is bouncing for you also.

So if anyone else has one of these hybrid PMU x86 systems then it would 
be appreciated to check this change.

Thanks!

Re: [PATCH] perf pmu: Fix event list for uncore PMUs

[Liang, Kan]

On 12/20/2021 3:38 AM, John Garry wrote:
> On 18/12/2021 01:47, Arnaldo Carvalho de Melo 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.
>> Hi Jin,
>>
>>     Can I have your acked-by/tested-by?
> 
> Hi Arnaldo,
> 
> I assume that address is bouncing for you also.
> 

Yes, Jin Yao has left Intel.

> So if anyone else has one of these hybrid PMU x86 systems then it would 
> be appreciated to check this change.
> 

+ Zhengjun

Zhengjun,

Could you please help to verify the change?

Thanks,
Kan

Re: [PATCH] perf pmu: Fix event list for uncore PMUs

[Xing Zhengjun]

On 12/21/2021 12:34 AM, Liang, Kan wrote:
> 
> 
> On 12/20/2021 3:38 AM, John Garry wrote:
>> On 18/12/2021 01:47, Arnaldo Carvalho de Melo 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.
>>> Hi Jin,
>>>
>>>     Can I have your acked-by/tested-by?
>>
>> Hi Arnaldo,
>>
>> I assume that address is bouncing for you also.
>>
> 
> Yes, Jin Yao has left Intel.
> 
>> So if anyone else has one of these hybrid PMU x86 systems then it 
>> would be appreciated to check this change.
>>
> 
> + Zhengjun
> 
> Zhengjun,
> 
> Could you please help to verify the change?
> 
> Thanks,
> Kan

Tested this patch on one hybrid PMU x86 system, it works OK.

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

-- 
Zhengjun Xing

Re: [PATCH] perf pmu: Fix event list for uncore PMUs

[Jiri Olsa]

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
> 

Re: [PATCH] perf pmu: Fix event list for uncore PMUs

[John Garry]

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 */

Re: [PATCH] perf pmu: Fix event list for uncore PMUs

[Jiri Olsa]

On Tue, Dec 21, 2021 at 09:10:37AM +0000, John Garry wrote:
> 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.

sure, it cought my eye because the is_cpu check later is done on
the same line, so I started wondering what's the difference ;-)

jirka

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

Re: [PATCH] perf pmu: Fix event list for uncore PMUs

[John Garry]

On 21/12/2021 09:35, Jiri Olsa wrote:
> On Tue, Dec 21, 2021 at 09:10:37AM +0000, John Garry wrote:
>> 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.
> 
> sure, it cought my eye because the is_cpu check later is done on
> the same line, so I started wondering what's the difference ;-)
> 

Now thinking a bit more I am not confident that this patch is a full fix.

arm have heterogeneous CPU systems as well - which are not "hybrid" - 
and I need to ensure that aliasing is still working properly there, as I 
think that this following check would stop removing duplicates there:

+	/* uncore PMUs */
+	if (!alias_a->is_cpu && !alias_b->is_cpu)
+		return true;
+	return false;

Thanks,
John

Re: [PATCH] perf pmu: Fix event list for uncore PMUs

[Arnaldo Carvalho de Melo]

Em Tue, Dec 21, 2021 at 10:14:42AM +0000, John Garry escreveu:
> On 21/12/2021 09:35, Jiri Olsa wrote:
> > On Tue, Dec 21, 2021 at 09:10:37AM +0000, John Garry wrote:
> > > 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.
> > 
> > sure, it cought my eye because the is_cpu check later is done on
> > the same line, so I started wondering what's the difference ;-)
> > 
> 
> Now thinking a bit more I am not confident that this patch is a full fix.
> 
> arm have heterogeneous CPU systems as well - which are not "hybrid" - and I
> need to ensure that aliasing is still working properly there, as I think
> that this following check would stop removing duplicates there:
> 
> +	/* uncore PMUs */
> +	if (!alias_a->is_cpu && !alias_b->is_cpu)
> +		return true;
> +	return false;

I was about to process this, do you think its better to revert the
original patch while this gets fixed?

- Arnaldo

Re: [PATCH] perf pmu: Fix event list for uncore PMUs

[John Garry]

On 21/12/2021 20:48, Arnaldo Carvalho de Melo wrote:
>> Now thinking a bit more I am not confident that this patch is a full fix.
>>
>> arm have heterogeneous CPU systems as well - which are not "hybrid" - and I
>> need to ensure that aliasing is still working properly there, as I think
>> that this following check would stop removing duplicates there:
>>
>> +	/* uncore PMUs */
>> +	if (!alias_a->is_cpu && !alias_b->is_cpu)
>> +		return true;
>> +	return false;
> I was about to process this, do you think its better to revert the
> original patch while this gets fixed?

I think that the v2 should now be ok.

@jirka, can you kindly help to check that?

BTW, my patch is based on v5.16-rc5 . I assumed that I would need to be 
based on acme/perf/urgent, but that seems to be based on 5.15

Thanks,
John