| Message ID | 1579876505-113251-5-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf pmu-events: Support event aliasing for system PMUs | expand |
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
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
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
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
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
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;
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(-)