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

Groups > linux.kernel > #1569897 > unrolled thread

[PATCH] perf: fix topology test on systems with sparse CPUs

Back to article view | Back to linux.kernel

Contents

  [PATCH] perf: fix topology test on systems with sparse CPUs Jan Stancek <jstancek@redhat.com> - 2017-01-30 18:00 +0100
    Re: [PATCH] perf: fix topology test on systems with sparse CPUs Jiri Olsa <jolsa@redhat.com> - 2017-01-30 19:50 +0100
      Re: [PATCH] perf: fix topology test on systems with sparse CPUs Jan Stancek <jstancek@redhat.com> - 2017-01-30 20:30 +0100
      Re: [PATCH] perf: fix topology test on systems with sparse CPUs Jan Stancek <jstancek@redhat.com> - 2017-01-31 17:10 +0100
        Re: [PATCH] perf: fix topology test on systems with sparse CPUs Jiri Olsa <jolsa@redhat.com> - 2017-02-02 12:30 +0100
          Re: [PATCH] perf: fix topology test on systems with sparse CPUs Jan Stancek <jstancek@redhat.com> - 2017-02-02 13:10 +0100
            Re: [PATCH] perf: fix topology test on systems with sparse CPUs Jiri Olsa <jolsa@redhat.com> - 2017-02-02 14:10 +0100
              [PATCH v2 1/3] perf: add cpu__max_present_cpu() Jan Stancek <jstancek@redhat.com> - 2017-02-13 16:40 +0100
                [PATCH v2 2/3] perf: make build_cpu_topology skip offline/absent CPUs Jan Stancek <jstancek@redhat.com> - 2017-02-13 16:40 +0100
        Re: [PATCH] perf: fix topology test on systems with sparse CPUs Jiri Olsa <jolsa@redhat.com> - 2017-02-02 12:40 +0100
    Re: [PATCH] perf: fix topology test on systems with sparse CPUs Jiri Olsa <jolsa@redhat.com> - 2017-01-30 19:50 +0100
    Re: [PATCH] perf: fix topology test on systems with sparse CPUs Jiri Olsa <jolsa@redhat.com> - 2017-01-30 19:50 +0100

#1569897 — [PATCH] perf: fix topology test on systems with sparse CPUs

Topology test fails on systems with sparse CPUs, e.g.
CPU not present or offline:

36: Test topology in session                                 :
--- start ---
test child forked, pid 23703
templ file: /tmp/perf-test-i2rNki
failed to write feature 13
perf: Segmentation fault

available: 2 nodes (0-1)
node 0 cpus: 0 6 8 10 16 22 24 26
node 0 size: 11797 MB
node 0 free: 10526 MB
node 1 cpus: 1 7 9 11 17 23 25 27
node 1 size: 12065 MB
node 1 free: 10770 MB
node distances:
node   0   1
  0:  10  20
  1:  20  10

Enumerating CPU ids from 0 to _SC_NPROCESSORS_CONF-1 in header.env.cpu[]
doesn't work on system like one above, because some ids are higher than
number of CPUs, and there can be gaps.

On top of that, if CPU is offline, we can't get topology info from
sysfs entries, because they don't exist for offline CPUs.

This patch stores topology data only for online CPUs in header.env.cpu[]
list, regardless of their CPU ids, and then uses cpu_map to translate
index to actual CPU id.

Example:
coreid socketid for CPU0
coreid socketid for CPU1
coreid socketid for CPU6
coreid socketid for CPU7
...

Alternative is we go from 0 to highest CPU id, but CPUs which are
missing would contain some dummy values in topology data.

Example:
coreid socketid for CPU0
coreid socketid for CPU1
-1 -1
-1 -1
-1 -1
-1 -1
coreid socketid for CPU6
coreid socketid for CPU7
...

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 tools/perf/tests/topology.c |  7 ++++---
 tools/perf/util/env.c       | 40 ++++++++++++++++++++++++++++------------
 tools/perf/util/header.c    | 36 ++++++++++++++++++++----------------
 3 files changed, 52 insertions(+), 31 deletions(-)

diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c
index 98fe69ac553c..7b0b621ea8c0 100644
--- a/tools/perf/tests/topology.c
+++ b/tools/perf/tests/topology.c
@@ -66,17 +66,18 @@ static int check_cpu_topology(char *path, struct cpu_map *map)
 	TEST_ASSERT_VAL("can't get session", session);
 
 	for (i = 0; i < session->header.env.nr_cpus_online; i++) {
-		pr_debug("CPU %d, core %d, socket %d\n", i,
+		pr_debug("CPU %d, core %d, socket %d\n", map->map[i],
 			 session->header.env.cpu[i].core_id,
 			 session->header.env.cpu[i].socket_id);
 	}
 
 	for (i = 0; i < map->nr; i++) {
+		int cpu = map->map[i];
 		TEST_ASSERT_VAL("Core ID doesn't match",
-			(session->header.env.cpu[map->map[i]].core_id == (cpu_map__get_core(map, i, NULL) & 0xffff)));
+			(session->header.env.cpu[i].core_id == (cpu_map__get_core_id(cpu) & 0xffff)));
 
 		TEST_ASSERT_VAL("Socket ID doesn't match",
-			(session->header.env.cpu[map->map[i]].socket_id == cpu_map__get_socket(map, i, NULL)));
+			(session->header.env.cpu[i].socket_id == cpu_map__get_socket_id(cpu)));
 	}
 
 	perf_session__delete(session);
diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index bb964e86b09d..0c2cae807a61 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -60,29 +60,45 @@ int perf_env__set_cmdline(struct perf_env *env, int argc, const char *argv[])
 
 int perf_env__read_cpu_topology_map(struct perf_env *env)
 {
-	int cpu, nr_cpus;
+	int cpu, nr_cpus, i, err = 0;
+	struct cpu_map *map;
 
 	if (env->cpu != NULL)
 		return 0;
 
-	if (env->nr_cpus_avail == 0)
-		env->nr_cpus_avail = sysconf(_SC_NPROCESSORS_CONF);
+	map = cpu_map__new(NULL);
+	if (map == NULL) {
+		pr_debug("failed to get system cpumap\n");
+		err = -ENOMEM;
+		goto out;
+	}
+
+	if (env->nr_cpus_online == 0)
+		env->nr_cpus_online = map->nr;
 
-	nr_cpus = env->nr_cpus_avail;
-	if (nr_cpus == -1)
-		return -EINVAL;
+	nr_cpus = env->nr_cpus_online;
+	if (nr_cpus == -1 || map->nr < nr_cpus) {
+		err = -EINVAL;
+		goto out_free;
+	}
 
 	env->cpu = calloc(nr_cpus, sizeof(env->cpu[0]));
-	if (env->cpu == NULL)
-		return -ENOMEM;
+	if (env->cpu == NULL) {
+		err = -ENOMEM;
+		goto out_free;
+	}
 
-	for (cpu = 0; cpu < nr_cpus; ++cpu) {
-		env->cpu[cpu].core_id	= cpu_map__get_core_id(cpu);
-		env->cpu[cpu].socket_id	= cpu_map__get_socket_id(cpu);
+	for (i = 0; i < nr_cpus; i++) {
+		cpu = map->map[i];
+		env->cpu[i].core_id	= cpu_map__get_core_id(cpu);
+		env->cpu[i].socket_id	= cpu_map__get_socket_id(cpu);
 	}
 
 	env->nr_cpus_avail = nr_cpus;
-	return 0;
+out_free:
+	cpu_map__put(map);
+out:
+	return err;
 }
 
 void cpu_cache_level__free(struct cpu_cache_level *cache)
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index d89c9c7ef4e5..25faa93d143a 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -503,41 +503,45 @@ static void free_cpu_topo(struct cpu_topo *tp)
 
 static struct cpu_topo *build_cpu_topology(void)
 {
-	struct cpu_topo *tp;
+	struct cpu_topo *tp = NULL;
 	void *addr;
-	u32 nr, i;
+	u32 i;
 	size_t sz;
-	long ncpus;
-	int ret = -1;
-
-	ncpus = sysconf(_SC_NPROCESSORS_CONF);
-	if (ncpus < 0)
-		return NULL;
-
-	nr = (u32)(ncpus & UINT_MAX);
+	int ret = 0, cpu;
+	struct cpu_map *map;
 
-	sz = nr * sizeof(char *);
+	map = cpu_map__new(NULL);
+	if (map == NULL) {
+		pr_debug("failed to get system cpumap\n");
+		goto out;
+	}
 
+	sz = map->nr * sizeof(char *);
 	addr = calloc(1, sizeof(*tp) + 2 * sz);
 	if (!addr)
-		return NULL;
+		goto out_free;
 
 	tp = addr;
-	tp->cpu_nr = nr;
+	tp->cpu_nr = map->nr;
 	addr += sizeof(*tp);
 	tp->core_siblings = addr;
 	addr += sz;
 	tp->thread_siblings = addr;
 
-	for (i = 0; i < nr; i++) {
-		ret = build_cpu_topo(tp, i);
+	for (i = 0; i < tp->cpu_nr; i++) {
+		cpu = map->map[i];
+		ret = build_cpu_topo(tp, cpu);
 		if (ret < 0)
 			break;
 	}
+
+out_free:
+	cpu_map__put(map);
 	if (ret) {
 		free_cpu_topo(tp);
 		tp = NULL;
 	}
+out:
 	return tp;
 }
 
@@ -575,7 +579,7 @@ static int write_cpu_topology(int fd, struct perf_header *h __maybe_unused,
 	if (ret < 0)
 		goto done;
 
-	for (j = 0; j < perf_env.nr_cpus_avail; j++) {
+	for (j = 0; j < perf_env.nr_cpus_online; j++) {
 		ret = do_write(fd, &perf_env.cpu[j].core_id,
 			       sizeof(perf_env.cpu[j].core_id));
 		if (ret < 0)
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1570002

On Mon, Jan 30, 2017 at 05:53:34PM +0100, Jan Stancek wrote:

SNIP

> +		ret = build_cpu_topo(tp, cpu);
>  		if (ret < 0)
>  			break;
>  	}
> +
> +out_free:
> +	cpu_map__put(map);
>  	if (ret) {
>  		free_cpu_topo(tp);
>  		tp = NULL;
>  	}
> +out:
>  	return tp;
>  }
>  
> @@ -575,7 +579,7 @@ static int write_cpu_topology(int fd, struct perf_header *h __maybe_unused,
>  	if (ret < 0)
>  		goto done;
>  
> -	for (j = 0; j < perf_env.nr_cpus_avail; j++) {
> +	for (j = 0; j < perf_env.nr_cpus_online; j++) {

so basically we're changing from avail to online cpus

have you checked all the users of this FEATURE
if such change is ok?

I can't find any after quick search, but it would be
good to be sure and mention that in changelog

thanks,
jirka

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

#1570058

----- Original Message -----
> From: "Jiri Olsa" <jolsa@redhat.com>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: linux-kernel@vger.kernel.org, peterz@infradead.org, mingo@redhat.com, acme@kernel.org, "alexander shishkin"
> <alexander.shishkin@linux.intel.com>, jolsa@kernel.org, mhiramat@kernel.org, "rui teng"
> <rui.teng@linux.vnet.ibm.com>, sukadev@linux.vnet.ibm.com
> Sent: Monday, 30 January, 2017 7:49:08 PM
> Subject: Re: [PATCH] perf: fix topology test on systems with sparse CPUs
> 
> On Mon, Jan 30, 2017 at 05:53:34PM +0100, Jan Stancek wrote:
> 
> SNIP
> 
> > +		ret = build_cpu_topo(tp, cpu);
> >  		if (ret < 0)
> >  			break;
> >  	}
> > +
> > +out_free:
> > +	cpu_map__put(map);
> >  	if (ret) {
> >  		free_cpu_topo(tp);
> >  		tp = NULL;
> >  	}
> > +out:
> >  	return tp;
> >  }
> >  
> > @@ -575,7 +579,7 @@ static int write_cpu_topology(int fd, struct
> > perf_header *h __maybe_unused,
> >  	if (ret < 0)
> >  		goto done;
> >  
> > -	for (j = 0; j < perf_env.nr_cpus_avail; j++) {
> > +	for (j = 0; j < perf_env.nr_cpus_online; j++) {
> 
> so basically we're changing from avail to online cpus
> 
> have you checked all the users of this FEATURE
> if such change is ok?

You're right, I missed some. Looking again, I see at least
perf_env__get_core() could break.

Regards,
Jan

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

#1570854

[Multipart message — attachments visible in raw view] — view raw

On 01/30/2017 07:49 PM, Jiri Olsa wrote:
> so basically we're changing from avail to online cpus
> 
> have you checked all the users of this FEATURE
> if such change is ok?

Jiri,

It wasn't OK as there are other users who index cpu_topology_map by CPU id.
I decided to give the alternative a try (attached): keep cpu_topology_map
indexed by CPU id, but extend it to fit max present CPU.

So for a system like this one:
  _SC_NPROCESSORS_CONF == 16
  available: 2 nodes (0-1)
  node 0 cpus: 0 6 8 10 16 22 24 26
  node 0 size: 12004 MB
  node 0 free: 9470 MB
  node 1 cpus: 1 7 9 11 23 25 27
  node 1 size: 12093 MB
  node 1 free: 9406 MB
  node distances:
  node   0   1
    0:  10  20
    1:  20  10

HEADER_NRCPUS before:
nr_cpus_online = 15
nr_cpus_available = 16

HEADER_CPU_TOPOLOGY before:
core_sib_nr: 2
core_siblings: 0,6,8,10,16,22,24,26
core_siblings: 1,7,9,11,23,25,27
thread_sib_nr: 8
thread_siblings: 0,16
thread_siblings: 1
thread_siblings: 6,22
thread_siblings: 7,23
thread_siblings: 8,24
thread_siblings: 9,25
thread_siblings: 10,26
thread_siblings: 11,27
core_id: 0, socket_id: 0
core_id: 0, socket_id: 1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: 10, socket_id: 0
core_id: 10, socket_id: 1
core_id: 1, socket_id: 0
core_id: 1, socket_id: 1
core_id: 9, socket_id: 0
core_id: 9, socket_id: 1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1

HEADER_NRCPUS after:
nr_cpus_online = 15
nr_cpus_available = 28

HEADER_CPU_TOPOLOGY after:
core_sib_nr: 2
core_siblings: 0,6,8,10,16,22,24,26
core_siblings: 1,7,9,11,23,25,27
thread_sib_nr: 8
thread_siblings: 0,16
thread_siblings: 1
thread_siblings: 6,22
thread_siblings: 7,23
thread_siblings: 8,24
thread_siblings: 9,25
thread_siblings: 10,26
thread_siblings: 11,27
core_id: 0, socket_id: 0
core_id: 0, socket_id: 1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: 10, socket_id: 0
core_id: 10, socket_id: 1
core_id: 1, socket_id: 0
core_id: 1, socket_id: 1
core_id: 9, socket_id: 0
core_id: 9, socket_id: 1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: 0, socket_id: 0
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: -1, socket_id: -1
core_id: 10, socket_id: 0
core_id: 10, socket_id: 1
core_id: 1, socket_id: 0
core_id: 1, socket_id: 1
core_id: 9, socket_id: 0
core_id: 9, socket_id: 1

Regards,
Jan

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

#1572287

On Tue, Jan 31, 2017 at 05:03:51PM +0100, Jan Stancek wrote:
> On 01/30/2017 07:49 PM, Jiri Olsa wrote:
> > so basically we're changing from avail to online cpus
> > 
> > have you checked all the users of this FEATURE
> > if such change is ok?
> 
> Jiri,
> 
> It wasn't OK as there are other users who index cpu_topology_map by CPU id.
> I decided to give the alternative a try (attached): keep cpu_topology_map
> indexed by CPU id, but extend it to fit max present CPU.

please send this next time as a standard patchset,
it's hard to discuss over attachments

SNIP

> When build_cpu_topo() encounters offline/absent CPUs,
> it fails to find any sysfs entries and returns failure.
> This leads to build_cpu_topology() and write_cpu_topology()
> failing as well.
> 
> Because HEADER_CPU_TOPOLOGY has not been written, read leaves
> cpu_topology_map NULL and we get NULL ptr deref at:
> 
>  ...
>   cmd_test
>    __cmd_test
>     test_and_print
>      run_test
>       test_session_topology
>        check_cpu_topology

So IIUIC that's the key issue here.. write_cpu_topology that fails
to write the TOPO data and following readers crashing on processing
uncomplete data? if thats the case write_cpu_topology needs to
be fixed, instead of doing workarounds

SNIP

>  	u32 nr, i;
>  	size_t sz;
>  	long ncpus;
> -	int ret = -1;
> +	int ret = 0;
> +	struct cpu_map *map;
>  
>  	ncpus = sysconf(_SC_NPROCESSORS_CONF);
>  	if (ncpus < 0)
> -		return NULL;
> +		goto out;

can just return NULL

> +
> +	/* build online CPU map */
> +	map = cpu_map__new(NULL);
> +	if (map == NULL) {
> +		pr_debug("failed to get system cpumap\n");
> +		goto out;
> +	}
>  
>  	nr = (u32)(ncpus & UINT_MAX);
>  
>  	sz = nr * sizeof(char *);
> -
>  	addr = calloc(1, sizeof(*tp) + 2 * sz);
>  	if (!addr)
> -		return NULL;
> +		goto out_free;
>  
>  	tp = addr;
>  	tp->cpu_nr = nr;
> @@ -530,14 +537,21 @@ static struct cpu_topo *build_cpu_topology(void)
>  	tp->thread_siblings = addr;
>  
>  	for (i = 0; i < nr; i++) {
> +		if (!cpu_map__has(map, i))
> +			continue;
> +

so this prevents build_cpu_topo to fail due to missing topology
info because cpu is offline.. can it fail for other reasons?


>  		ret = build_cpu_topo(tp, i);
>  		if (ret < 0)
>  			break;


SNIP

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

#1572310

> 
> > When build_cpu_topo() encounters offline/absent CPUs,
> > it fails to find any sysfs entries and returns failure.
> > This leads to build_cpu_topology() and write_cpu_topology()
> > failing as well.
> > 
> > Because HEADER_CPU_TOPOLOGY has not been written, read leaves
> > cpu_topology_map NULL and we get NULL ptr deref at:
> > 
> >  ...
> >   cmd_test
> >    __cmd_test
> >     test_and_print
> >      run_test
> >       test_session_topology
> >        check_cpu_topology
> 
> So IIUIC that's the key issue here.. write_cpu_topology that fails
> to write the TOPO data and following readers crashing on processing
> uncomplete data? if thats the case write_cpu_topology needs to
> be fixed, instead of doing workarounds

It's already late when you are in write_cpu_topology(), because
build_cpu_topology() returned you NULL - there's nothing to write.
That's why patch aims to fix this in build_cpu_topology().

> 
> SNIP
> 
> >  	u32 nr, i;
> >  	size_t sz;
> >  	long ncpus;
> > -	int ret = -1;
> > +	int ret = 0;
> > +	struct cpu_map *map;
> >  
> >  	ncpus = sysconf(_SC_NPROCESSORS_CONF);
> >  	if (ncpus < 0)
> > -		return NULL;
> > +		goto out;
> 
> can just return NULL
> 
> > +
> > +	/* build online CPU map */
> > +	map = cpu_map__new(NULL);
> > +	if (map == NULL) {
> > +		pr_debug("failed to get system cpumap\n");
> > +		goto out;
> > +	}
> >  
> >  	nr = (u32)(ncpus & UINT_MAX);
> >  
> >  	sz = nr * sizeof(char *);
> > -
> >  	addr = calloc(1, sizeof(*tp) + 2 * sz);
> >  	if (!addr)
> > -		return NULL;
> > +		goto out_free;
> >  
> >  	tp = addr;
> >  	tp->cpu_nr = nr;
> > @@ -530,14 +537,21 @@ static struct cpu_topo *build_cpu_topology(void)
> >  	tp->thread_siblings = addr;
> >  
> >  	for (i = 0; i < nr; i++) {
> > +		if (!cpu_map__has(map, i))
> > +			continue;
> > +
> 
> so this prevents build_cpu_topo to fail due to missing topology
> info because cpu is offline.. can it fail for other reasons?

It's unlikely, though I suppose if you couldn't open and read something
from sysfs (say sysfs is not mounted) it can fail for online CPU too.

> 
> 
> >  		ret = build_cpu_topo(tp, i);
> >  		if (ret < 0)
> >  			break;
> 

SNIP

> For example:
>   _SC_NPROCESSORS_CONF == 16
>   available: 2 nodes (0-1)
>   node 0 cpus: 0 6 8 10 16 22 24 26
>   node 0 size: 12004 MB
>   node 0 free: 9470 MB
>   node 1 cpus: 1 7 9 11 23 25 27
>   node 1 size: 12093 MB
>   node 1 free: 9406 MB
>   node distances:
>   node   0   1
>     0:  10  20
>     1:  20  10
> so what's max_present_cpu in this example?

It's 28, which is the number of core_id/socket_id entries,
for CPUs 0 up to 27.

Regards,
Jan

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

#1572345

On Thu, Feb 02, 2017 at 07:06:43AM -0500, Jan Stancek wrote:
> > 
> > > When build_cpu_topo() encounters offline/absent CPUs,
> > > it fails to find any sysfs entries and returns failure.
> > > This leads to build_cpu_topology() and write_cpu_topology()
> > > failing as well.
> > > 
> > > Because HEADER_CPU_TOPOLOGY has not been written, read leaves
> > > cpu_topology_map NULL and we get NULL ptr deref at:
> > > 
> > >  ...
> > >   cmd_test
> > >    __cmd_test
> > >     test_and_print
> > >      run_test
> > >       test_session_topology
> > >        check_cpu_topology
> > 
> > So IIUIC that's the key issue here.. write_cpu_topology that fails
> > to write the TOPO data and following readers crashing on processing
> > uncomplete data? if thats the case write_cpu_topology needs to
> > be fixed, instead of doing workarounds
> 
> It's already late when you are in write_cpu_topology(), because
> build_cpu_topology() returned you NULL - there's nothing to write.
> That's why patch aims to fix this in build_cpu_topology().

ok, then we need to make sure we can't fail in write_cpu_topology

might be another patch scope though.. we can go with your fix so far

SNIP

> 
> > For example:
> >   _SC_NPROCESSORS_CONF == 16
> >   available: 2 nodes (0-1)
> >   node 0 cpus: 0 6 8 10 16 22 24 26
> >   node 0 size: 12004 MB
> >   node 0 free: 9470 MB
> >   node 1 cpus: 1 7 9 11 23 25 27
> >   node 1 size: 12093 MB
> >   node 1 free: 9406 MB
> >   node distances:
> >   node   0   1
> >     0:  10  20
> >     1:  20  10
> > so what's max_present_cpu in this example?
> 
> It's 28, which is the number of core_id/socket_id entries,
> for CPUs 0 up to 27.

ok, good

jirka

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

#1579872 — [PATCH v2 1/3] perf: add cpu__max_present_cpu()

Similar to cpu__max_cpu() (which returns max possible CPU),
returns max present CPU.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 tools/perf/util/cpumap.c | 22 ++++++++++++++++++++++
 tools/perf/util/cpumap.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 2c0b52264a46..8c7504939113 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -9,6 +9,7 @@
 #include "asm/bug.h"
 
 static int max_cpu_num;
+static int max_present_cpu_num;
 static int max_node_num;
 static int *cpunode_map;
 
@@ -442,6 +443,7 @@ static void set_max_cpu_num(void)
 
 	/* set up default */
 	max_cpu_num = 4096;
+	max_present_cpu_num = 4096;
 
 	mnt = sysfs__mountpoint();
 	if (!mnt)
@@ -455,6 +457,17 @@ static void set_max_cpu_num(void)
 	}
 
 	ret = get_max_num(path, &max_cpu_num);
+	if (ret)
+		goto out;
+
+	/* get the highest present cpu number for a sparse allocation */
+	ret = snprintf(path, PATH_MAX, "%s/devices/system/cpu/present", mnt);
+	if (ret == PATH_MAX) {
+		pr_err("sysfs path crossed PATH_MAX(%d) size\n", PATH_MAX);
+		goto out;
+	}
+
+	ret = get_max_num(path, &max_present_cpu_num);
 
 out:
 	if (ret)
@@ -505,6 +518,15 @@ int cpu__max_cpu(void)
 	return max_cpu_num;
 }
 
+int cpu__max_present_cpu(void)
+{
+	if (unlikely(!max_present_cpu_num))
+		set_max_cpu_num();
+
+	return max_present_cpu_num;
+}
+
+
 int cpu__get_node(int cpu)
 {
 	if (unlikely(cpunode_map == NULL)) {
diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
index 06bd689f5989..1a0549af8f5c 100644
--- a/tools/perf/util/cpumap.h
+++ b/tools/perf/util/cpumap.h
@@ -62,6 +62,7 @@ static inline bool cpu_map__empty(const struct cpu_map *map)
 
 int cpu__max_node(void);
 int cpu__max_cpu(void);
+int cpu__max_present_cpu(void);
 int cpu__get_node(int cpu);
 
 int cpu_map__build_map(struct cpu_map *cpus, struct cpu_map **res,
-- 
1.8.3.1

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

#1579885 — [PATCH v2 2/3] perf: make build_cpu_topology skip offline/absent CPUs

When build_cpu_topo() encounters offline/absent CPUs,
it fails to find any sysfs entries and returns failure.
This leads to build_cpu_topology() and write_cpu_topology()
failing as well.

Because HEADER_CPU_TOPOLOGY has not been written, read leaves
cpu_topology_map NULL and we get NULL ptr deref at:

 ...
  cmd_test
   __cmd_test
    test_and_print
     run_test
      test_session_topology
       check_cpu_topology

36: Session topology                           :
--- start ---
test child forked, pid 14902
templ file: /tmp/perf-test-4CKocW
failed to write feature HEADER_CPU_TOPOLOGY
perf: Segmentation fault
Obtained 9 stack frames.
./perf(sighandler_dump_stack+0x41) [0x5095f1]
/lib64/libc.so.6(+0x35250) [0x7f4b7c3c9250]
./perf(test_session_topology+0x1db) [0x490ceb]
./perf() [0x475b68]
./perf(cmd_test+0x5b9) [0x4763c9]
./perf() [0x4945a3]
./perf(main+0x69f) [0x427e8f]
/lib64/libc.so.6(__libc_start_main+0xf5) [0x7f4b7c3b5b35]
./perf() [0x427fb9]
test child interrupted
---- end ----
Session topology: FAILED!

This patch makes build_cpu_topology() skip offline/absent CPUs,
by checking their presence against cpu_map built from online CPUs.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 tools/perf/util/header.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Changes in v2:
- drop out label, use return NULL where possible

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index d89c9c7ef4e5..4b0ea4e92e9d 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -503,24 +503,31 @@ static void free_cpu_topo(struct cpu_topo *tp)
 
 static struct cpu_topo *build_cpu_topology(void)
 {
-	struct cpu_topo *tp;
+	struct cpu_topo *tp = NULL;
 	void *addr;
 	u32 nr, i;
 	size_t sz;
 	long ncpus;
-	int ret = -1;
+	int ret = 0;
+	struct cpu_map *map;
 
 	ncpus = sysconf(_SC_NPROCESSORS_CONF);
 	if (ncpus < 0)
 		return NULL;
 
+	/* build online CPU map */
+	map = cpu_map__new(NULL);
+	if (map == NULL) {
+		pr_debug("failed to get system cpumap\n");
+		return NULL;
+	}
+
 	nr = (u32)(ncpus & UINT_MAX);
 
 	sz = nr * sizeof(char *);
-
 	addr = calloc(1, sizeof(*tp) + 2 * sz);
 	if (!addr)
-		return NULL;
+		goto out_free;
 
 	tp = addr;
 	tp->cpu_nr = nr;
@@ -530,10 +537,16 @@ static struct cpu_topo *build_cpu_topology(void)
 	tp->thread_siblings = addr;
 
 	for (i = 0; i < nr; i++) {
+		if (!cpu_map__has(map, i))
+			continue;
+
 		ret = build_cpu_topo(tp, i);
 		if (ret < 0)
 			break;
 	}
+
+out_free:
+	cpu_map__put(map);
 	if (ret) {
 		free_cpu_topo(tp);
 		tp = NULL;
-- 
1.8.3.1

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

#1572297

On Tue, Jan 31, 2017 at 05:03:51PM +0100, Jan Stancek wrote:

SNIP

> 
> There are 2 problems wrt. cpu_topology_map on systems with sparse CPUs:
> 
> 1. offline/absent CPUs will have their socket_id and core_id set to -1
>    which triggers:
>    "socket_id number is too big.You may need to upgrade the perf tool."
> 
> 2. size of cpu_topology_map (perf_env.cpu[]) is allocated based on
>    _SC_NPROCESSORS_CONF, but can be indexed with CPU ids going above.
>    Users of perf_env.cpu[] are using CPU id as index. This can lead
>    to read beyond what was allocated:
>    ==19991== Invalid read of size 4
>    ==19991==    at 0x490CEB: check_cpu_topology (topology.c:69)
>    ==19991==    by 0x490CEB: test_session_topology (topology.c:106)
>    ...
> 
> For example:
>   _SC_NPROCESSORS_CONF == 16
>   available: 2 nodes (0-1)
>   node 0 cpus: 0 6 8 10 16 22 24 26
>   node 0 size: 12004 MB
>   node 0 free: 9470 MB
>   node 1 cpus: 1 7 9 11 23 25 27
>   node 1 size: 12093 MB
>   node 1 free: 9406 MB
>   node distances:
>   node   0   1
>     0:  10  20
>     1:  20  10

so what's max_present_cpu in this example?

jirka

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

#1570007

On Mon, Jan 30, 2017 at 05:53:34PM +0100, Jan Stancek wrote:
> Topology test fails on systems with sparse CPUs, e.g.

SNIP

>  
> -	for (i = 0; i < nr; i++) {
> -		ret = build_cpu_topo(tp, i);
> +	for (i = 0; i < tp->cpu_nr; i++) {
> +		cpu = map->map[i];

no need for cpu variable

jirka

> +		ret = build_cpu_topo(tp, cpu);
>  		if (ret < 0)
>  			break;

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

#1570015

On Mon, Jan 30, 2017 at 05:53:34PM +0100, Jan Stancek wrote:

SNIP

> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index bb964e86b09d..0c2cae807a61 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
> @@ -60,29 +60,45 @@ int perf_env__set_cmdline(struct perf_env *env, int argc, const char *argv[])
>  
>  int perf_env__read_cpu_topology_map(struct perf_env *env)
>  {
> -	int cpu, nr_cpus;
> +	int cpu, nr_cpus, i, err = 0;
> +	struct cpu_map *map;
>  
>  	if (env->cpu != NULL)
>  		return 0;
>  
> -	if (env->nr_cpus_avail == 0)
> -		env->nr_cpus_avail = sysconf(_SC_NPROCESSORS_CONF);
> +	map = cpu_map__new(NULL);

could you please put comment in here, explaining that
cpu_map__new(NULL) makes map with current online cpus

thanks,
jirka

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web