Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1444138 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFCv2 0/4] perf tools: play nicely with CPU PMU cpumasks Mark Rutland <mark.rutland@arm.com> - 2016-07-15 12:10 +0200
    [RFCv2 1/4] perf stat: balance opening and reading events Mark Rutland <mark.rutland@arm.com> - 2016-07-15 12:10 +0200
      Re: [RFCv2 1/4] perf stat: balance opening and reading events Jiri Olsa <jolsa@redhat.com> - 2016-07-18 16:40 +0200
      [tip:perf/core] perf stat: Balance opening and reading events tip-bot for Mark Rutland <tipbot@zytor.com> - 2016-07-19 09:00 +0200
    [RFCv2 3/4] perf: util: only open events on CPUs an evsel permits Mark Rutland <mark.rutland@arm.com> - 2016-07-15 12:20 +0200
      Re: [RFCv2 3/4] perf: util: only open events on CPUs an evsel permits Jiri Olsa <jolsa@redhat.com> - 2016-07-18 16:40 +0200
        Re: [RFCv2 3/4] perf: util: only open events on CPUs an evsel permits Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-07-19 00:50 +0200
          Re: [RFCv2 3/4] perf: util: only open events on CPUs an evsel permits Jiri Olsa <jolsa@redhat.com> - 2016-07-19 08:30 +0200

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

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

[toc] | [next] | [standalone]

#1444141 — [RFCv2 1/4] 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>
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

[toc] | [prev] | [next] | [standalone]

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

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
> 

[toc] | [prev] | [next] | [standalone]

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

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;

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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
> 

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web