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


Groups > linux.kernel > #1570002 > unrolled thread

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

Started byJiri Olsa <jolsa@redhat.com>
First post2017-01-30 19:50 +0100
Last post2017-02-02 12:40 +0100
Articles 20 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  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 v2 2/3] perf: make build_cpu_topology skip offline/absent  CPUs Jiri Olsa <jolsa@redhat.com> - 2017-02-14 12:10 +0100
                  Re: [PATCH v2 2/3] perf: make build_cpu_topology skip  offline/absent CPUs Jan Stancek <jstancek@redhat.com> - 2017-02-15 09:50 +0100
                    [PATCH v3 1/3] perf: add cpu__max_present_cpu() Jan Stancek <jstancek@redhat.com> - 2017-02-17 12:20 +0100
                      [PATCH v3 2/3] perf: make build_cpu_topology skip offline/absent CPUs Jan Stancek <jstancek@redhat.com> - 2017-02-17 12:20 +0100
                        Re: [PATCH v3 2/3] perf: make build_cpu_topology skip offline/absent  CPUs Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-17 16:40 +0100
                        [tip:perf/urgent] perf header: Make build_cpu_topology skip  offline/absent CPUs tip-bot for Jan Stancek <tipbot@zytor.com> - 2017-02-21 09:20 +0100
                      [PATCH v3 3/3] perf: replace _SC_NPROCESSORS_CONF with max_present_cpu in cpu_topology_map Jan Stancek <jstancek@redhat.com> - 2017-02-17 12:20 +0100
                        Re: [PATCH v3 3/3] perf: replace _SC_NPROCESSORS_CONF with  max_present_cpu in cpu_topology_map Jiri Olsa <jolsa@redhat.com> - 2017-02-17 16:10 +0100
                        [tip:perf/urgent] perf tools: Replace _SC_NPROCESSORS_CONF with  max_present_cpu in cpu_topology_map tip-bot for Jan Stancek <tipbot@zytor.com> - 2017-02-21 09:20 +0100
                      [tip:perf/urgent] perf cpumap: Add cpu__max_present_cpu() tip-bot for Jan Stancek <tipbot@zytor.com> - 2017-02-21 09:20 +0100
              Re: [PATCH v2 1/3] perf: add cpu__max_present_cpu() Jiri Olsa <jolsa@redhat.com> - 2017-02-14 12:20 +0100
      Re: [PATCH] perf: fix topology test on systems with sparse CPUs Jiri Olsa <jolsa@redhat.com> - 2017-02-02 12:40 +0100

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

FromJiri Olsa <jolsa@redhat.com>
Date2017-01-30 19:50 +0100
SubjectRe: [PATCH] perf: fix topology test on systems with sparse CPUs
Message-ID<t5pbz-7p2-7@gated-at.bofh.it>
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] | [next] | [standalone]


#1570058

FromJan Stancek <jstancek@redhat.com>
Date2017-01-30 20:30 +0100
Message-ID<t5pOj-7RS-21@gated-at.bofh.it>
In reply to#1570002



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

FromJan Stancek <jstancek@redhat.com>
Date2017-01-31 17:10 +0100
Message-ID<t5Jai-2Mv-15@gated-at.bofh.it>
In reply to#1570002

[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

FromJiri Olsa <jolsa@redhat.com>
Date2017-02-02 12:30 +0100
Message-ID<t6nKp-3ij-3@gated-at.bofh.it>
In reply to#1570854
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

FromJan Stancek <jstancek@redhat.com>
Date2017-02-02 13:10 +0100
Message-ID<t6on8-3Ln-5@gated-at.bofh.it>
In reply to#1572287
> 
> > 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

FromJiri Olsa <jolsa@redhat.com>
Date2017-02-02 14:10 +0100
Message-ID<t6pjc-4nr-21@gated-at.bofh.it>
In reply to#1572310
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()

FromJan Stancek <jstancek@redhat.com>
Date2017-02-13 16:40 +0100
Subject[PATCH v2 1/3] perf: add cpu__max_present_cpu()
Message-ID<taqTn-5xK-17@gated-at.bofh.it>
In reply to#1572345
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

FromJan Stancek <jstancek@redhat.com>
Date2017-02-13 16:40 +0100
Subject[PATCH v2 2/3] perf: make build_cpu_topology skip offline/absent CPUs
Message-ID<taqTq-5xK-75@gated-at.bofh.it>
In reply to#1579872
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]


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

FromJiri Olsa <jolsa@redhat.com>
Date2017-02-14 12:10 +0100
SubjectRe: [PATCH v2 2/3] perf: make build_cpu_topology skip offline/absent CPUs
Message-ID<taJ9D-186-9@gated-at.bofh.it>
In reply to#1579885
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
> 

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


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

FromJan Stancek <jstancek@redhat.com>
Date2017-02-15 09:50 +0100
SubjectRe: [PATCH v2 2/3] perf: make build_cpu_topology skip offline/absent CPUs
Message-ID<tb3rH-62y-3@gated-at.bofh.it>
In reply to#1580478

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

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


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

FromJan Stancek <jstancek@redhat.com>
Date2017-02-17 12:20 +0100
Subject[PATCH v3 1/3] perf: add cpu__max_present_cpu()
Message-ID<tbOJY-3B2-17@gated-at.bofh.it>
In reply to#1581126
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]


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

FromJan Stancek <jstancek@redhat.com>
Date2017-02-17 12:20 +0100
Subject[PATCH v3 2/3] perf: make build_cpu_topology skip offline/absent CPUs
Message-ID<tbOJY-3B2-21@gated-at.bofh.it>
In reply to#1583337
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

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


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

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2017-02-17 16:40 +0100
SubjectRe: [PATCH v3 2/3] perf: make build_cpu_topology skip offline/absent CPUs
Message-ID<tbSNA-63T-23@gated-at.bofh.it>
In reply to#1583338
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

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


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

Fromtip-bot for Jan Stancek <tipbot@zytor.com>
Date2017-02-21 09:20 +0100
Subject[tip:perf/urgent] perf header: Make build_cpu_topology skip offline/absent CPUs
Message-ID<tddPY-8nM-5@gated-at.bofh.it>
In reply to#1583338
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;

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


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

FromJan Stancek <jstancek@redhat.com>
Date2017-02-17 12:20 +0100
Subject[PATCH v3 3/3] perf: replace _SC_NPROCESSORS_CONF with max_present_cpu in cpu_topology_map
Message-ID<tbOJY-3B2-19@gated-at.bofh.it>
In reply to#1583337
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

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


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

FromJiri Olsa <jolsa@redhat.com>
Date2017-02-17 16:10 +0100
SubjectRe: [PATCH v3 3/3] perf: replace _SC_NPROCESSORS_CONF with max_present_cpu in cpu_topology_map
Message-ID<tbSky-5TK-35@gated-at.bofh.it>
In reply to#1583341
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

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


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

Fromtip-bot for Jan Stancek <tipbot@zytor.com>
Date2017-02-21 09:20 +0100
Subject[tip:perf/urgent] perf tools: Replace _SC_NPROCESSORS_CONF with max_present_cpu in cpu_topology_map
Message-ID<tddPZ-8nM-41@gated-at.bofh.it>
In reply to#1583341
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;

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


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

Fromtip-bot for Jan Stancek <tipbot@zytor.com>
Date2017-02-21 09:20 +0100
Subject[tip:perf/urgent] perf cpumap: Add cpu__max_present_cpu()
Message-ID<tddPY-8nM-9@gated-at.bofh.it>
In reply to#1583337
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,

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


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

FromJiri Olsa <jolsa@redhat.com>
Date2017-02-14 12:20 +0100
SubjectRe: [PATCH v2 1/3] perf: add cpu__max_present_cpu()
Message-ID<taJjj-1cM-3@gated-at.bofh.it>
In reply to#1579872
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

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


#1572297

FromJiri Olsa <jolsa@redhat.com>
Date2017-02-02 12:40 +0100
Message-ID<t6nU5-3lz-25@gated-at.bofh.it>
In reply to#1570854
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] | [standalone]


Back to top | Article view | linux.kernel


csiph-web