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

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

[Jan Stancek]

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

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

[Jiri Olsa]

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

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

[Jan Stancek]

----- 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

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

[Jan Stancek]

[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]

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


[-- Attachment #2: 0001-perf-add-cpu__max_present_cpu.patch --]
[-- Type: text/x-patch, Size: 2214 bytes --]

>From 9bf8ece1e397b851beedaeceeb0cd07421ff6f43 Mon Sep 17 00:00:00 2001
Message-Id: <9bf8ece1e397b851beedaeceeb0cd07421ff6f43.1485877985.git.jstancek@redhat.com>
From: Jan Stancek <jstancek@redhat.com>
Date: Tue, 31 Jan 2017 14:41:46 +0100
Subject: [PATCH 1/3 v2] 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


[-- Attachment #3: 0002-perf-make-build_cpu_topology-skip-offline-absent-CPU.patch --]
[-- Type: text/x-patch, Size: 3055 bytes --]

>From 8f23543a314f359ee0625345bbc6d54b0e278358 Mon Sep 17 00:00:00 2001
Message-Id: <8f23543a314f359ee0625345bbc6d54b0e278358.1485877985.git.jstancek@redhat.com>
In-Reply-To: <9bf8ece1e397b851beedaeceeb0cd07421ff6f43.1485877985.git.jstancek@redhat.com>
References: <9bf8ece1e397b851beedaeceeb0cd07421ff6f43.1485877985.git.jstancek@redhat.com>
From: Jan Stancek <jstancek@redhat.com>
Date: Tue, 31 Jan 2017 14:44:57 +0100
Subject: [PATCH 2/3 v2] 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 | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index d89c9c7ef4e5..2b7ed52df807 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;
+		goto out;
+
+	/* 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;
+
 		ret = build_cpu_topo(tp, i);
 		if (ret < 0)
 			break;
 	}
+
+out_free:
+	cpu_map__put(map);
 	if (ret) {
 		free_cpu_topo(tp);
 		tp = NULL;
 	}
+out:
 	return tp;
 }
 
-- 
1.8.3.1


[-- Attachment #4: 0003-perf-replace-_SC_NPROCESSORS_CONF-with-max_present_c.patch --]
[-- Type: text/x-patch, Size: 5783 bytes --]

>From 4a43c50e547b4a0bc474d56fe3f07d137774d60a Mon Sep 17 00:00:00 2001
Message-Id: <4a43c50e547b4a0bc474d56fe3f07d137774d60a.1485877985.git.jstancek@redhat.com>
In-Reply-To: <9bf8ece1e397b851beedaeceeb0cd07421ff6f43.1485877985.git.jstancek@redhat.com>
References: <9bf8ece1e397b851beedaeceeb0cd07421ff6f43.1485877985.git.jstancek@redhat.com>
From: Jan Stancek <jstancek@redhat.com>
Date: Tue, 31 Jan 2017 14:46:54 +0100
Subject: [PATCH 3/3 v2] perf: replace _SC_NPROCESSORS_CONF with max_present_cpu
 in cpu_topology_map

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

This patch changes HEADER_NRCPUS.nr_cpus_available from _SC_NPROCESSORS_CONF
to max_present_cpu and updates any user of cpu_topology_map to iterate
with nr_cpus_avail. As consequence HEADER_CPU_TOPOLOGY core_id and
socket_id lists get longer, but maintain compatibility with pre-patch
state - index to cpu_topology_map is CPU id.

 ./perf test 36 -v
36: Session topology                           :
--- start ---
test child forked, pid 22211
templ file: /tmp/perf-test-gmdX5i
CPU 0, core 0, socket 0
CPU 1, core 0, socket 1
CPU 6, core 10, socket 0
CPU 7, core 10, socket 1
CPU 8, core 1, socket 0
CPU 9, core 1, socket 1
CPU 10, core 9, socket 0
CPU 11, core 9, socket 1
CPU 16, core 0, socket 0
CPU 22, core 10, socket 0
CPU 23, core 10, socket 1
CPU 24, core 1, socket 0
CPU 25, core 1, socket 1
CPU 26, core 9, socket 0
CPU 27, core 9, socket 1
test child finished with 0
---- end ----
Session topology: Ok

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 tools/perf/builtin-stat.c   |  2 +-
 tools/perf/tests/topology.c |  4 +++-
 tools/perf/util/env.c       |  2 +-
 tools/perf/util/header.c    | 16 +++++-----------
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index a02f2e965628..9e2be1c52fbd 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1765,7 +1765,7 @@ static inline int perf_env__get_cpu(struct perf_env *env, struct cpu_map *map, i
 
 	cpu = map->map[idx];
 
-	if (cpu >= env->nr_cpus_online)
+	if (cpu >= env->nr_cpus_avail)
 		return -1;
 
 	return cpu;
diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c
index 98fe69ac553c..803f893550d6 100644
--- a/tools/perf/tests/topology.c
+++ b/tools/perf/tests/topology.c
@@ -65,7 +65,9 @@ static int check_cpu_topology(char *path, struct cpu_map *map)
 	session = perf_session__new(&file, false, NULL);
 	TEST_ASSERT_VAL("can't get session", session);
 
-	for (i = 0; i < session->header.env.nr_cpus_online; i++) {
+	for (i = 0; i < session->header.env.nr_cpus_avail; i++) {
+		if (!cpu_map__has(map, i))
+			continue;
 		pr_debug("CPU %d, core %d, socket %d\n", i,
 			 session->header.env.cpu[i].core_id,
 			 session->header.env.cpu[i].socket_id);
diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index bb964e86b09d..075fc77286bf 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -66,7 +66,7 @@ int perf_env__read_cpu_topology_map(struct perf_env *env)
 		return 0;
 
 	if (env->nr_cpus_avail == 0)
-		env->nr_cpus_avail = sysconf(_SC_NPROCESSORS_CONF);
+		env->nr_cpus_avail = cpu__max_present_cpu();
 
 	nr_cpus = env->nr_cpus_avail;
 	if (nr_cpus == -1)
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 2b7ed52df807..5c81e0853bff 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -293,11 +293,7 @@ static int write_nrcpus(int fd, struct perf_header *h __maybe_unused,
 	u32 nrc, nra;
 	int ret;
 
-	nr = sysconf(_SC_NPROCESSORS_CONF);
-	if (nr < 0)
-		return -1;
-
-	nrc = (u32)(nr & UINT_MAX);
+	nrc = cpu__max_present_cpu();
 
 	nr = sysconf(_SC_NPROCESSORS_ONLN);
 	if (nr < 0)
@@ -511,9 +507,7 @@ static struct cpu_topo *build_cpu_topology(void)
 	int ret = 0;
 	struct cpu_map *map;
 
-	ncpus = sysconf(_SC_NPROCESSORS_CONF);
-	if (ncpus < 0)
-		goto out;
+	ncpus = cpu__max_present_cpu();
 
 	/* build online CPU map */
 	map = cpu_map__new(NULL);
@@ -1138,7 +1132,7 @@ static void print_cpu_topology(struct perf_header *ph, int fd __maybe_unused,
 {
 	int nr, i;
 	char *str;
-	int cpu_nr = ph->env.nr_cpus_online;
+	int cpu_nr = ph->env.nr_cpus_avail;
 
 	nr = ph->env.nr_sibling_cores;
 	str = ph->env.sibling_cores;
@@ -1793,7 +1787,7 @@ static int process_cpu_topology(struct perf_file_section *section,
 	u32 nr, i;
 	char *str;
 	struct strbuf sb;
-	int cpu_nr = ph->env.nr_cpus_online;
+	int cpu_nr = ph->env.nr_cpus_avail;
 	u64 size = 0;
 
 	ph->env.cpu = calloc(cpu_nr, sizeof(*ph->env.cpu));
@@ -1874,7 +1868,7 @@ static int process_cpu_topology(struct perf_file_section *section,
 		if (ph->needs_swap)
 			nr = bswap_32(nr);
 
-		if (nr > (u32)cpu_nr) {
+		if (nr != (u32)-1 && nr > (u32)cpu_nr) {
 			pr_debug("socket_id number is too big."
 				 "You may need to upgrade the perf tool.\n");
 			goto free_cpu;
-- 
1.8.3.1

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

[Jiri Olsa]

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

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

[Jan Stancek]

> 
> > 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

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

[Jiri Olsa]

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

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

[Jan Stancek]

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

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

[Jan Stancek]

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

Re: [PATCH v2 2/3] perf: make build_cpu_topology skip offline/absent CPUs

[Jiri Olsa]

On Mon, Feb 13, 2017 at 04:34:35PM +0100, Jan Stancek wrote:

SNIP

> 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;

hum, shoudn't we fail if we dont find any online cpu?

jirka

> +	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
> 

Re: [PATCH v2 2/3] perf: make build_cpu_topology skip offline/absent CPUs

[Jan Stancek]

----- 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
> Sent: Tuesday, 14 February, 2017 12:01:10 PM
> Subject: Re: [PATCH v2 2/3] perf: make build_cpu_topology skip offline/absent CPUs
> 
> On Mon, Feb 13, 2017 at 04:34:35PM +0100, Jan Stancek wrote:
> 
> SNIP
> 
> > 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;
> 
> hum, shoudn't we fail if we dont find any online cpu?
> 
> jirka

You're right, there's no reason to change this. I'll send v3.

[PATCH v3 1/3] perf: add cpu__max_present_cpu()

[Jan Stancek]

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

[PATCH v3 2/3] perf: make build_cpu_topology skip offline/absent CPUs

[Jan Stancek]

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 | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Changes in v3:
- make ret = -1 by default, so we fail in case build_cpu_topo
  doesn't run at all

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index d89c9c7ef4e5..ca0f12fb5fd3 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;
+	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

Re: [PATCH v3 2/3] perf: make build_cpu_topology skip offline/absent CPUs

[Arnaldo Carvalho de Melo]

Em Fri, Feb 17, 2017 at 12:10:25PM +0100, Jan Stancek escreveu:
> 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 ---

Please add two spaces before tool output, so that we can avoid breaking
scripts looking for --- as separator of patch message from patch
content.

Also look at 'git log file/you/are/modifying' to see the patch log
style, for this one I'm changing to:

perf header: Make build_cpu_topology skip offline/absent CPUs

Thanks,

- Arnaldo

> 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 | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> Changes in v3:
> - make ret = -1 by default, so we fail in case build_cpu_topo
>   doesn't run at all
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index d89c9c7ef4e5..ca0f12fb5fd3 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;
> +	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

[tip:perf/urgent] perf header: Make build_cpu_topology skip offline/absent CPUs

[tip-bot for Jan Stancek]

Commit-ID:  43db2843a4a41cc8cdb6ab696639aeee1f4d5062
Gitweb:     http://git.kernel.org/tip/43db2843a4a41cc8cdb6ab696639aeee1f4d5062
Author:     Jan Stancek <jstancek@redhat.com>
AuthorDate: Fri, 17 Feb 2017 12:10:25 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 17 Feb 2017 12:37:04 -0300

perf header: 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>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/a271b770175524f4961d4903af33798358a4a518.1487146877.git.jstancek@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 3d12c16..1222f6c 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -505,24 +505,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;
+	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;
@@ -532,10 +539,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;

[PATCH v3 3/3] perf: replace _SC_NPROCESSORS_CONF with max_present_cpu in cpu_topology_map

[Jan Stancek]

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

This patch changes HEADER_NRCPUS.nr_cpus_available from _SC_NPROCESSORS_CONF
to max_present_cpu and updates any user of cpu_topology_map to iterate
with nr_cpus_avail. As consequence HEADER_CPU_TOPOLOGY core_id and
socket_id lists get longer, but maintain compatibility with pre-patch
state - index to cpu_topology_map is CPU id.

 ./perf test 36 -v
36: Session topology                           :
--- start ---
test child forked, pid 22211
templ file: /tmp/perf-test-gmdX5i
CPU 0, core 0, socket 0
CPU 1, core 0, socket 1
CPU 6, core 10, socket 0
CPU 7, core 10, socket 1
CPU 8, core 1, socket 0
CPU 9, core 1, socket 1
CPU 10, core 9, socket 0
CPU 11, core 9, socket 1
CPU 16, core 0, socket 0
CPU 22, core 10, socket 0
CPU 23, core 10, socket 1
CPU 24, core 1, socket 0
CPU 25, core 1, socket 1
CPU 26, core 9, socket 0
CPU 27, core 9, socket 1
test child finished with 0
---- end ----
Session topology: Ok

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 tools/perf/builtin-stat.c   |  2 +-
 tools/perf/tests/topology.c |  4 +++-
 tools/perf/util/env.c       |  2 +-
 tools/perf/util/header.c    | 16 +++++-----------
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index a02f2e965628..9e2be1c52fbd 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1765,7 +1765,7 @@ static inline int perf_env__get_cpu(struct perf_env *env, struct cpu_map *map, i
 
 	cpu = map->map[idx];
 
-	if (cpu >= env->nr_cpus_online)
+	if (cpu >= env->nr_cpus_avail)
 		return -1;
 
 	return cpu;
diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c
index 98fe69ac553c..803f893550d6 100644
--- a/tools/perf/tests/topology.c
+++ b/tools/perf/tests/topology.c
@@ -65,7 +65,9 @@ static int check_cpu_topology(char *path, struct cpu_map *map)
 	session = perf_session__new(&file, false, NULL);
 	TEST_ASSERT_VAL("can't get session", session);
 
-	for (i = 0; i < session->header.env.nr_cpus_online; i++) {
+	for (i = 0; i < session->header.env.nr_cpus_avail; i++) {
+		if (!cpu_map__has(map, i))
+			continue;
 		pr_debug("CPU %d, core %d, socket %d\n", i,
 			 session->header.env.cpu[i].core_id,
 			 session->header.env.cpu[i].socket_id);
diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index bb964e86b09d..075fc77286bf 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -66,7 +66,7 @@ int perf_env__read_cpu_topology_map(struct perf_env *env)
 		return 0;
 
 	if (env->nr_cpus_avail == 0)
-		env->nr_cpus_avail = sysconf(_SC_NPROCESSORS_CONF);
+		env->nr_cpus_avail = cpu__max_present_cpu();
 
 	nr_cpus = env->nr_cpus_avail;
 	if (nr_cpus == -1)
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index ca0f12fb5fd3..3a79a0f10bf6 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -293,11 +293,7 @@ static int write_nrcpus(int fd, struct perf_header *h __maybe_unused,
 	u32 nrc, nra;
 	int ret;
 
-	nr = sysconf(_SC_NPROCESSORS_CONF);
-	if (nr < 0)
-		return -1;
-
-	nrc = (u32)(nr & UINT_MAX);
+	nrc = cpu__max_present_cpu();
 
 	nr = sysconf(_SC_NPROCESSORS_ONLN);
 	if (nr < 0)
@@ -511,9 +507,7 @@ static struct cpu_topo *build_cpu_topology(void)
 	int ret = -1;
 	struct cpu_map *map;
 
-	ncpus = sysconf(_SC_NPROCESSORS_CONF);
-	if (ncpus < 0)
-		return NULL;
+	ncpus = cpu__max_present_cpu();
 
 	/* build online CPU map */
 	map = cpu_map__new(NULL);
@@ -1137,7 +1131,7 @@ static void print_cpu_topology(struct perf_header *ph, int fd __maybe_unused,
 {
 	int nr, i;
 	char *str;
-	int cpu_nr = ph->env.nr_cpus_online;
+	int cpu_nr = ph->env.nr_cpus_avail;
 
 	nr = ph->env.nr_sibling_cores;
 	str = ph->env.sibling_cores;
@@ -1792,7 +1786,7 @@ static int process_cpu_topology(struct perf_file_section *section,
 	u32 nr, i;
 	char *str;
 	struct strbuf sb;
-	int cpu_nr = ph->env.nr_cpus_online;
+	int cpu_nr = ph->env.nr_cpus_avail;
 	u64 size = 0;
 
 	ph->env.cpu = calloc(cpu_nr, sizeof(*ph->env.cpu));
@@ -1873,7 +1867,7 @@ static int process_cpu_topology(struct perf_file_section *section,
 		if (ph->needs_swap)
 			nr = bswap_32(nr);
 
-		if (nr > (u32)cpu_nr) {
+		if (nr != (u32)-1 && nr > (u32)cpu_nr) {
 			pr_debug("socket_id number is too big."
 				 "You may need to upgrade the perf tool.\n");
 			goto free_cpu;
-- 
1.8.3.1

Re: [PATCH v3 3/3] perf: replace _SC_NPROCESSORS_CONF with max_present_cpu in cpu_topology_map

[Jiri Olsa]

On Fri, Feb 17, 2017 at 12:10:26PM +0100, Jan Stancek wrote:

SNIP

> -	int cpu_nr = ph->env.nr_cpus_online;
> +	int cpu_nr = ph->env.nr_cpus_avail;
>  
>  	nr = ph->env.nr_sibling_cores;
>  	str = ph->env.sibling_cores;
> @@ -1792,7 +1786,7 @@ static int process_cpu_topology(struct perf_file_section *section,
>  	u32 nr, i;
>  	char *str;
>  	struct strbuf sb;
> -	int cpu_nr = ph->env.nr_cpus_online;
> +	int cpu_nr = ph->env.nr_cpus_avail;
>  	u64 size = 0;
>  
>  	ph->env.cpu = calloc(cpu_nr, sizeof(*ph->env.cpu));
> @@ -1873,7 +1867,7 @@ static int process_cpu_topology(struct perf_file_section *section,
>  		if (ph->needs_swap)
>  			nr = bswap_32(nr);
>  
> -		if (nr > (u32)cpu_nr) {
> +		if (nr != (u32)-1 && nr > (u32)cpu_nr) {

could you please add a comment here exaplaining the reason
for possible -1 in here.. other than that it's ok,

for the series:

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

thanks,
jirka

[tip:perf/urgent] perf tools: Replace _SC_NPROCESSORS_CONF with max_present_cpu in cpu_topology_map

[tip-bot for Jan Stancek]

Commit-ID:  da8a58b56c661681f9b2fd2fa59c6da3a5bac8d1
Gitweb:     http://git.kernel.org/tip/da8a58b56c661681f9b2fd2fa59c6da3a5bac8d1
Author:     Jan Stancek <jstancek@redhat.com>
AuthorDate: Fri, 17 Feb 2017 12:10:26 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 17 Feb 2017 12:56:35 -0300

perf tools: Replace _SC_NPROCESSORS_CONF with max_present_cpu in cpu_topology_map

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

This patch changes HEADER_NRCPUS.nr_cpus_available from _SC_NPROCESSORS_CONF
to max_present_cpu and updates any user of cpu_topology_map to iterate
with nr_cpus_avail.

As a consequence HEADER_CPU_TOPOLOGY core_id and socket_id lists get longer,
but maintain compatibility with pre-patch state - index to cpu_topology_map is
CPU id.

  perf test 36 -v
  36: Session topology                           :
  --- start ---
  test child forked, pid 22211
  templ file: /tmp/perf-test-gmdX5i
  CPU 0, core 0, socket 0
  CPU 1, core 0, socket 1
  CPU 6, core 10, socket 0
  CPU 7, core 10, socket 1
  CPU 8, core 1, socket 0
  CPU 9, core 1, socket 1
  CPU 10, core 9, socket 0
  CPU 11, core 9, socket 1
  CPU 16, core 0, socket 0
  CPU 22, core 10, socket 0
  CPU 23, core 10, socket 1
  CPU 24, core 1, socket 0
  CPU 25, core 1, socket 1
  CPU 26, core 9, socket 0
  CPU 27, core 9, socket 1
  test child finished with 0
  ---- end ----
  Session topology: Ok

Signed-off-by: Jan Stancek <jstancek@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/d7c05c6445fca74a8442c2c73cfffd349c52c44f.1487146877.git.jstancek@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c   |  2 +-
 tools/perf/tests/topology.c |  4 +++-
 tools/perf/util/env.c       |  2 +-
 tools/perf/util/header.c    | 16 +++++-----------
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index f287191..ca27a8a 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1765,7 +1765,7 @@ static inline int perf_env__get_cpu(struct perf_env *env, struct cpu_map *map, i
 
 	cpu = map->map[idx];
 
-	if (cpu >= env->nr_cpus_online)
+	if (cpu >= env->nr_cpus_avail)
 		return -1;
 
 	return cpu;
diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c
index 98fe69a..803f893 100644
--- a/tools/perf/tests/topology.c
+++ b/tools/perf/tests/topology.c
@@ -65,7 +65,9 @@ static int check_cpu_topology(char *path, struct cpu_map *map)
 	session = perf_session__new(&file, false, NULL);
 	TEST_ASSERT_VAL("can't get session", session);
 
-	for (i = 0; i < session->header.env.nr_cpus_online; i++) {
+	for (i = 0; i < session->header.env.nr_cpus_avail; i++) {
+		if (!cpu_map__has(map, i))
+			continue;
 		pr_debug("CPU %d, core %d, socket %d\n", i,
 			 session->header.env.cpu[i].core_id,
 			 session->header.env.cpu[i].socket_id);
diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index bb964e8..075fc77 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -66,7 +66,7 @@ int perf_env__read_cpu_topology_map(struct perf_env *env)
 		return 0;
 
 	if (env->nr_cpus_avail == 0)
-		env->nr_cpus_avail = sysconf(_SC_NPROCESSORS_CONF);
+		env->nr_cpus_avail = cpu__max_present_cpu();
 
 	nr_cpus = env->nr_cpus_avail;
 	if (nr_cpus == -1)
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 1222f6c..05714d5 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -295,11 +295,7 @@ static int write_nrcpus(int fd, struct perf_header *h __maybe_unused,
 	u32 nrc, nra;
 	int ret;
 
-	nr = sysconf(_SC_NPROCESSORS_CONF);
-	if (nr < 0)
-		return -1;
-
-	nrc = (u32)(nr & UINT_MAX);
+	nrc = cpu__max_present_cpu();
 
 	nr = sysconf(_SC_NPROCESSORS_ONLN);
 	if (nr < 0)
@@ -513,9 +509,7 @@ static struct cpu_topo *build_cpu_topology(void)
 	int ret = -1;
 	struct cpu_map *map;
 
-	ncpus = sysconf(_SC_NPROCESSORS_CONF);
-	if (ncpus < 0)
-		return NULL;
+	ncpus = cpu__max_present_cpu();
 
 	/* build online CPU map */
 	map = cpu_map__new(NULL);
@@ -1139,7 +1133,7 @@ static void print_cpu_topology(struct perf_header *ph, int fd __maybe_unused,
 {
 	int nr, i;
 	char *str;
-	int cpu_nr = ph->env.nr_cpus_online;
+	int cpu_nr = ph->env.nr_cpus_avail;
 
 	nr = ph->env.nr_sibling_cores;
 	str = ph->env.sibling_cores;
@@ -1794,7 +1788,7 @@ static int process_cpu_topology(struct perf_file_section *section,
 	u32 nr, i;
 	char *str;
 	struct strbuf sb;
-	int cpu_nr = ph->env.nr_cpus_online;
+	int cpu_nr = ph->env.nr_cpus_avail;
 	u64 size = 0;
 
 	ph->env.cpu = calloc(cpu_nr, sizeof(*ph->env.cpu));
@@ -1875,7 +1869,7 @@ static int process_cpu_topology(struct perf_file_section *section,
 		if (ph->needs_swap)
 			nr = bswap_32(nr);
 
-		if (nr > (u32)cpu_nr) {
+		if (nr != (u32)-1 && nr > (u32)cpu_nr) {
 			pr_debug("socket_id number is too big."
 				 "You may need to upgrade the perf tool.\n");
 			goto free_cpu;

[tip:perf/urgent] perf cpumap: Add cpu__max_present_cpu()

[tip-bot for Jan Stancek]

Commit-ID:  92a7e1278005b6bb3459affc50b2b6e2464e7e7c
Gitweb:     http://git.kernel.org/tip/92a7e1278005b6bb3459affc50b2b6e2464e7e7c
Author:     Jan Stancek <jstancek@redhat.com>
AuthorDate: Fri, 17 Feb 2017 12:10:24 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 17 Feb 2017 12:33:05 -0300

perf cpumap: Add cpu__max_present_cpu()

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

Signed-off-by: Jan Stancek <jstancek@redhat.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/8ea4601b5cacc49927235b4ebac424bd6eeccb06.1487146877.git.jstancek@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@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 2c0b522..8c75049 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 06bd689..1a0549a 100644
--- a/tools/perf/util/cpumap.h
+++ b/tools/perf/util/cpumap.h
@@ -62,6 +62,7 @@ int cpu__setup_cpunode_map(void);
 
 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,

[PATCH v2 3/3] perf: replace _SC_NPROCESSORS_CONF with max_present_cpu in cpu_topology_map

[Jan Stancek]

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

This patch changes HEADER_NRCPUS.nr_cpus_available from _SC_NPROCESSORS_CONF
to max_present_cpu and updates any user of cpu_topology_map to iterate
with nr_cpus_avail. As consequence HEADER_CPU_TOPOLOGY core_id and
socket_id lists get longer, but maintain compatibility with pre-patch
state - index to cpu_topology_map is CPU id.

 ./perf test 36 -v
36: Session topology                           :
--- start ---
test child forked, pid 22211
templ file: /tmp/perf-test-gmdX5i
CPU 0, core 0, socket 0
CPU 1, core 0, socket 1
CPU 6, core 10, socket 0
CPU 7, core 10, socket 1
CPU 8, core 1, socket 0
CPU 9, core 1, socket 1
CPU 10, core 9, socket 0
CPU 11, core 9, socket 1
CPU 16, core 0, socket 0
CPU 22, core 10, socket 0
CPU 23, core 10, socket 1
CPU 24, core 1, socket 0
CPU 25, core 1, socket 1
CPU 26, core 9, socket 0
CPU 27, core 9, socket 1
test child finished with 0
---- end ----
Session topology: Ok

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 tools/perf/builtin-stat.c   |  2 +-
 tools/perf/tests/topology.c |  4 +++-
 tools/perf/util/env.c       |  2 +-
 tools/perf/util/header.c    | 16 +++++-----------
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index a02f2e965628..9e2be1c52fbd 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1765,7 +1765,7 @@ static inline int perf_env__get_cpu(struct perf_env *env, struct cpu_map *map, i
 
 	cpu = map->map[idx];
 
-	if (cpu >= env->nr_cpus_online)
+	if (cpu >= env->nr_cpus_avail)
 		return -1;
 
 	return cpu;
diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c
index 98fe69ac553c..803f893550d6 100644
--- a/tools/perf/tests/topology.c
+++ b/tools/perf/tests/topology.c
@@ -65,7 +65,9 @@ static int check_cpu_topology(char *path, struct cpu_map *map)
 	session = perf_session__new(&file, false, NULL);
 	TEST_ASSERT_VAL("can't get session", session);
 
-	for (i = 0; i < session->header.env.nr_cpus_online; i++) {
+	for (i = 0; i < session->header.env.nr_cpus_avail; i++) {
+		if (!cpu_map__has(map, i))
+			continue;
 		pr_debug("CPU %d, core %d, socket %d\n", i,
 			 session->header.env.cpu[i].core_id,
 			 session->header.env.cpu[i].socket_id);
diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index bb964e86b09d..075fc77286bf 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -66,7 +66,7 @@ int perf_env__read_cpu_topology_map(struct perf_env *env)
 		return 0;
 
 	if (env->nr_cpus_avail == 0)
-		env->nr_cpus_avail = sysconf(_SC_NPROCESSORS_CONF);
+		env->nr_cpus_avail = cpu__max_present_cpu();
 
 	nr_cpus = env->nr_cpus_avail;
 	if (nr_cpus == -1)
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 4b0ea4e92e9d..e9384d17ae7d 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -293,11 +293,7 @@ static int write_nrcpus(int fd, struct perf_header *h __maybe_unused,
 	u32 nrc, nra;
 	int ret;
 
-	nr = sysconf(_SC_NPROCESSORS_CONF);
-	if (nr < 0)
-		return -1;
-
-	nrc = (u32)(nr & UINT_MAX);
+	nrc = cpu__max_present_cpu();
 
 	nr = sysconf(_SC_NPROCESSORS_ONLN);
 	if (nr < 0)
@@ -511,9 +507,7 @@ static struct cpu_topo *build_cpu_topology(void)
 	int ret = 0;
 	struct cpu_map *map;
 
-	ncpus = sysconf(_SC_NPROCESSORS_CONF);
-	if (ncpus < 0)
-		return NULL;
+	ncpus = cpu__max_present_cpu();
 
 	/* build online CPU map */
 	map = cpu_map__new(NULL);
@@ -1137,7 +1131,7 @@ static void print_cpu_topology(struct perf_header *ph, int fd __maybe_unused,
 {
 	int nr, i;
 	char *str;
-	int cpu_nr = ph->env.nr_cpus_online;
+	int cpu_nr = ph->env.nr_cpus_avail;
 
 	nr = ph->env.nr_sibling_cores;
 	str = ph->env.sibling_cores;
@@ -1792,7 +1786,7 @@ static int process_cpu_topology(struct perf_file_section *section,
 	u32 nr, i;
 	char *str;
 	struct strbuf sb;
-	int cpu_nr = ph->env.nr_cpus_online;
+	int cpu_nr = ph->env.nr_cpus_avail;
 	u64 size = 0;
 
 	ph->env.cpu = calloc(cpu_nr, sizeof(*ph->env.cpu));
@@ -1873,7 +1867,7 @@ static int process_cpu_topology(struct perf_file_section *section,
 		if (ph->needs_swap)
 			nr = bswap_32(nr);
 
-		if (nr > (u32)cpu_nr) {
+		if (nr != (u32)-1 && nr > (u32)cpu_nr) {
 			pr_debug("socket_id number is too big."
 				 "You may need to upgrade the perf tool.\n");
 			goto free_cpu;
-- 
1.8.3.1

Re: [PATCH v2 1/3] perf: add cpu__max_present_cpu()

[Jiri Olsa]

On Mon, Feb 13, 2017 at 04:34:34PM +0100, Jan Stancek wrote:
> 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;

I think it'd be less confusing for me if we follow the kernel names here

 static int max_cpu_possible;
 static int max_cpu_present;

also I wonder we should use 

 static int max_cpu_online;

instead of:

 sysconf(_SC_NPROCESSORS_ONLN)

but we can do it all later on as follow up

jirka

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

[Jiri Olsa]

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

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

[Jiri Olsa]

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;

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

[Jiri Olsa]

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