[RFCv2 0/4] perf tools: play nicely with CPU PMU cpumasks

[RFCv2 0/4] perf tools: play nicely with CPU PMU cpumasks

[Mark Rutland]

Hi,

I'm trying to make the perf tool play better with PMUs in heterogeneous systems
(e.g. big.LITTLE). These patches fix some brokenness that exists today, but
they require the addition of a cpumask file to each CPU PMU sysfs directory,
and this happens to break prior versions of perf-stat. Due to this, I have not
yet added a cpumask attribute to the ARM PMU code.

In these system we have separate logical PMUs for discrete sets of CPUs. For
example, on an ARM Juno system we have a logical PMU for all Cortex-A53 CPUs,
and a logical PMU for all the Cortex-A57 CPUs. The logical PMUs allow
task-bound events, but reject CPU-bound events for CPUs they do not cover.

Currently perf-record doesn't work for these PMUs, unless forced to use
per-thread mmaps. In the absence of a cpumask, it tries to open events on CPUs
not supported by a PMU, and gives up. In the presence of a cpumask, it ends up
failing to mmap, as the evlist->cpus map contains a different set of CPUs from
the evsel->cpus map populated from the cpumask.

Today's perf-stat can profile a task in the absence of a cpumask file, but in
the presence of one ends up blocking after the profiled task exits. Due to an
inconsistency between __perf_evsel__open and read_counter, it ends up treating
some uninitialised memory as a file descriptor, and typically ends up blocked
on stdin. That can avoided as in patch 1, but existing binaries would be broken
by the addition of the cpumask kernel-side.

To cater for this, this series adds support for a new PMU sysfs file,
supported_cpus, listing a number of CPUs that a logical PMU covers. As old
binaries will not look for this, this can be safely added to the kernel without
risk of breakage.

Does using a sysfs cpumask to handle (heterogeneous) CPU PMUs feel like the
right approach?

Does it make sense to have a differently-named cpumask file that only new tools
will look at?

Since v1 [1]:
* Avoid double cpu_map__idx() call in perf_evlist__mmap_per_evsel
* Look for a supported_cpumask file when a cpumask file is not present

Thanks,
Mark.

[1] http://lkml.kernel.org/r/1467907474-3290-1-git-send-email-mark.rutland@arm.com

Mark Rutland (4):
  perf stat: balance opening and reading events
  perf: util: Add more cpu_map helpers
  perf: util: only open events on CPUs an evsel permits
  perf: util: support sysfs supported_cpumask file

 tools/perf/builtin-stat.c |  8 ++++++--
 tools/perf/util/cpumap.c  | 14 ++++++++++++--
 tools/perf/util/cpumap.h  |  2 ++
 tools/perf/util/evlist.c  |  8 +++++++-
 tools/perf/util/pmu.c     | 15 ++++++++++++---
 5 files changed, 39 insertions(+), 8 deletions(-)

-- 
1.9.1

[RFCv2 1/4] perf stat: balance opening and reading events

[Mark Rutland]

In create_perf_stat_counter, when a target CPU has not been provided, we
call __perf_evsel__open with empty_cpu_map, and open a single FD per
thread. However, in read_counter we assume that we opened events for
the product of threads and CPUs described in the evsel's cpu_map.

Thus, if an evsel has a cpu_map with more than one entry, we will
attempt to access FDs that we didn't open. This could result in a number
of problems (e.g. blocking while reading from STDIN if the fd memory
happened to be initialised to zero).

This is problematic for systems were a logical CPU PMU covers some
arbitrary subset of CPUs. The cpu_map of any evsel for that PMU will be
initialised based on the cpumask exposed through sysfs, even if the user
requests per-thread events.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org
---
 tools/perf/builtin-stat.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index ee7ada7..f3e21a2 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -276,8 +276,12 @@ perf_evsel__write_stat_event(struct perf_evsel *counter, u32 cpu, u32 thread,
 static int read_counter(struct perf_evsel *counter)
 {
 	int nthreads = thread_map__nr(evsel_list->threads);
-	int ncpus = perf_evsel__nr_cpus(counter);
-	int cpu, thread;
+	int ncpus, cpu, thread;
+
+	if (target__has_cpu(&target))
+		ncpus = perf_evsel__nr_cpus(counter);
+	else
+		ncpus = 1;
 
 	if (!counter->supported)
 		return -ENOENT;
-- 
1.9.1

[RFCv2 2/4] perf: util: Add more cpu_map helpers

[Mark Rutland]

In some cases it's necessry to figure out the map-local index of a given
Linux logical CPU ID. Add a new helper, cpu_map__idx, to acquire this.
As the logic is largely the same as the existing cpu_map__has, this is
rewritten in terms of the new helper.

At the same time, add the inverse operation, cpu_map__cpu, which yields
the logical CPU id for a map-local index. While this can be performed
manually, wrapping this in a helper can make code more legible.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org
---
 tools/perf/util/cpumap.c | 14 ++++++++++++--
 tools/perf/util/cpumap.h |  2 ++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 02d8016..efc88fe 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -590,12 +590,22 @@ int cpu__setup_cpunode_map(void)
 
 bool cpu_map__has(struct cpu_map *cpus, int cpu)
 {
+	return cpu_map__idx(cpus, cpu) != -1;
+}
+
+int cpu_map__idx(struct cpu_map *cpus, int cpu)
+{
 	int i;
 
 	for (i = 0; i < cpus->nr; ++i) {
 		if (cpus->map[i] == cpu)
-			return true;
+			return i;
 	}
 
-	return false;
+	return -1;
+}
+
+int cpu_map__cpu(struct cpu_map *cpus, int idx)
+{
+	return cpus->map[idx];
 }
diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
index 1a0a350..46f7642 100644
--- a/tools/perf/util/cpumap.h
+++ b/tools/perf/util/cpumap.h
@@ -67,5 +67,7 @@ int cpu_map__build_map(struct cpu_map *cpus, struct cpu_map **res,
 		       int (*f)(struct cpu_map *map, int cpu, void *data),
 		       void *data);
 
+int cpu_map__cpu(struct cpu_map *cpus, int idx);
 bool cpu_map__has(struct cpu_map *cpus, int cpu);
+int cpu_map__idx(struct cpu_map *cpus, int cpu);
 #endif /* __PERF_CPUMAP_H */
-- 
1.9.1

[RFCv2 3/4] perf: util: only open events on CPUs an evsel permits

[Mark Rutland]

In systems with heterogeneous CPU PMUs, it's possible for each evsel to
cover a distinct set of CPUs, and hence the cpu_map associated with each
evsel may have a distinct idx<->id mapping. Any of these may be distinct from
the evlist's cpu map.

Events can be tied to the same fd so long as they use the same per-cpu
ringbuffer (i.e. so long as they are on the same CPU). To acquire the
correct FDs, we must compare the Linux logical IDs rather than the evsel
or evlist indices.

This path adds logic to perf_evlist__mmap_per_evsel to handle this,
translating IDs as required. As PMUs may cover a subset of CPUs from the
evlist, we skip the CPUs a PMU cannot handle.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: He Kuang <hekuang@huawei.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Wang Nan <wangnan0@huawei.com>
Cc: linux-kernel@vger.kernel.org
---
 tools/perf/util/evlist.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index e82ba90..ef56b7f 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -984,17 +984,23 @@ static int __perf_evlist__mmap(struct perf_evlist *evlist, int idx,
 }
 
 static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
-				       struct mmap_params *mp, int cpu,
+				       struct mmap_params *mp, int cpu_idx,
 				       int thread, int *output)
 {
 	struct perf_evsel *evsel;
+	int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
 
 	evlist__for_each(evlist, evsel) {
 		int fd;
+		int cpu;
 
 		if (evsel->system_wide && thread)
 			continue;
 
+		cpu = cpu_map__idx(evsel->cpus, evlist_cpu);
+		if (cpu == -1)
+			continue;
+
 		fd = FD(evsel, cpu, thread);
 
 		if (*output == -1) {
-- 
1.9.1

[RFCv2 4/4] perf: util: support sysfs supported_cpumask file

[Mark Rutland]

For system PMUs, the perf tools have long expected a cpumask file under
sysfs, describing the single CPU which they support events being
opened/handled on. Prior patches in this series have reworked this
support to support multiple CPUs in a mask, as is required to handle
heterogeneous CPU PMUs.

Unfortunately, adding a cpumask file to CPU PMUs would break existing
userspace. Prior to this series, perf record will refuse to open events,
and perf stat may unexpectedly block at exit time. In the absence of a
cpumask, perf stat is functional.

To address this, this patch adds support for a new file,
supported_cpumask, which can be used to describe heterogeneous CPUs,
without the risk of breaking existing userspace binaries.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 tools/perf/util/pmu.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index ddb0261..06c985c 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -445,14 +445,23 @@ static struct cpu_map *pmu_cpumask(const char *name)
 	FILE *file;
 	struct cpu_map *cpus;
 	const char *sysfs = sysfs__mountpoint();
+	const char *path_template[] = {
+		 "%s/bus/event_source/devices/%s/cpumask",
+		 "%s/bus/event_source/devices/%s/supported_cpumask",
+		 NULL
+	};
+	unsigned int i;
 
 	if (!sysfs)
 		return NULL;
 
-	snprintf(path, PATH_MAX,
-		 "%s/bus/event_source/devices/%s/cpumask", sysfs, name);
+	for (i = 0; i < ARRAY_SIZE(path_template); i++) {
+		snprintf(path, PATH_MAX, *path_template, sysfs, name);
+		if (stat(path, &st) == 0)
+			break;
+	}
 
-	if (stat(path, &st) < 0)
+	if (!*path_template)
 		return NULL;
 
 	file = fopen(path, "r");
-- 
1.9.1

Re: [RFCv2 4/4] perf: util: support sysfs supported_cpumask file

[Jiri Olsa]

On Fri, Jul 15, 2016 at 11:08:13AM +0100, Mark Rutland wrote:
> For system PMUs, the perf tools have long expected a cpumask file under
> sysfs, describing the single CPU which they support events being

single cpu? it's cpumask.. 

> opened/handled on. Prior patches in this series have reworked this
> support to support multiple CPUs in a mask, as is required to handle
> heterogeneous CPU PMUs.
> 
> Unfortunately, adding a cpumask file to CPU PMUs would break existing
> userspace. Prior to this series, perf record will refuse to open events,

I'm lost.. we already have 'cpumask' file under pmu..

> and perf stat may unexpectedly block at exit time. In the absence of a
> cpumask, perf stat is functional.
> 
> To address this, this patch adds support for a new file,
> supported_cpumask, which can be used to describe heterogeneous CPUs,
> without the risk of breaking existing userspace binaries.

is there kernel patch adding supported_cpumask support?

thanks,
jirka

> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  tools/perf/util/pmu.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index ddb0261..06c985c 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -445,14 +445,23 @@ static struct cpu_map *pmu_cpumask(const char *name)
>  	FILE *file;
>  	struct cpu_map *cpus;
>  	const char *sysfs = sysfs__mountpoint();
> +	const char *path_template[] = {
> +		 "%s/bus/event_source/devices/%s/cpumask",
> +		 "%s/bus/event_source/devices/%s/supported_cpumask",
> +		 NULL
> +	};
> +	unsigned int i;
>  
>  	if (!sysfs)
>  		return NULL;
>  
> -	snprintf(path, PATH_MAX,
> -		 "%s/bus/event_source/devices/%s/cpumask", sysfs, name);
> +	for (i = 0; i < ARRAY_SIZE(path_template); i++) {
> +		snprintf(path, PATH_MAX, *path_template, sysfs, name);
> +		if (stat(path, &st) == 0)
> +			break;
> +	}
>  
> -	if (stat(path, &st) < 0)
> +	if (!*path_template)
>  		return NULL;
>  
>  	file = fopen(path, "r");
> -- 
> 1.9.1
> 

Re: [RFCv2 1/4] perf stat: balance opening and reading events

[Jiri Olsa]

On Fri, Jul 15, 2016 at 11:08:10AM +0100, Mark Rutland wrote:
> In create_perf_stat_counter, when a target CPU has not been provided, we
> call __perf_evsel__open with empty_cpu_map, and open a single FD per
> thread. However, in read_counter we assume that we opened events for
> the product of threads and CPUs described in the evsel's cpu_map.
> 
> Thus, if an evsel has a cpu_map with more than one entry, we will
> attempt to access FDs that we didn't open. This could result in a number
> of problems (e.g. blocking while reading from STDIN if the fd memory
> happened to be initialised to zero).
> 
> This is problematic for systems were a logical CPU PMU covers some
> arbitrary subset of CPUs. The cpu_map of any evsel for that PMU will be
> initialised based on the cpumask exposed through sysfs, even if the user
> requests per-thread events.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: linux-kernel@vger.kernel.org

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/builtin-stat.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index ee7ada7..f3e21a2 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -276,8 +276,12 @@ perf_evsel__write_stat_event(struct perf_evsel *counter, u32 cpu, u32 thread,
>  static int read_counter(struct perf_evsel *counter)
>  {
>  	int nthreads = thread_map__nr(evsel_list->threads);
> -	int ncpus = perf_evsel__nr_cpus(counter);
> -	int cpu, thread;
> +	int ncpus, cpu, thread;
> +
> +	if (target__has_cpu(&target))
> +		ncpus = perf_evsel__nr_cpus(counter);
> +	else
> +		ncpus = 1;
>  
>  	if (!counter->supported)
>  		return -ENOENT;
> -- 
> 1.9.1
> 

Re: [RFCv2 2/4] perf: util: Add more cpu_map helpers

[Jiri Olsa]

On Fri, Jul 15, 2016 at 11:08:11AM +0100, Mark Rutland wrote:
> In some cases it's necessry to figure out the map-local index of a given
> Linux logical CPU ID. Add a new helper, cpu_map__idx, to acquire this.
> As the logic is largely the same as the existing cpu_map__has, this is
> rewritten in terms of the new helper.
> 
> At the same time, add the inverse operation, cpu_map__cpu, which yields
> the logical CPU id for a map-local index. While this can be performed
> manually, wrapping this in a helper can make code more legible.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Kan Liang <kan.liang@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: linux-kernel@vger.kernel.org

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/util/cpumap.c | 14 ++++++++++++--
>  tools/perf/util/cpumap.h |  2 ++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index 02d8016..efc88fe 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -590,12 +590,22 @@ int cpu__setup_cpunode_map(void)
>  
>  bool cpu_map__has(struct cpu_map *cpus, int cpu)
>  {
> +	return cpu_map__idx(cpus, cpu) != -1;
> +}
> +
> +int cpu_map__idx(struct cpu_map *cpus, int cpu)
> +{
>  	int i;
>  
>  	for (i = 0; i < cpus->nr; ++i) {
>  		if (cpus->map[i] == cpu)
> -			return true;
> +			return i;
>  	}
>  
> -	return false;
> +	return -1;
> +}
> +
> +int cpu_map__cpu(struct cpu_map *cpus, int idx)
> +{
> +	return cpus->map[idx];
>  }
> diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
> index 1a0a350..46f7642 100644
> --- a/tools/perf/util/cpumap.h
> +++ b/tools/perf/util/cpumap.h
> @@ -67,5 +67,7 @@ int cpu_map__build_map(struct cpu_map *cpus, struct cpu_map **res,
>  		       int (*f)(struct cpu_map *map, int cpu, void *data),
>  		       void *data);
>  
> +int cpu_map__cpu(struct cpu_map *cpus, int idx);
>  bool cpu_map__has(struct cpu_map *cpus, int cpu);
> +int cpu_map__idx(struct cpu_map *cpus, int cpu);
>  #endif /* __PERF_CPUMAP_H */
> -- 
> 1.9.1
> 

Re: [RFCv2 3/4] perf: util: only open events on CPUs an evsel permits

[Jiri Olsa]

On Fri, Jul 15, 2016 at 11:08:12AM +0100, Mark Rutland wrote:
> In systems with heterogeneous CPU PMUs, it's possible for each evsel to
> cover a distinct set of CPUs, and hence the cpu_map associated with each
> evsel may have a distinct idx<->id mapping. Any of these may be distinct from
> the evlist's cpu map.
> 
> Events can be tied to the same fd so long as they use the same per-cpu
> ringbuffer (i.e. so long as they are on the same CPU). To acquire the
> correct FDs, we must compare the Linux logical IDs rather than the evsel
> or evlist indices.
> 
> This path adds logic to perf_evlist__mmap_per_evsel to handle this,
> translating IDs as required. As PMUs may cover a subset of CPUs from the
> evlist, we skip the CPUs a PMU cannot handle.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: He Kuang <hekuang@huawei.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Wang Nan <wangnan0@huawei.com>
> Cc: linux-kernel@vger.kernel.org

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/util/evlist.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index e82ba90..ef56b7f 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -984,17 +984,23 @@ static int __perf_evlist__mmap(struct perf_evlist *evlist, int idx,
>  }
>  
>  static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
> -				       struct mmap_params *mp, int cpu,
> +				       struct mmap_params *mp, int cpu_idx,
>  				       int thread, int *output)
>  {
>  	struct perf_evsel *evsel;
> +	int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
>  
>  	evlist__for_each(evlist, evsel) {
>  		int fd;
> +		int cpu;
>  
>  		if (evsel->system_wide && thread)
>  			continue;
>  
> +		cpu = cpu_map__idx(evsel->cpus, evlist_cpu);
> +		if (cpu == -1)
> +			continue;
> +
>  		fd = FD(evsel, cpu, thread);
>  
>  		if (*output == -1) {
> -- 
> 1.9.1
> 

Re: [RFCv2 4/4] perf: util: support sysfs supported_cpumask file

[Mark Rutland]

On Mon, Jul 18, 2016 at 04:30:18PM +0200, Jiri Olsa wrote:
> On Fri, Jul 15, 2016 at 11:08:13AM +0100, Mark Rutland wrote:
> > For system PMUs, the perf tools have long expected a cpumask file under
> > sysfs, describing the single CPU which they support events being
> 
> single cpu? it's cpumask.. 

Indeed.

The issue is that in practice, due to an internal inconsistency the
perf tools only work work when a single CPU is described in the mask.
More details below (and in patch 1).

> > opened/handled on. Prior patches in this series have reworked this
> > support to support multiple CPUs in a mask, as is required to handle
> > heterogeneous CPU PMUs.
> > 
> > Unfortunately, adding a cpumask file to CPU PMUs would break existing
> > userspace. Prior to this series, perf record will refuse to open events,
> 
> I'm lost.. we already have 'cpumask' file under pmu..

Sorry, I should spell out the problem more concretely:

When manipulating events, the tools sometimes use evsel->cpus, and other
times evlist->cpus. Sometimes, the two are used inconsistently, which
only works if they are the same size and/or describe the same CPUs.
Patch 1 fixes an instance of this, where the inconsistency results in
treating uninitialised memory as perf event FDs.

In the absence of a PMU cpumask file, the evsel's cpumask is initialised
to that of the evlist, so things line up.

Currently the only PMUs which happen to expose a cpumask are uncore
PMUs, which in practice only describe a single CPU.

When recording system-wide, various parts of the perf tools assume a
single CPU, regardless of evlist->cpus, for the purpose of manipulating
events. This happens to make uncore PMUs work, avoiding the
inconsistency.

Were we to just add a 'cpumask' file to our CPU PMUs, we would break
existing userspace (e.g. hitting the issue fixed in patch 1).

The difference in naming allows new userspace to do the right thing
while not breaking existing userspace, though I agree it's somewhat
clunky.

> > and perf stat may unexpectedly block at exit time. In the absence of a
> > cpumask, perf stat is functional.
> > 
> > To address this, this patch adds support for a new file,
> > supported_cpumask, which can be used to describe heterogeneous CPUs,
> > without the risk of breaking existing userspace binaries.
> 
> is there kernel patch adding supported_cpumask support?

Modulo the naming, a patch exists [1]. We were holding off adding that
until we'd figured out how to address breaking existing userspace [2].

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438239.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/441953.html

Re: [RFCv2 4/4] perf: util: support sysfs supported_cpumask file

[Suzuki K Poulose]

On 15/07/16 11:08, Mark Rutland wrote:
> For system PMUs, the perf tools have long expected a cpumask file under
> sysfs, describing the single CPU which they support events being
> opened/handled on. Prior patches in this series have reworked this
> support to support multiple CPUs in a mask, as is required to handle
> heterogeneous CPU PMUs.
>
> Unfortunately, adding a cpumask file to CPU PMUs would break existing
> userspace. Prior to this series, perf record will refuse to open events,
> and perf stat may unexpectedly block at exit time. In the absence of a
> cpumask, perf stat is functional.
>
> To address this, this patch adds support for a new file,
> supported_cpumask, which can be used to describe heterogeneous CPUs,
> without the risk of breaking existing userspace binaries.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  tools/perf/util/pmu.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index ddb0261..06c985c 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -445,14 +445,23 @@ static struct cpu_map *pmu_cpumask(const char *name)
>  	FILE *file;
>  	struct cpu_map *cpus;
>  	const char *sysfs = sysfs__mountpoint();
> +	const char *path_template[] = {
> +		 "%s/bus/event_source/devices/%s/cpumask",
> +		 "%s/bus/event_source/devices/%s/supported_cpumask",
> +		 NULL
> +	};
> +	unsigned int i;
>
>  	if (!sysfs)
>  		return NULL;
>
> -	snprintf(path, PATH_MAX,
> -		 "%s/bus/event_source/devices/%s/cpumask", sysfs, name);
> +	for (i = 0; i < ARRAY_SIZE(path_template); i++) {

The check could be "path_template[i]" to avoid an iteration with NULL
template.

> +		snprintf(path, PATH_MAX, *path_template, sysfs, name);

Btw, did you mean to use path_template[i] here instead of *path_template ?

> +		if (stat(path, &st) == 0)
> +			break;
> +	}
>
> -	if (stat(path, &st) < 0)
> +	if (!*path_template)

Same here ?

>  		return NULL;

Suzuki

Re: [RFCv2 4/4] perf: util: support sysfs supported_cpumask file

[Mark Rutland]

On Mon, Jul 18, 2016 at 05:38:16PM +0100, Suzuki K Poulose wrote:
> On 15/07/16 11:08, Mark Rutland wrote:
> >For system PMUs, the perf tools have long expected a cpumask file under
> >sysfs, describing the single CPU which they support events being
> >opened/handled on. Prior patches in this series have reworked this
> >support to support multiple CPUs in a mask, as is required to handle
> >heterogeneous CPU PMUs.
> >
> >Unfortunately, adding a cpumask file to CPU PMUs would break existing
> >userspace. Prior to this series, perf record will refuse to open events,
> >and perf stat may unexpectedly block at exit time. In the absence of a
> >cpumask, perf stat is functional.
> >
> >To address this, this patch adds support for a new file,
> >supported_cpumask, which can be used to describe heterogeneous CPUs,
> >without the risk of breaking existing userspace binaries.
> >
> >Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> >---
> > tools/perf/util/pmu.c | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> >diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> >index ddb0261..06c985c 100644
> >--- a/tools/perf/util/pmu.c
> >+++ b/tools/perf/util/pmu.c
> >@@ -445,14 +445,23 @@ static struct cpu_map *pmu_cpumask(const char *name)
> > 	FILE *file;
> > 	struct cpu_map *cpus;
> > 	const char *sysfs = sysfs__mountpoint();
> >+	const char *path_template[] = {
> >+		 "%s/bus/event_source/devices/%s/cpumask",
> >+		 "%s/bus/event_source/devices/%s/supported_cpumask",
> >+		 NULL
> >+	};
> >+	unsigned int i;
> >
> > 	if (!sysfs)
> > 		return NULL;
> >
> >-	snprintf(path, PATH_MAX,
> >-		 "%s/bus/event_source/devices/%s/cpumask", sysfs, name);
> >+	for (i = 0; i < ARRAY_SIZE(path_template); i++) {
> 
> The check could be "path_template[i]" to avoid an iteration with NULL
> template.

True. I'd reworked this loop a few times to try to avoid duplicated
checks, but evidently I'd messed that up. I'll clean this up.

> 
> >+		snprintf(path, PATH_MAX, *path_template, sysfs, name);
> 
> Btw, did you mean to use path_template[i] here instead of *path_template ?
> 
> >+		if (stat(path, &st) == 0)
> >+			break;
> >+	}
> >
> >-	if (stat(path, &st) < 0)
> >+	if (!*path_template)
> 
> Same here ?

Yes. On both counts I meant to use the current iteration's entry.

Thanks for spotting that. I'll fix that up.

Thanks,
Mark.

Re: [RFCv2 3/4] perf: util: only open events on CPUs an evsel permits

[Arnaldo Carvalho de Melo]

Em Mon, Jul 18, 2016 at 04:32:59PM +0200, Jiri Olsa escreveu:
> On Fri, Jul 15, 2016 at 11:08:12AM +0100, Mark Rutland wrote:
> > In systems with heterogeneous CPU PMUs, it's possible for each evsel to
> > cover a distinct set of CPUs, and hence the cpu_map associated with each
> > evsel may have a distinct idx<->id mapping. Any of these may be distinct from
> > the evlist's cpu map.
> > 
> > Events can be tied to the same fd so long as they use the same per-cpu
> > ringbuffer (i.e. so long as they are on the same CPU). To acquire the
> > correct FDs, we must compare the Linux logical IDs rather than the evsel
> > or evlist indices.
> > 
> > This path adds logic to perf_evlist__mmap_per_evsel to handle this,
> > translating IDs as required. As PMUs may cover a subset of CPUs from the
> > evlist, we skip the CPUs a PMU cannot handle.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Cc: He Kuang <hekuang@huawei.com>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Wang Nan <wangnan0@huawei.com>
> > Cc: linux-kernel@vger.kernel.org
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Applied the first two, this one is not applying, please check my
perf/core branch, what is there should soon be pushed to Ingo, so
tip/perf/core may be ok too.

- Arnaldo

Re: [RFCv2 3/4] perf: util: only open events on CPUs an evsel permits

[Jiri Olsa]

On Mon, Jul 18, 2016 at 07:46:04PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 18, 2016 at 04:32:59PM +0200, Jiri Olsa escreveu:
> > On Fri, Jul 15, 2016 at 11:08:12AM +0100, Mark Rutland wrote:
> > > In systems with heterogeneous CPU PMUs, it's possible for each evsel to
> > > cover a distinct set of CPUs, and hence the cpu_map associated with each
> > > evsel may have a distinct idx<->id mapping. Any of these may be distinct from
> > > the evlist's cpu map.
> > > 
> > > Events can be tied to the same fd so long as they use the same per-cpu
> > > ringbuffer (i.e. so long as they are on the same CPU). To acquire the
> > > correct FDs, we must compare the Linux logical IDs rather than the evsel
> > > or evlist indices.
> > > 
> > > This path adds logic to perf_evlist__mmap_per_evsel to handle this,
> > > translating IDs as required. As PMUs may cover a subset of CPUs from the
> > > evlist, we skip the CPUs a PMU cannot handle.
> > > 
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > > Cc: He Kuang <hekuang@huawei.com>
> > > Cc: Ingo Molnar <mingo@redhat.com>
> > > Cc: Jiri Olsa <jolsa@kernel.org>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Wang Nan <wangnan0@huawei.com>
> > > Cc: linux-kernel@vger.kernel.org
> > 
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> 
> Applied the first two, this one is not applying, please check my
> perf/core branch, what is there should soon be pushed to Ingo, so
> tip/perf/core may be ok too.

ouch, forgot to mentioned that.. 3rd one did not apply because
of the backward maps we just merged in, I changed it for my review,
but it needs repost

jirka

[tip:perf/core] perf stat: Balance opening and reading events

[tip-bot for Mark Rutland]

Commit-ID:  00e727bb389359c81101b03d34fec8cc7be5168d
Gitweb:     http://git.kernel.org/tip/00e727bb389359c81101b03d34fec8cc7be5168d
Author:     Mark Rutland <mark.rutland@arm.com>
AuthorDate: Fri, 15 Jul 2016 11:08:10 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 18 Jul 2016 19:41:14 -0300

perf stat: Balance opening and reading events

In create_perf_stat_counter, when a target CPU has not been provided, we
call __perf_evsel__open with empty_cpu_map, and open a single FD per
thread. However, in read_counter we assume that we opened events for the
product of threads and CPUs described in the evsel's cpu_map.

Thus, if an evsel has a cpu_map with more than one entry, we will
attempt to access FDs that we didn't open. This could result in a number
of problems (e.g. blocking while reading from STDIN if the fd memory
happened to be initialised to zero).

This is problematic for systems were a logical CPU PMU covers some
arbitrary subset of CPUs. The cpu_map of any evsel for that PMU will be
initialised based on the cpumask exposed through sysfs, even if the user
requests per-thread events.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: He Kuang <hekuang@huawei.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1468577293-19667-2-git-send-email-mark.rutland@arm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 8c5a3bf..0c16d20 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -290,8 +290,12 @@ perf_evsel__write_stat_event(struct perf_evsel *counter, u32 cpu, u32 thread,
 static int read_counter(struct perf_evsel *counter)
 {
 	int nthreads = thread_map__nr(evsel_list->threads);
-	int ncpus = perf_evsel__nr_cpus(counter);
-	int cpu, thread;
+	int ncpus, cpu, thread;
+
+	if (target__has_cpu(&target))
+		ncpus = perf_evsel__nr_cpus(counter);
+	else
+		ncpus = 1;
 
 	if (!counter->supported)
 		return -ENOENT;

[tip:perf/core] perf cpu_map: Add more helpers

[tip-bot for Mark Rutland]

Commit-ID:  9a6c582d57a0fc37fa4e13a69d9129fb3d98a401
Gitweb:     http://git.kernel.org/tip/9a6c582d57a0fc37fa4e13a69d9129fb3d98a401
Author:     Mark Rutland <mark.rutland@arm.com>
AuthorDate: Fri, 15 Jul 2016 11:08:11 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 18 Jul 2016 19:42:47 -0300

perf cpu_map: Add more helpers

In some cases it's necessry to figure out the map-local index of a given
Linux logical CPU ID. Add a new helper, cpu_map__idx, to acquire this.
As the logic is largely the same as the existing cpu_map__has, this is
rewritten in terms of the new helper.

At the same time, add the inverse operation, cpu_map__cpu, which yields
the logical CPU id for a map-local index. While this can be performed
manually, wrapping this in a helper can make code more legible.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: He Kuang <hekuang@huawei.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1468577293-19667-3-git-send-email-mark.rutland@arm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/cpumap.c | 14 ++++++++++++--
 tools/perf/util/cpumap.h |  2 ++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 15f83ac..2c0b522 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -589,14 +589,24 @@ int cpu__setup_cpunode_map(void)
 
 bool cpu_map__has(struct cpu_map *cpus, int cpu)
 {
+	return cpu_map__idx(cpus, cpu) != -1;
+}
+
+int cpu_map__idx(struct cpu_map *cpus, int cpu)
+{
 	int i;
 
 	for (i = 0; i < cpus->nr; ++i) {
 		if (cpus->map[i] == cpu)
-			return true;
+			return i;
 	}
 
-	return false;
+	return -1;
+}
+
+int cpu_map__cpu(struct cpu_map *cpus, int idx)
+{
+	return cpus->map[idx];
 }
 
 size_t cpu_map__snprint(struct cpu_map *map, char *buf, size_t size)
diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
index 206dc55..06bd689 100644
--- a/tools/perf/util/cpumap.h
+++ b/tools/perf/util/cpumap.h
@@ -68,5 +68,7 @@ int cpu_map__build_map(struct cpu_map *cpus, struct cpu_map **res,
 		       int (*f)(struct cpu_map *map, int cpu, void *data),
 		       void *data);
 
+int cpu_map__cpu(struct cpu_map *cpus, int idx);
 bool cpu_map__has(struct cpu_map *cpus, int cpu);
+int cpu_map__idx(struct cpu_map *cpus, int cpu);
 #endif /* __PERF_CPUMAP_H */

Re: [RFCv2 4/4] perf: util: support sysfs supported_cpumask file

[Jiri Olsa]

On Mon, Jul 18, 2016 at 04:00:45PM +0100, Mark Rutland wrote:
> On Mon, Jul 18, 2016 at 04:30:18PM +0200, Jiri Olsa wrote:
> > On Fri, Jul 15, 2016 at 11:08:13AM +0100, Mark Rutland wrote:
> > > For system PMUs, the perf tools have long expected a cpumask file under
> > > sysfs, describing the single CPU which they support events being
> > 
> > single cpu? it's cpumask.. 
> 
> Indeed.
> 
> The issue is that in practice, due to an internal inconsistency the
> perf tools only work work when a single CPU is described in the mask.
> More details below (and in patch 1).
> 
> > > opened/handled on. Prior patches in this series have reworked this
> > > support to support multiple CPUs in a mask, as is required to handle
> > > heterogeneous CPU PMUs.
> > > 
> > > Unfortunately, adding a cpumask file to CPU PMUs would break existing
> > > userspace. Prior to this series, perf record will refuse to open events,
> > 
> > I'm lost.. we already have 'cpumask' file under pmu..
> 
> Sorry, I should spell out the problem more concretely:
> 
> When manipulating events, the tools sometimes use evsel->cpus, and other
> times evlist->cpus. Sometimes, the two are used inconsistently, which
> only works if they are the same size and/or describe the same CPUs.
> Patch 1 fixes an instance of this, where the inconsistency results in
> treating uninitialised memory as perf event FDs.
> 
> In the absence of a PMU cpumask file, the evsel's cpumask is initialised
> to that of the evlist, so things line up.
> 
> Currently the only PMUs which happen to expose a cpumask are uncore
> PMUs, which in practice only describe a single CPU.
> 
> When recording system-wide, various parts of the perf tools assume a
> single CPU, regardless of evlist->cpus, for the purpose of manipulating
> events. This happens to make uncore PMUs work, avoiding the
> inconsistency.
> 
> Were we to just add a 'cpumask' file to our CPU PMUs, we would break
> existing userspace (e.g. hitting the issue fixed in patch 1).

so you're saying that perf is broken once pmu's cpumask
contains more than single cpu, is that right?

we should fix that, not make workarounds.. I'll go check,
I might be still missing something ;-)

would be great to have some automated test for this stuff 

thanks,
jirka

Re: [RFCv2 4/4] perf: util: support sysfs supported_cpumask file

[Mark Rutland]

On Thu, Jul 21, 2016 at 10:10:35AM +0200, Jiri Olsa wrote:
> On Mon, Jul 18, 2016 at 04:00:45PM +0100, Mark Rutland wrote:
> > On Mon, Jul 18, 2016 at 04:30:18PM +0200, Jiri Olsa wrote:
> > > On Fri, Jul 15, 2016 at 11:08:13AM +0100, Mark Rutland wrote:
> > > > For system PMUs, the perf tools have long expected a cpumask file under
> > > > sysfs, describing the single CPU which they support events being
> > > 
> > > single cpu? it's cpumask.. 
> > 
> > Indeed.
> > 
> > The issue is that in practice, due to an internal inconsistency the
> > perf tools only work work when a single CPU is described in the mask.
> > More details below (and in patch 1).
> > 
> > > > opened/handled on. Prior patches in this series have reworked this
> > > > support to support multiple CPUs in a mask, as is required to handle
> > > > heterogeneous CPU PMUs.
> > > > 
> > > > Unfortunately, adding a cpumask file to CPU PMUs would break existing
> > > > userspace. Prior to this series, perf record will refuse to open events,
> > > 
> > > I'm lost.. we already have 'cpumask' file under pmu..
> > 
> > Sorry, I should spell out the problem more concretely:
> > 
> > When manipulating events, the tools sometimes use evsel->cpus, and other
> > times evlist->cpus. Sometimes, the two are used inconsistently, which
> > only works if they are the same size and/or describe the same CPUs.
> > Patch 1 fixes an instance of this, where the inconsistency results in
> > treating uninitialised memory as perf event FDs.
> > 
> > In the absence of a PMU cpumask file, the evsel's cpumask is initialised
> > to that of the evlist, so things line up.
> > 
> > Currently the only PMUs which happen to expose a cpumask are uncore
> > PMUs, which in practice only describe a single CPU.
> > 
> > When recording system-wide, various parts of the perf tools assume a
> > single CPU, regardless of evlist->cpus, for the purpose of manipulating
> > events. This happens to make uncore PMUs work, avoiding the
> > inconsistency.
> > 
> > Were we to just add a 'cpumask' file to our CPU PMUs, we would break
> > existing userspace (e.g. hitting the issue fixed in patch 1).
> 
> so you're saying that perf is broken once pmu's cpumask
> contains more than single cpu, is that right?

Yes.

> we should fix that, not make workarounds.. I'll go check,
> I might be still missing something ;-)

I certainly agree that this should be fixed in the perf tool; hence
patches 1-3. ;)

The problem the workaround is trying to solve is kernel compatibility
with existing binaries, for which (prior to this series):

- perf record doesn't work by default in heterogeneous systems in the
  *absence* of a cpumask.

- perf stat doesn't work by default in heterogeneous systems in the
  *presence* of a cpumask.

The kernel doesn't *currently* expose a cpumask for the ARM CPU PMUs, so
we'd need to add one. While new userspace should work as of these
patches, I can't add a file called 'cpumask' kernel-side without
breaking existing perf existing binaries (in the case of perf stat).

If it's possible to solve this without exposing a cpumask file at all,
that would be ideal, but so far I haven't been able to make that work.
Any ideas welcome!

> would be great to have some automated test for this stuff 

Good point. I will take a look into that.

Thanks,
Mark.

[1] http://lkml.kernel.org/r/1468577293-19667-1-git-send-email-mark.rutland@arm.com