[PATCH] perf list: Skip the invalid hybrid pmu

[PATCH] perf list: Skip the invalid hybrid pmu

[Jin Yao]

On hybrid platform, such as Alderlake, if atom CPUs are offlined,
the kernel still exports the sysfs path '/sys/devices/cpu_atom/' for
'cpu_atom' pmu but the file '/sys/devices/cpu_atom/cpus' is empty,
which indicates this is an invalid pmu.

The perf-list needs to check and skip the invalid hybrid pmu.

Before:

  # perf list
  ...
  branch-instructions OR cpu_atom/branch-instructions/ [Kernel PMU event]
  branch-instructions OR cpu_core/branch-instructions/ [Kernel PMU event]
  branch-misses OR cpu_atom/branch-misses/           [Kernel PMU event]
  branch-misses OR cpu_core/branch-misses/           [Kernel PMU event]
  bus-cycles OR cpu_atom/bus-cycles/                 [Kernel PMU event]
  bus-cycles OR cpu_core/bus-cycles/                 [Kernel PMU event]
  ...

The cpu_atom events are still displayed even if atom CPUs are offlined.

After:

  # perf list
  ...
  branch-instructions OR cpu_core/branch-instructions/ [Kernel PMU event]
  branch-misses OR cpu_core/branch-misses/           [Kernel PMU event]
  bus-cycles OR cpu_core/bus-cycles/                 [Kernel PMU event]
  ...

Now only cpu_core events are displayed.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/util/pmu-hybrid.c | 11 +++++++++++
 tools/perf/util/pmu-hybrid.h |  2 ++
 tools/perf/util/pmu.c        |  3 +++
 3 files changed, 16 insertions(+)

diff --git a/tools/perf/util/pmu-hybrid.c b/tools/perf/util/pmu-hybrid.c
index f51ccaac60ee..fcc1182f8fe5 100644
--- a/tools/perf/util/pmu-hybrid.c
+++ b/tools/perf/util/pmu-hybrid.c
@@ -87,3 +87,14 @@ char *perf_pmu__hybrid_type_to_pmu(const char *type)
 	free(pmu_name);
 	return NULL;
 }
+
+bool perf_pmu__is_invalid_hybrid(const char *name)
+{
+	if (strncmp(name, "cpu_", 4))
+		return false;
+
+	if (perf_pmu__hybrid_mounted(name))
+		return false;
+
+	return true;
+}
diff --git a/tools/perf/util/pmu-hybrid.h b/tools/perf/util/pmu-hybrid.h
index 2b186c26a43e..8261a312c854 100644
--- a/tools/perf/util/pmu-hybrid.h
+++ b/tools/perf/util/pmu-hybrid.h
@@ -30,4 +30,6 @@ static inline int perf_pmu__hybrid_pmu_num(void)
 	return num;
 }
 
+bool perf_pmu__is_invalid_hybrid(const char *name);
+
 #endif /* __PMU_HYBRID_H */
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 88c8ecdc60b0..281670e9c4bd 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1604,6 +1604,9 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
 	pmu = NULL;
 	j = 0;
 	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
+		if (perf_pmu__is_invalid_hybrid(pmu->name))
+			continue;
+
 		list_for_each_entry(alias, &pmu->aliases, list) {
 			char *name = alias->desc ? alias->name :
 				format_alias(buf, sizeof(buf), pmu, alias);
-- 
2.17.1

Re: [PATCH] perf list: Skip the invalid hybrid pmu

[Andi Kleen]

On 6/9/2021 10:16 PM, Jin Yao wrote:
> On hybrid platform, such as Alderlake, if atom CPUs are offlined,
> the kernel still exports the sysfs path '/sys/devices/cpu_atom/' for
> 'cpu_atom' pmu but the file '/sys/devices/cpu_atom/cpus' is empty,
> which indicates this is an invalid pmu.


The patch looks good, but it would probably make sense to fix the kernel 
to avoid this too.

Of course the tools patch would still be needed for older kernels.

-Andi

Re: [PATCH] perf list: Skip the invalid hybrid pmu

[Jin, Yao]

Hi Arnaldo, hi Jiri,

On 6/17/2021 12:37 AM, Andi Kleen wrote:
> 
> On 6/9/2021 10:16 PM, Jin Yao wrote:
>> On hybrid platform, such as Alderlake, if atom CPUs are offlined,
>> the kernel still exports the sysfs path '/sys/devices/cpu_atom/' for
>> 'cpu_atom' pmu but the file '/sys/devices/cpu_atom/cpus' is empty,
>> which indicates this is an invalid pmu.
> 
> 
> The patch looks good, but it would probably make sense to fix the kernel to avoid this too.
> 
> Of course the tools patch would still be needed for older kernels.
> 
> -Andi
> 
> 

Any comments for this patch?

Thanks
Jin Yao

Re: [PATCH] perf list: Skip the invalid hybrid pmu

[Jiri Olsa]

On Thu, Jun 10, 2021 at 01:16:46PM +0800, Jin Yao wrote:
> On hybrid platform, such as Alderlake, if atom CPUs are offlined,
> the kernel still exports the sysfs path '/sys/devices/cpu_atom/' for
> 'cpu_atom' pmu but the file '/sys/devices/cpu_atom/cpus' is empty,
> which indicates this is an invalid pmu.
> 
> The perf-list needs to check and skip the invalid hybrid pmu.
> 
> Before:
> 
>   # perf list
>   ...
>   branch-instructions OR cpu_atom/branch-instructions/ [Kernel PMU event]
>   branch-instructions OR cpu_core/branch-instructions/ [Kernel PMU event]
>   branch-misses OR cpu_atom/branch-misses/           [Kernel PMU event]
>   branch-misses OR cpu_core/branch-misses/           [Kernel PMU event]
>   bus-cycles OR cpu_atom/bus-cycles/                 [Kernel PMU event]
>   bus-cycles OR cpu_core/bus-cycles/                 [Kernel PMU event]
>   ...
> 
> The cpu_atom events are still displayed even if atom CPUs are offlined.
> 
> After:
> 
>   # perf list
>   ...
>   branch-instructions OR cpu_core/branch-instructions/ [Kernel PMU event]
>   branch-misses OR cpu_core/branch-misses/           [Kernel PMU event]
>   bus-cycles OR cpu_core/bus-cycles/                 [Kernel PMU event]
>   ...
> 
> Now only cpu_core events are displayed.
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/util/pmu-hybrid.c | 11 +++++++++++
>  tools/perf/util/pmu-hybrid.h |  2 ++
>  tools/perf/util/pmu.c        |  3 +++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/tools/perf/util/pmu-hybrid.c b/tools/perf/util/pmu-hybrid.c
> index f51ccaac60ee..fcc1182f8fe5 100644
> --- a/tools/perf/util/pmu-hybrid.c
> +++ b/tools/perf/util/pmu-hybrid.c
> @@ -87,3 +87,14 @@ char *perf_pmu__hybrid_type_to_pmu(const char *type)
>  	free(pmu_name);
>  	return NULL;
>  }
> +
> +bool perf_pmu__is_invalid_hybrid(const char *name)
> +{
> +	if (strncmp(name, "cpu_", 4))
> +		return false;
> +
> +	if (perf_pmu__hybrid_mounted(name))
> +		return false;
> +
> +	return true;
> +}
> diff --git a/tools/perf/util/pmu-hybrid.h b/tools/perf/util/pmu-hybrid.h
> index 2b186c26a43e..8261a312c854 100644
> --- a/tools/perf/util/pmu-hybrid.h
> +++ b/tools/perf/util/pmu-hybrid.h
> @@ -30,4 +30,6 @@ static inline int perf_pmu__hybrid_pmu_num(void)
>  	return num;
>  }
>  
> +bool perf_pmu__is_invalid_hybrid(const char *name);
> +
>  #endif /* __PMU_HYBRID_H */
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 88c8ecdc60b0..281670e9c4bd 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1604,6 +1604,9 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
>  	pmu = NULL;
>  	j = 0;
>  	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
> +		if (perf_pmu__is_invalid_hybrid(pmu->name))
> +			continue;

hum why not detect it in pmu_lookup early on
and not add that pmu at all?

thanks,
jirka

> +
>  		list_for_each_entry(alias, &pmu->aliases, list) {
>  			char *name = alias->desc ? alias->name :
>  				format_alias(buf, sizeof(buf), pmu, alias);
> -- 
> 2.17.1
> 

Re: [PATCH] perf list: Skip the invalid hybrid pmu

[Jin, Yao]

Hi Jiri,

On 7/7/2021 3:58 AM, Jiri Olsa wrote:
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 88c8ecdc60b0..281670e9c4bd 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -1604,6 +1604,9 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
>>   	pmu = NULL;
>>   	j = 0;
>>   	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
>> +		if (perf_pmu__is_invalid_hybrid(pmu->name))
>> +			continue;
> hum why not detect it in pmu_lookup early on
> and not add that pmu at all?
> 
> thanks,
> jirka
> 

Yes, it's a good idea that detects it in pmu_lookup, thanks!

Thanks
Jin Yao