[RFC,4/7] perf pmu: Rename uncore symbols to include system PMUs
diff mbox series

Message ID 1579876505-113251-5-git-send-email-john.garry@huawei.com
State New
Headers show
Series
  • perf pmu-events: Support event aliasing for system PMUs
Related show

Commit Message

John Garry Jan. 24, 2020, 2:35 p.m. UTC
We want to expand the perf PMU support to cover system PMUs, which are
essentially the same as uncore pmus (from a kernel sysfs perspective
anyway).

So rename pmu_is_uncore() et al to cover this.

Unfortunately we have no real way to detect if a PMU is uncore or system.
We could check the PMU name for "uncore_" prefix to detect if really uncore,
but this does not work for all uncore PMUs - maybe we should introduce
this kernel naming convention for future support.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 tools/perf/arch/arm64/util/arm-spe.c |  2 +-
 tools/perf/util/evsel.h              |  2 +-
 tools/perf/util/parse-events.c       | 12 ++++++------
 tools/perf/util/pmu.c                |  6 +++---
 tools/perf/util/pmu.h                |  2 +-
 5 files changed, 12 insertions(+), 12 deletions(-)

Comments

Jiri Olsa Feb. 10, 2020, 12:07 p.m. UTC | #1
On Fri, Jan 24, 2020 at 10:35:02PM +0800, John Garry wrote:

SNIP

>  		/* Only split the uncore group which members use alias */
> -		if (!evsel->use_uncore_alias)
> +		if (!evsel->use_uncore_or_system_alias)
>  			goto out;
>  
>  		/* The events must be from the same uncore block */
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 8b99fd312aae..569aba4cec89 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -623,7 +623,7 @@ static struct perf_cpu_map *pmu_cpumask(const char *name)
>  	return NULL;
>  }
>  
> -static bool pmu_is_uncore(const char *name)
> +static bool pmu_is_uncore_or_sys(const char *name)

so we detect uncore PMU by checking for cpumask file

I don't see the connection here with the sysid or '_sys' checking,
that's just telling which ID to use when looking for an alias, no?
shouldn't that be separated?

jirka
John Garry Feb. 10, 2020, 3:44 p.m. UTC | #2
On 10/02/2020 12:07, Jiri Olsa wrote:
> On Fri, Jan 24, 2020 at 10:35:02PM +0800, John Garry wrote:
> 
> SNIP
> 
>>   		/* Only split the uncore group which members use alias */
>> -		if (!evsel->use_uncore_alias)
>> +		if (!evsel->use_uncore_or_system_alias)
>>   			goto out;
>>   
>>   		/* The events must be from the same uncore block */
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 8b99fd312aae..569aba4cec89 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -623,7 +623,7 @@ static struct perf_cpu_map *pmu_cpumask(const char *name)
>>   	return NULL;
>>   }
>>   
>> -static bool pmu_is_uncore(const char *name)
>> +static bool pmu_is_uncore_or_sys(const char *name)
> 

Hi jirka,

> so we detect uncore PMU by checking for cpumask file
> 

For PMUs which could be considered "system" PMUs, they also have a 
cpumask, like the PMU I use as motivation for this series:

root@(none)$ pwd
/sys/bus/event_source/devices/smmuv3_pmcg_100020
root@(none)$ ls -l
total 0
-r--r--r--    1 root     root          4096 Feb 10 14:50 cpumask
drwxr-xr-x    2 root     root             0 Feb 10 14:50 events
drwxr-xr-x    2 root     root             0 Feb 10 14:50 format
-rw-r--r--    1 root     root          4096 Feb 10 14:50 
perf_event_mux_interval_ms
drwxr-xr-x    2 root     root             0 Feb 10 14:50 power
lrwxrwxrwx    1 root     root             0 Feb 10 14:50 subsystem -> 
../../bus/event_source
-r--r--r--    1 root     root          4096 Feb 10 14:50 type
-rw-r--r--    1 root     root          4096 Feb 10 14:50 uevent


Other PMU drivers which I have checked in drivers/perf also have the same.

Indeed I see no way to differentiate whether a PMU is an uncore or 
system. So that is why I change the name to cover both. Maybe there is a 
better name than the verbose pmu_is_uncore_or_sys().

> I don't see the connection here with the sysid or '_sys' checking,
> that's just telling which ID to use when looking for an alias, no?

So the connection is that in perf_pmu__find_map(), for a given PMU, the 
matching is now extended from only core or uncore PMUs to also these 
system PMUs. And I use the sysid to find an aliasing table for any 
system PMUs present.

> shouldn't that be separated?

Yes, I now think that this would be a better option. So currently I am 
combining it and it causes a problem, like I have noted in patch #5:

struct pmu_events_map *perf_pmu__find_map(struct perf_pmu *pmu)
{
[SNIP]
	sysid = perf_pmu__getsysid();

   /*
   * Match sysid as first perference for uncore/sys PMUs.
   *
   * x86 uncore events match by cpuid, but we would not have 	map->socid
* set for that arch (so any matching here would fail for that).
*/
if (pmu && pmu_is_uncore_or_sys(pmu->name) &&
    !is_arm_pmu_core(pmu->name) && sysid) {


Thanks,
John
Jiri Olsa Feb. 11, 2020, 2:43 p.m. UTC | #3
On Mon, Feb 10, 2020 at 03:44:48PM +0000, John Garry wrote:
> On 10/02/2020 12:07, Jiri Olsa wrote:
> > On Fri, Jan 24, 2020 at 10:35:02PM +0800, John Garry wrote:
> > 
> > SNIP
> > 
> > >   		/* Only split the uncore group which members use alias */
> > > -		if (!evsel->use_uncore_alias)
> > > +		if (!evsel->use_uncore_or_system_alias)
> > >   			goto out;
> > >   		/* The events must be from the same uncore block */
> > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > > index 8b99fd312aae..569aba4cec89 100644
> > > --- a/tools/perf/util/pmu.c
> > > +++ b/tools/perf/util/pmu.c
> > > @@ -623,7 +623,7 @@ static struct perf_cpu_map *pmu_cpumask(const char *name)
> > >   	return NULL;
> > >   }
> > > -static bool pmu_is_uncore(const char *name)
> > > +static bool pmu_is_uncore_or_sys(const char *name)
> > 
> 
> Hi jirka,
> 
> > so we detect uncore PMU by checking for cpumask file
> > 
> 
> For PMUs which could be considered "system" PMUs, they also have a cpumask,
> like the PMU I use as motivation for this series:
> 
> root@(none)$ pwd
> /sys/bus/event_source/devices/smmuv3_pmcg_100020
> root@(none)$ ls -l
> total 0
> -r--r--r--    1 root     root          4096 Feb 10 14:50 cpumask
> drwxr-xr-x    2 root     root             0 Feb 10 14:50 events
> drwxr-xr-x    2 root     root             0 Feb 10 14:50 format
> -rw-r--r--    1 root     root          4096 Feb 10 14:50
> perf_event_mux_interval_ms
> drwxr-xr-x    2 root     root             0 Feb 10 14:50 power
> lrwxrwxrwx    1 root     root             0 Feb 10 14:50 subsystem ->
> ../../bus/event_source
> -r--r--r--    1 root     root          4096 Feb 10 14:50 type
> -rw-r--r--    1 root     root          4096 Feb 10 14:50 uevent
> 
> 
> Other PMU drivers which I have checked in drivers/perf also have the same.
> 
> Indeed I see no way to differentiate whether a PMU is an uncore or system.
> So that is why I change the name to cover both. Maybe there is a better name
> than the verbose pmu_is_uncore_or_sys().
> 
> > I don't see the connection here with the sysid or '_sys' checking,
> > that's just telling which ID to use when looking for an alias, no?
> 
> So the connection is that in perf_pmu__find_map(), for a given PMU, the
> matching is now extended from only core or uncore PMUs to also these system
> PMUs. And I use the sysid to find an aliasing table for any system PMUs
> present.

I see.. can't we just check sysid for uncore PMUs? because
that's what the code is doing, right? having pmu_is_uncore_or_sys
makes me think there's some sysid-type PMU

jirka
John Garry Feb. 11, 2020, 3:36 p.m. UTC | #4
On 11/02/2020 14:43, Jiri Olsa wrote:
>> root@(none)$ pwd
>> /sys/bus/event_source/devices/smmuv3_pmcg_100020
>> root@(none)$ ls -l
>> total 0
>> -r--r--r--    1 root     root          4096 Feb 10 14:50 cpumask
>> drwxr-xr-x    2 root     root             0 Feb 10 14:50 events
>> drwxr-xr-x    2 root     root             0 Feb 10 14:50 format
>> -rw-r--r--    1 root     root          4096 Feb 10 14:50
>> perf_event_mux_interval_ms
>> drwxr-xr-x    2 root     root             0 Feb 10 14:50 power
>> lrwxrwxrwx    1 root     root             0 Feb 10 14:50 subsystem ->
>> ../../bus/event_source
>> -r--r--r--    1 root     root          4096 Feb 10 14:50 type
>> -rw-r--r--    1 root     root          4096 Feb 10 14:50 uevent
>>
>>
>> Other PMU drivers which I have checked in drivers/perf also have the same.
>>
>> Indeed I see no way to differentiate whether a PMU is an uncore or system.
>> So that is why I change the name to cover both. Maybe there is a better name
>> than the verbose pmu_is_uncore_or_sys().
>>
>>> I don't see the connection here with the sysid or '_sys' checking,
>>> that's just telling which ID to use when looking for an alias, no?
>> So the connection is that in perf_pmu__find_map(), for a given PMU, the
>> matching is now extended from only core or uncore PMUs to also these system
>> PMUs. And I use the sysid to find an aliasing table for any system PMUs
>> present.

Hi Jirka,

 > I see.. can't we just check sysid for uncore PMUs?

x86 will still alias PMUs (uncore or CPU) based on an alias table 
matched to the cpuid, as it is today. x86 has the benefit of fixed 
uncore PMUs for a given cpuid.

For other archs whose uncore or system PMUs are not fixed for a given 
CPU - like arm - we will support matching uncore and system PMUs on 
cpuid or sysid.

Uncore PMUs are a grey area for arm, as they may or may not be tied to a 
specific cpuid, so we will need to support both matching methods.

because
 > that's what the code is doing, right?

Not exactly.

The code will match on an alias table matched to the cpuid and also an 
alias table matched to the sysid (if perf could actually get a sysid and 
there is a table matching that sysid).

I hope that this makes sense....

having pmu_is_uncore_or_sys
 > makes me think there's some sysid-type PMU
 >
 > jirka
 >

Thanks,
John
Jiri Olsa Feb. 12, 2020, 12:08 p.m. UTC | #5
On Tue, Feb 11, 2020 at 03:36:39PM +0000, John Garry wrote:
> On 11/02/2020 14:43, Jiri Olsa wrote:
> > > root@(none)$ pwd
> > > /sys/bus/event_source/devices/smmuv3_pmcg_100020
> > > root@(none)$ ls -l
> > > total 0
> > > -r--r--r--    1 root     root          4096 Feb 10 14:50 cpumask
> > > drwxr-xr-x    2 root     root             0 Feb 10 14:50 events
> > > drwxr-xr-x    2 root     root             0 Feb 10 14:50 format
> > > -rw-r--r--    1 root     root          4096 Feb 10 14:50
> > > perf_event_mux_interval_ms
> > > drwxr-xr-x    2 root     root             0 Feb 10 14:50 power
> > > lrwxrwxrwx    1 root     root             0 Feb 10 14:50 subsystem ->
> > > ../../bus/event_source
> > > -r--r--r--    1 root     root          4096 Feb 10 14:50 type
> > > -rw-r--r--    1 root     root          4096 Feb 10 14:50 uevent
> > > 
> > > 
> > > Other PMU drivers which I have checked in drivers/perf also have the same.
> > > 
> > > Indeed I see no way to differentiate whether a PMU is an uncore or system.
> > > So that is why I change the name to cover both. Maybe there is a better name
> > > than the verbose pmu_is_uncore_or_sys().
> > > 
> > > > I don't see the connection here with the sysid or '_sys' checking,
> > > > that's just telling which ID to use when looking for an alias, no?
> > > So the connection is that in perf_pmu__find_map(), for a given PMU, the
> > > matching is now extended from only core or uncore PMUs to also these system
> > > PMUs. And I use the sysid to find an aliasing table for any system PMUs
> > > present.
> 
> Hi Jirka,
> 
> > I see.. can't we just check sysid for uncore PMUs?
> 
> x86 will still alias PMUs (uncore or CPU) based on an alias table matched to
> the cpuid, as it is today. x86 has the benefit of fixed uncore PMUs for a
> given cpuid.

ok, I did mean 'on addition' to the cpuid checks

> 
> For other archs whose uncore or system PMUs are not fixed for a given CPU -
> like arm - we will support matching uncore and system PMUs on cpuid or
> sysid.
> 
> Uncore PMUs are a grey area for arm, as they may or may not be tied to a
> specific cpuid, so we will need to support both matching methods.
> 
> because
> > that's what the code is doing, right?
> 
> Not exactly.
> 
> The code will match on an alias table matched to the cpuid and also an alias
> table matched to the sysid (if perf could actually get a sysid and there is
> a table matching that sysid).
> 
> I hope that this makes sense....

right,  please make sure this kind of explanation is in changelog
or better in the code comment 

thanks,
jirka

Patch
diff mbox series

diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index eba6541ec0f1..4241ad6c9fa0 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -223,7 +223,7 @@  struct perf_event_attr
 	}
 
 	arm_spe_pmu->selectable = true;
-	arm_spe_pmu->is_uncore = false;
+	arm_spe_pmu->is_uncore_or_sys = false;
 
 	return attr;
 }
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index dc14f4a823cd..d583b2a64d93 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -75,7 +75,7 @@  struct evsel {
 	bool			precise_max;
 	bool			ignore_missing_thread;
 	bool			forced_leader;
-	bool			use_uncore_alias;
+	bool			use_uncore_or_system_alias;
 	/* parse modifier helper */
 	int			exclude_GH;
 	int			sample_read;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index ed7c008b9c8b..89105d5f0f0b 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -367,7 +367,7 @@  __add_event(struct list_head *list, int *idx,
 	(*idx)++;
 	evsel->core.cpus   = perf_cpu_map__get(cpus);
 	evsel->core.own_cpus = perf_cpu_map__get(cpus);
-	evsel->core.system_wide = pmu ? pmu->is_uncore : false;
+	evsel->core.system_wide = pmu ? pmu->is_uncore_or_sys : false;
 	evsel->auto_merge_stats = auto_merge_stats;
 
 	if (name)
@@ -1404,7 +1404,7 @@  int parse_events_add_pmu(struct parse_events_state *parse_state,
 	struct perf_pmu *pmu;
 	struct evsel *evsel;
 	struct parse_events_error *err = parse_state->error;
-	bool use_uncore_alias;
+	bool use_uncore_or_system_alias;
 	LIST_HEAD(config_terms);
 
 	pmu = perf_pmu__find(name);
@@ -1425,7 +1425,7 @@  int parse_events_add_pmu(struct parse_events_state *parse_state,
 		memset(&attr, 0, sizeof(attr));
 	}
 
-	use_uncore_alias = (pmu->is_uncore && use_alias);
+	use_uncore_or_system_alias = (pmu->is_uncore_or_sys && use_alias);
 
 	if (!head_config) {
 		attr.type = pmu->type;
@@ -1433,7 +1433,7 @@  int parse_events_add_pmu(struct parse_events_state *parse_state,
 				    auto_merge_stats, NULL);
 		if (evsel) {
 			evsel->pmu_name = name;
-			evsel->use_uncore_alias = use_uncore_alias;
+			evsel->use_uncore_or_system_alias = use_uncore_or_system_alias;
 			return 0;
 		} else {
 			return -ENOMEM;
@@ -1481,7 +1481,7 @@  int parse_events_add_pmu(struct parse_events_state *parse_state,
 		evsel->metric_expr = info.metric_expr;
 		evsel->metric_name = info.metric_name;
 		evsel->pmu_name = name;
-		evsel->use_uncore_alias = use_uncore_alias;
+		evsel->use_uncore_or_system_alias = use_uncore_or_system_alias;
 		evsel->percore = config_term_percore(&evsel->config_terms);
 	}
 
@@ -1598,7 +1598,7 @@  parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
 	__evlist__for_each_entry(list, evsel) {
 
 		/* Only split the uncore group which members use alias */
-		if (!evsel->use_uncore_alias)
+		if (!evsel->use_uncore_or_system_alias)
 			goto out;
 
 		/* The events must be from the same uncore block */
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 8b99fd312aae..569aba4cec89 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -623,7 +623,7 @@  static struct perf_cpu_map *pmu_cpumask(const char *name)
 	return NULL;
 }
 
-static bool pmu_is_uncore(const char *name)
+static bool pmu_is_uncore_or_sys(const char *name)
 {
 	char path[PATH_MAX];
 	const char *sysfs;
@@ -769,7 +769,7 @@  static void pmu_add_cpu_aliases(struct list_head *head, struct perf_pmu *pmu)
 			break;
 		}
 
-		if (pmu_is_uncore(name) &&
+		if (pmu_is_uncore_or_sys(name) &&
 		    pmu_uncore_alias_match(pname, name))
 			goto new_alias;
 
@@ -838,7 +838,7 @@  static struct perf_pmu *pmu_lookup(const char *name)
 	pmu->cpus = pmu_cpumask(name);
 	pmu->name = strdup(name);
 	pmu->type = type;
-	pmu->is_uncore = pmu_is_uncore(name);
+	pmu->is_uncore_or_sys = pmu_is_uncore_or_sys(name);
 	pmu->max_precise = pmu_max_precise(name);
 	pmu_add_cpu_aliases(&aliases, pmu);
 
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 6737e3d5d568..67cf002c9458 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -25,7 +25,7 @@  struct perf_pmu {
 	char *name;
 	__u32 type;
 	bool selectable;
-	bool is_uncore;
+	bool is_uncore_or_sys;
 	bool auxtrace;
 	int max_precise;
 	struct perf_event_attr *default_config;