perf_env/CPU socket reorg/fixes

tools/perf/util/header.c index renamed from tools/perf/util/header.c +0 -0

151b8310ac70..d4c8aa2f4db7 100644

tools/perf/util/header.c +0 -2

@@ -415,8 +415,6 @@ struct cpu_topo {
 	u32 thread_sib;
 	char **core_siblings;
 	char **thread_siblings;
-	int *core_id;
-	int *phy_pkg_id;
 };
 
 static int build_cpu_topo(struct cpu_topo *tp, int cpu) @@ -479,9 +477,6

@@ try_threads:

tools/perf/util/header.c +3 -11

 	}
 	ret = 0;
 done:
-	tp->core_id[cpu] = cpu_map__get_core_id(cpu);
-	tp->phy_pkg_id[cpu] = cpu_map__get_socket_id(cpu);
-
 	if(fp)
 		fclose(fp);
 	free(buf);

@@ -509,7 +504,7 @@ static struct cpu_topo *build_cpu_topology(void)
 	struct cpu_topo *tp;
 	void *addr;
 	u32 nr, i;
-	size_t sz, sz_id;
+	size_t sz;
 	long ncpus;
 	int ret = -1;
 

@@ -520,9 +515,8 @@ static struct cpu_topo *build_cpu_topology(void)
 	nr = (u32)(ncpus & UINT_MAX);
 
 	sz = nr * sizeof(char *);
-	sz_id = nr * sizeof(int);
 
-	addr = calloc(1, sizeof(*tp) + 2 * sz + 2 * sz_id);
+	addr = calloc(1, sizeof(*tp) + 2 * sz);
 	if (!addr)
 		return NULL;
 

@@ -532,10 +526,6 @@ static struct cpu_topo *build_cpu_topology(void)
 	tp->core_siblings = addr;
 	addr += sz;
 	tp->thread_siblings = addr;
-	addr += sz;
-	tp->core_id = addr;
-	addr += sz_id;
-	tp->phy_pkg_id = addr;
 
 	for (i = 0; i < nr; i++) {
 		ret = build_cpu_topo(tp, i);

@@ -554,7 +544,7 @@ static int write_cpu_topology(int fd, struct
 perf_header *h __maybe_unused,  {
 	struct cpu_topo *tp;
 	u32 i;
-	int ret;
+	int ret, j;
 
 	tp = build_cpu_topology();
 	if (!tp)

Review

Quoted 4 lines

(quoted diff) +2 -2

-	for (i = 0; i < tp->cpu_nr; i++) {
-		ret = do_write(fd, &tp->core_id[i], sizeof(int));
+	perf_env__read_cpu_topology_map(&perf_env);
+

I think we need to handle error here.

Em Wed, Sep 09, 2015 at 09:41:18PM +0000, Liang, Kan escreveu:

Quoted 6 lines

tools/perf/util/header.c static int write_cpu_topology(int fd, struct perf_header *h __maybe_unused, · line 575

Quoted 4 lines

tools/perf/util/header.c +2 -2

-	for (i = 0; i < tp->cpu_nr; i++) {
-		ret = do_write(fd, &tp->core_id[i], sizeof(int));
+	perf_env__read_cpu_topology_map(&perf_env);
+

I think we need to handle error here.

Ok, adding a test, updated patch below. While doing that, noticed that the in the thread_sib case the "ret = do_something()" doesn't makes the whole function fail, will fix later.

- Arnaldo

commit c98989f4a47a6fd3f3f8bd937b96faaced8b203e

Author: Arnaldo Carvalho de Melo <[email protected]>
Date:   Wed Sep 9 10:37:01 2015 -0300

perf env: Introduce read_cpu_topology_map() method

Out of the code to write the cpu topology map in the perf.data file header.

Now if one needs the CPU topology map for the running machine, one
needs
to call perf_env__read_cpu_topology_map(perf_env) and the info will
be
stored in perf_env.cpu.

For now we're using a global perf_env variable, that will have its contents freed after we run a builtin.

v2: Check perf_env__read_cpu_topology_map() return in
write_cpu_topology() (Kan Liang)

Cc Adrian Hunter <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc David Ahern <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Kan Liang <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Wang Nan <[email protected]>

Link http://lkml.kernel.org/r/1441828225-667-5-git-send-email-

[email protected]

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

tools/perf/perf.c index renamed from tools/perf/perf.c +0 -0

f2fc019b3671..1fded922bcc8 100644

tools/perf/perf.c +2 -0

@@ -8,6 +8,7 @@
  */
 #include "builtin.h"
 
+#include "util/env.h"
 #include "util/exec_cmd.h"
 #include "util/cache.h"
 #include "util/quote.h"

@@ -369,6 +370,7 @@ static int run_builtin(struct cmd_struct *p, int argc,
 const char **argv)
 
 	status = p->fn(argc, argv, prefix);
 	exit_browser(status);
+	perf_env__exit(&perf_env);
 
 	if (status)

tools/perf/perf.c +0 -0

 		return status & 0xff;

tools/perf/util/env.c index renamed from tools/perf/util/env.c +0 -0

ca1e33a2203e..6af4f7c36820 100644

tools/perf/util/env.c +22 -0

@@ -1,3 +1,4 @@
+#include "cpumap.h"
 #include "env.h"
 #include "util.h"
 

@@ -56,3 +57,30 @@ out_free:
 out_enomem:
 	return -ENOMEM;
 }
+
+int perf_env__read_cpu_topology_map(struct perf_env *env) {
+	int cpu, nr_cpus;
+
+	if (env->cpu != NULL)
+		return 0;
+
+	if (env->nr_cpus_avail == 0)
+		env->nr_cpus_avail = sysconf(_SC_NPROCESSORS_CONF);
+
+	nr_cpus = env->nr_cpus_avail;
+	if (nr_cpus == -1)
+		return -EINVAL;
+
+	env->cpu = calloc(nr_cpus, sizeof(env->cpu[0]));
+	if (env->cpu == NULL)
+		return -ENOMEM;
+
+	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);

tools/perf/util/env.c +5 -0

+	}
+
+	env->nr_cpus_avail = nr_cpus;
+	return 0;
+}

tools/perf/util/env.h index renamed from tools/perf/util/env.h +0 -0

70124d9a1624..c4e36323d91e 100644

tools/perf/util/env.h +2 -0

@@ -38,4 +38,6 @@ void perf_env__exit(struct perf_env *env);
 
 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);
+

#endif /* __PERF_ENV_H */

tools/perf/util/header.c index renamed from tools/perf/util/header.c +0 -0

151b8310ac70..d6437465f70f 100644

tools/perf/util/header.c +0 -2

@@ -415,8 +415,6 @@ struct cpu_topo {
 	u32 thread_sib;
 	char **core_siblings;
 	char **thread_siblings;
-	int *core_id;
-	int *phy_pkg_id;
 };
 
 static int build_cpu_topo(struct cpu_topo *tp, int cpu) @@ -479,9 +477,6

@@ try_threads:

tools/perf/util/header.c +12 -14

 	}
 	ret = 0;
 done:
-	tp->core_id[cpu] = cpu_map__get_core_id(cpu);
-	tp->phy_pkg_id[cpu] = cpu_map__get_socket_id(cpu);
-
 	if(fp)
 		fclose(fp);
 	free(buf);

@@ -509,7 +504,7 @@ static struct cpu_topo *build_cpu_topology(void)
 	struct cpu_topo *tp;
 	void *addr;
 	u32 nr, i;
-	size_t sz, sz_id;
+	size_t sz;
 	long ncpus;
 	int ret = -1;
 

@@ -520,9 +515,8 @@ static struct cpu_topo *build_cpu_topology(void)
 	nr = (u32)(ncpus & UINT_MAX);
 
 	sz = nr * sizeof(char *);
-	sz_id = nr * sizeof(int);
 
-	addr = calloc(1, sizeof(*tp) + 2 * sz + 2 * sz_id);
+	addr = calloc(1, sizeof(*tp) + 2 * sz);
 	if (!addr)
 		return NULL;
 

@@ -532,10 +526,6 @@ static struct cpu_topo *build_cpu_topology(void)
 	tp->core_siblings = addr;
 	addr += sz;
 	tp->thread_siblings = addr;
-	addr += sz;
-	tp->core_id = addr;
-	addr += sz_id;
-	tp->phy_pkg_id = addr;
 
 	for (i = 0; i < nr; i++) {
 		ret = build_cpu_topo(tp, i);

@@ -554,7 +544,7 @@ static int write_cpu_topology(int fd, struct
 perf_header *h __maybe_unused,  {
 	struct cpu_topo *tp;
 	u32 i;
-	int ret;
+	int ret, j;
 
 	tp = build_cpu_topology();
 	if (!tp)

@@ -579,11 +569,17 @@ static int write_cpu_topology(int fd, struct
 perf_header *h __maybe_unused,
 			break;
 	}
 
-	for (i = 0; i < tp->cpu_nr; i++) {
-		ret = do_write(fd, &tp->core_id[i], sizeof(int));
+	ret = perf_env__read_cpu_topology_map(&perf_env);
+	if (ret < 0)
+		goto done;
+
+	for (j = 0; j < perf_env.nr_cpus_avail; j++) {
+		ret = do_write(fd, &perf_env.cpu[j].core_id,
+			       sizeof(perf_env.cpu[j].core_id));
 		if (ret < 0)
 			return ret;
-		ret = do_write(fd, &tp->phy_pkg_id[i], sizeof(int));
+		ret = do_write(fd, &perf_env.cpu[j].socket_id,
+			       sizeof(perf_env.cpu[j].socket_id));
 		if (ret < 0)
 			return ret;

}

Quoted 7 lines

Em Wed, Sep 09, 2015 at 09:41:18PM +0000, Liang, Kan escreveu:

Quoted 6 lines

tools/perf/util/header.c static int write_cpu_topology(int fd, struct perf_header *h __maybe_unused, · line 575

Quoted 4 lines

tools/perf/util/header.c +2 -2

-	for (i = 0; i < tp->cpu_nr; i++) {
-		ret = do_write(fd, &tp->core_id[i], sizeof(int));
+	perf_env__read_cpu_topology_map(&perf_env);
+

I think we need to handle error here.

Quoted 3 lines

Ok, adding a test, updated patch below. While doing that, noticed that the in the thread_sib case the "ret = do_something()" doesn't makes the whole function fail, will fix later.

Except patch 11 & 13 (for reverting), the rest of patches are good to me.

Em Thu, Sep 10, 2015 at 08:00:54PM +0000, Liang, Kan escreveu:

Quoted 7 lines

Quoted 7 lines

Em Wed, Sep 09, 2015 at 09:41:18PM +0000, Liang, Kan escreveu:

Quoted 6 lines

tools/perf/util/header.c static int write_cpu_topology(int fd, struct perf_header *h __maybe_unused, · line 575

Quoted 4 lines

tools/perf/util/header.c +2 -2

-	for (i = 0; i < tp->cpu_nr; i++) {
-		ret = do_write(fd, &tp->core_id[i], sizeof(int));
+	perf_env__read_cpu_topology_map(&perf_env);
+

I think we need to handle error here.

Quoted 5 lines

Quoted 3 lines

Ok, adding a test, updated patch below. While doing that, noticed that the in the thread_sib case the "ret = do_something()" doesn't makes the whole function fail, will fix later.

Except patch 11 & 13 (for reverting), the rest of patches are good to me.

Ok, so, to take it more formally, can I turn this "good to me" as justification for adding "Acked-by: Kan Liang", to those patches?

Please take a look at the other patches in the perf/env branch, I did it while trying to figure it out if the reverts should be kept.

- Arnaldo

tools/perf/util/env.c +13 -0

+
+int perf_env__read_cpu_topology_map(struct perf_env *env)
+{
+	int cpu, nr_cpus;
+
+	if (env->cpu != NULL)
+		return 0;
+
+	if (env->nr_cpus_avail == 0)
+		env->nr_cpus_avail = sysconf(_SC_NPROCESSORS_CONF);
+
+	nr_cpus = env->nr_cpus_avail;
+	if (nr_cpus == -1)

tools/perf/util/env.c +4 -0

+
+	env->nr_cpus_avail = nr_cpus;
+	return 0;
+}

tools/perf/util/env.h +2 -0

index 70124d9a1624..c4e36323d91e 100644
@@ -38,4 +38,6 @@ void perf_env__exit(struct perf_env *env);
 
  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);
+
  #endif /* __PERF_ENV_H */

tools/perf/util/header.c +12 -16

index 151b8310ac70..d6437465f70f 100644
@@ -415,8 +415,6 @@ struct cpu_topo {
  	u32 thread_sib;
  	char **core_siblings;
  	char **thread_siblings;
-	int *core_id;
-	int *phy_pkg_id;
  };
 
  static int build_cpu_topo(struct cpu_topo *tp, int cpu)

@@ -479,9 +477,6 @@ try_threads:
  	}
  	ret = 0;
  done:
-	tp->core_id[cpu] = cpu_map__get_core_id(cpu);
-	tp->phy_pkg_id[cpu] = cpu_map__get_socket_id(cpu);
-
  	if(fp)
  		fclose(fp);
  	free(buf);

@@ -509,7 +504,7 @@ static struct cpu_topo *build_cpu_topology(void)
  	struct cpu_topo *tp;
  	void *addr;
  	u32 nr, i;
-	size_t sz, sz_id;
+	size_t sz;
  	long ncpus;
  	int ret = -1;
 

@@ -520,9 +515,8 @@ static struct cpu_topo *build_cpu_topology(void)
  	nr = (u32)(ncpus & UINT_MAX);
 
  	sz = nr * sizeof(char *);
-	sz_id = nr * sizeof(int);
 
-	addr = calloc(1, sizeof(*tp) + 2 * sz + 2 * sz_id);
+	addr = calloc(1, sizeof(*tp) + 2 * sz);
  	if (!addr)
  		return NULL;
 

@@ -532,10 +526,6 @@ static struct cpu_topo *build_cpu_topology(void)
  	tp->core_siblings = addr;
  	addr += sz;
  	tp->thread_siblings = addr;
-	addr += sz;
-	tp->core_id = addr;
-	addr += sz_id;
-	tp->phy_pkg_id = addr;
 
  	for (i = 0; i < nr; i++) {
  		ret = build_cpu_topo(tp, i);

@@ -554,7 +544,7 @@ static int write_cpu_topology(int fd, struct perf_header *h __maybe_unused,
  {
  	struct cpu_topo *tp;
  	u32 i;
-	int ret;
+	int ret, j;
 
  	tp = build_cpu_topology();
  	if (!tp)

@@ -579,11 +569,17 @@ static int write_cpu_topology(int fd, struct perf_header *h __maybe_unused,
  			break;
  	}
 
-	for (i = 0; i < tp->cpu_nr; i++) {
-		ret = do_write(fd, &tp->core_id[i], sizeof(int));
+	ret = perf_env__read_cpu_topology_map(&perf_env);
+	if (ret < 0)
+		goto done;
+
+	for (j = 0; j < perf_env.nr_cpus_avail; j++) {
+		ret = do_write(fd, &perf_env.cpu[j].core_id,
+			       sizeof(perf_env.cpu[j].core_id));
  		if (ret < 0)
  			return ret;
-		ret = do_write(fd, &tp->phy_pkg_id[i], sizeof(int));
+		ret = do_write(fd, &perf_env.cpu[j].socket_id,
+			       sizeof(perf_env.cpu[j].socket_id));
  		if (ret < 0)
  			return ret;
  	}

[SNIP]

Quoted 17 lines

tools/perf/util/env.c +17 -0

+
+int perf_env__read_cpu_topology_map(struct perf_env *env)
+{
+	int cpu, nr_cpus;
+
+	if (env->cpu != NULL)
+		return 0;
+
+	if (env->nr_cpus_avail == 0)
+		env->nr_cpus_avail = sysconf(_SC_NPROCESSORS_CONF);
+
+	nr_cpus = env->nr_cpus_avail;
+	if (nr_cpus == -1)
+		return -EINVAL;
+
+	env->cpu = calloc(nr_cpus, sizeof(env->cpu[0]));
+	if (env->cpu == NULL)

tools/perf/util/env.c out_free: · line 82

Quoted 6 lines

tools/perf/util/env.c +6 -0

+		return -ENOMEM;
+
+	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);
+	}

Shouldn't we check the failure of these two functions?

At this point perf_env__read_cpu_topology_map and build_cpu_topology are doing
similar things. build_cpu_topology() reads /sys/xxxxx/cpu%d/topology/{core,thread}_siblings_list, perf_env__read_cpu_topology_map() reads /sys/xxxxx/cpu%d/topology/core_id, but build_cpu_topology() returns error if any read failed, but perf_env__read_cpu_topology_map() fills core_id and socket_id with -1 if read fail.

I tried to offline a core between build_cpu_topology() and perf_env__read_cpu_topology_map(),
and perf report say:

 # perf report -v --header-only -I
 build id event received for [kernel.kallsyms]: (...)
 core_id number is too big.You may need to upgrade the perf tool. <-- *see
this warning*
 # ========
 # captured on: Sun Feb 15 15:01:05 2009
 # hostname : localhost
 ...
 # sibling cores   : ...
 # sibling threads : 7
 # Core ID and Socket ID information is not available
 # pmu mappings: not available
 # ========

Quoted 89 lines

tools/perf/util/env.c +4 -0

+
+	env->nr_cpus_avail = nr_cpus;
+	return 0;
+}

tools/perf/util/env.h +2 -0

index 70124d9a1624..c4e36323d91e 100644
@@ -38,4 +38,6 @@ void perf_env__exit(struct perf_env *env);
 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);
+
 #endif /* __PERF_ENV_H */

tools/perf/util/header.c +12 -16

index 151b8310ac70..d6437465f70f 100644
@@ -415,8 +415,6 @@ struct cpu_topo {
 	u32 thread_sib;
 	char **core_siblings;
 	char **thread_siblings;
-	int *core_id;
-	int *phy_pkg_id;
 };
 static int build_cpu_topo(struct cpu_topo *tp, int cpu)

@@ -479,9 +477,6 @@ try_threads:
 	}
 	ret = 0;
 done:
-	tp->core_id[cpu] = cpu_map__get_core_id(cpu);
-	tp->phy_pkg_id[cpu] = cpu_map__get_socket_id(cpu);
-
 	if(fp)
 		fclose(fp);
 	free(buf);

@@ -509,7 +504,7 @@ static struct cpu_topo *build_cpu_topology(void)
 	struct cpu_topo *tp;
 	void *addr;
 	u32 nr, i;
-	size_t sz, sz_id;
+	size_t sz;
 	long ncpus;
 	int ret = -1;

@@ -520,9 +515,8 @@ static struct cpu_topo *build_cpu_topology(void)
 	nr = (u32)(ncpus & UINT_MAX);
 	sz = nr * sizeof(char *);
-	sz_id = nr * sizeof(int);
-	addr = calloc(1, sizeof(*tp) + 2 * sz + 2 * sz_id);
+	addr = calloc(1, sizeof(*tp) + 2 * sz);
 	if (!addr)
 		return NULL;

@@ -532,10 +526,6 @@ static struct cpu_topo *build_cpu_topology(void)
 	tp->core_siblings = addr;
 	addr += sz;
 	tp->thread_siblings = addr;
-	addr += sz;
-	tp->core_id = addr;
-	addr += sz_id;
-	tp->phy_pkg_id = addr;
 	for (i = 0; i < nr; i++) {
 		ret = build_cpu_topo(tp, i);

@@ -554,7 +544,7 @@ static int write_cpu_topology(int fd, struct perf_header *h __maybe_unused,
 {
 	struct cpu_topo *tp;
 	u32 i;
-	int ret;
+	int ret, j;
 	tp = build_cpu_topology();
 	if (!tp)

@@ -579,11 +569,17 @@ static int write_cpu_topology(int fd, struct perf_header *h __maybe_unused,
 			break;
 	}
-	for (i = 0; i < tp->cpu_nr; i++) {
-		ret = do_write(fd, &tp->core_id[i], sizeof(int));
+	ret = perf_env__read_cpu_topology_map(&perf_env);
+	if (ret < 0)
+		goto done;
+
+	for (j = 0; j < perf_env.nr_cpus_avail; j++) {
+		ret = do_write(fd, &perf_env.cpu[j].core_id,
+			       sizeof(perf_env.cpu[j].core_id));
 		if (ret < 0)
 			return ret;
-		ret = do_write(fd, &tp->phy_pkg_id[i], sizeof(int));
+		ret = do_write(fd, &perf_env.cpu[j].socket_id,
+			       sizeof(perf_env.cpu[j].socket_id));
 		if (ret < 0)
 			return ret;
 	}

So this is a problem before and after my patches, i.e. If I go on and do, with what we have in acme/perf/core, i.e. none of the changes I'm playing with in perf/env:

  $ git remote update acme
  Fetching acme
  $ git checkout -b tmp acme/perf/core
  Branch tmp set up to track remote branch perf/core from acme.
  Switched to a new branch 'tmp'
  $ git log --oneline | head -5
  7e150fb33a91 perf tests: Introduce iterator function for tests
  1765d9b26f84 perf test: Add entry for hists socket filter
  207bb55e9193 perf hists browser: Zoom in/out for processor socket
  9a2843a5f421 perf report: Introduce --socket-filter option
  99851c76436a perf tools: Introduce new sort type "socket" for the processor socket
  $ rm -rf /tmp/build/perf ; mkdir -p /tmp/build/perf ; make DEBUG=1 -C tools/perf O=/tmp/build/perf install-bin
  # echo 0 > /sys/devices/system/cpu/cpu2/online
  $ cat /sys/devices/system/cpu/cpu2/topology/core_id
  cat: /sys/devices/system/cpu/cpu2/topology/core_id: No such file or directory
  $ ls -la /sys/devices/system/cpu/cpu2/topology/
  ls: cannot access /sys/devices/system/cpu/cpu2/topology/: No such file or directory
  $ perf record usleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.012 MB perf.data (7 samples) ]
  [acme@felicio linux]$ perf report --header-only -I 
  # ========
  # captured on: Fri Sep 11 11:34:18 2015
  # hostname : felicio.ghostprotocols.net
  # os release : 4.2.0
  # perf version : 4.2.g7e150f
  #  arch : x86_64

  # nrcpus online : 4
  # nrcpus avail : 3
  # cpudesc : Intel(R) Core(TM) i5-2400 CPU @ 3.10GHz
  <SNIP>
  # node0 meminfo  : total = 8085412 kB, free = 5317596 kB
  # node0 cpu list : 0-1,3
  <SNIP>
  $ 

We can see multiple bugs here, right? online/avail is swapped, and when online != avail we simply do not record the cpu topology info at all!

If we get that CPU back online:

  # echo 1 > /sys/devices/system/cpu/cpu2/online

Then all works:

  $ perf record usleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.012 MB perf.data (7 samples) ]
  $ perf report --header-only -I
  # ========
  # captured on: Fri Sep 11 11:37:31 2015
  # hostname : felicio.ghostprotocols.net
  # os release : 4.2.0
  # perf version : 4.2.g7e150f
  # arch : x86_64
  # nrcpus online : 4
  # nrcpus avail : 4
  # cpudesc : Intel(R) Core(TM) i5-2400 CPU @ 3.10GHz
  <SNIP>
  # sibling cores   : 0-3
  # sibling threads : 0
  # sibling threads : 1
  # sibling threads : 2
  # sibling threads : 3
  # CPU 0: Core ID 0, Socket ID 0
  # CPU 1: Core ID 1, Socket ID 0
  # CPU 2: Core ID 2, Socket ID 0
  # CPU 3: Core ID 3, Socket ID 0
  # node0 meminfo  : total = 8085412 kB, free = 5316992 kB
  # node0 cpu list : 0-3

So, again, this was not introduced by this patchkit, but it is good that you did these offline tests, so we can fix it!

Em Fri, Sep 11, 2015 at 11:40:28AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 31 lines

So this is a problem before and after my patches, i.e. If I go on and do, with what we have in acme/perf/core, i.e. none of the changes I'm playing with in perf/env:

  $ git remote update acme
  Fetching acme
  $ git checkout -b tmp acme/perf/core
  Branch tmp set up to track remote branch perf/core from acme.
  Switched to a new branch 'tmp'
  $ git log --oneline | head -5
  7e150fb33a91 perf tests: Introduce iterator function for tests
  1765d9b26f84 perf test: Add entry for hists socket filter
  207bb55e9193 perf hists browser: Zoom in/out for processor socket
  9a2843a5f421 perf report: Introduce --socket-filter option
  99851c76436a perf tools: Introduce new sort type "socket" for the processor socket
  $ rm -rf /tmp/build/perf ; mkdir -p /tmp/build/perf ; make DEBUG=1 -C tools/perf O=/tmp/build/perf install-bin
  # echo 0 > /sys/devices/system/cpu/cpu2/online
  $ cat /sys/devices/system/cpu/cpu2/topology/core_id
  cat: /sys/devices/system/cpu/cpu2/topology/core_id: No such file or directory
  $ ls -la /sys/devices/system/cpu/cpu2/topology/
  ls: cannot access /sys/devices/system/cpu/cpu2/topology/: No such file or directory
  $ perf record usleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.012 MB perf.data (7 samples) ]
  [acme@felicio linux]$ perf report --header-only -I 
  # ========
  # captured on: Fri Sep 11 11:34:18 2015
  # hostname : felicio.ghostprotocols.net
  # os release : 4.2.0
  # perf version : 4.2.g7e150f
  #  arch : x86_64

Quoted 8 lines

  # nrcpus online : 4
  # nrcpus avail : 3
  # cpudesc : Intel(R) Core(TM) i5-2400 CPU @ 3.10GHz
  <SNIP>
  # node0 meminfo  : total = 8085412 kB, free = 5317596 kB
  # node0 cpu list : 0-1,3
  <SNIP>
  $ 

Stephane, Namhyung, the bug report below is about the online/avail above, they are swapped, full explanation below.

Quoted 35 lines

We can see multiple bugs here, right? online/avail is swapped, and when online != avail we simply do not record the cpu topology info at all!

If we get that CPU back online:

  # echo 1 > /sys/devices/system/cpu/cpu2/online

Then all works:

  $ perf record usleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.012 MB perf.data (7 samples) ]
  $ perf report --header-only -I
  # ========
  # captured on: Fri Sep 11 11:37:31 2015
  # hostname : felicio.ghostprotocols.net
  # os release : 4.2.0
  # perf version : 4.2.g7e150f
  # arch : x86_64
  # nrcpus online : 4
  # nrcpus avail : 4
  # cpudesc : Intel(R) Core(TM) i5-2400 CPU @ 3.10GHz
  <SNIP>
  # sibling cores   : 0-3
  # sibling threads : 0
  # sibling threads : 1
  # sibling threads : 2
  # sibling threads : 3
  # CPU 0: Core ID 0, Socket ID 0
  # CPU 1: Core ID 1, Socket ID 0
  # CPU 2: Core ID 2, Socket ID 0
  # CPU 3: Core ID 3, Socket ID 0
  # node0 meminfo  : total = 8085412 kB, free = 5316992 kB
  # node0 cpu list : 0-3

Quoted 2 lines

So, again, this was not introduced by this patchkit, but it is good that you did these offline tests, so we can fix it!

Stephane, this was introduced in:

commit fbe96f29ce4b33e0a22219cc7f5996d9157717e3
Author: Stephane Eranian <[email protected]>
Date: Fri Sep 30 15:40:40 2011 +0200

perf tools: Make perf.data more self-descriptive (v8)

------------------

When you write this part:

- HEADER_NRCPUS: number of online/avail cpus

You do:

static int write_nrcpus(int fd, struct perf_header *h __used,
struct perf_evlist *evlist __used)

{
       long nr;
       u32 nrc, nra;
       int ret;

       nr = sysconf(_SC_NPROCESSORS_CONF);
       if (nr < 0)
               return -1;

       nrc = (u32)(nr & UINT_MAX);

       nr = sysconf(_SC_NPROCESSORS_ONLN);
       if (nr < 0)
               return -1;

       nra = (u32)(nr & UINT_MAX);

       ret = do_write(fd, &nrc, sizeof(nrc));
       if (ret < 0)
               return ret;

       return do_write(fd, &nra, sizeof(nra));
}

I.e. write what you called 'nrc' using what is in SC_NRPROCESSORS_CONF, that in the documentation for glibc reads:

---------------------

http://www.gnu.org/software/libc/manual/html_node/Processor-Resources.html

sysconf (_SC_NPROCESSORS_CONF)

which returns the number of processors the operating system configured. But it might be possible for the operating system to disable individual processors and so the call

---------------------

Which menas "NR_AVAILABLE", right?

But then you call a variable 'nra' which sounds like you think that what is in _SC_NPROCESSORS_ONLN is the "available" number of CPUs, which is confused a bit more by the glibc docs when refering to _SC_NPROCESSORS_ONLN:

---------------------
sysconf (_SC_NPROCESSORS_ONLN)

returns the number of processors which are currently online (i.e., available).
---------------------

Then, when printing the number of CPUs encoded in the perf.data file by the above write_nrcpus() routine, you did:

static void print_nrcpus(struct perf_header *ph, int fd, FILE *fp)

{
       ssize_t ret;
       u32 nr;

       ret = read(fd, &nr, sizeof(nr));
       if (ret != (ssize_t)sizeof(nr))
               nr = -1; /* interpreted as error */

       if (ph->needs_swap)
               nr = bswap_32(nr);

       fprintf(fp, "# nrcpus online : %u\n", nr);

       ret = read(fd, &nr, sizeof(nr));
       if (ret != (ssize_t)sizeof(nr))
               nr = -1; /* interpreted as error */

       if (ph->needs_swap)
               nr = bswap_32(nr);

       fprintf(fp, "# nrcpus avail : %u\n", nr);
}

You inverted it, no?

So, could you please check if the below patch can have your Acked-by? Namhyung?

Thanks,

- Arnaldo

tools/perf/util/header.c +2 -2

index 41814547da15..fce6634aebe2 100644
@@ -1438,7 +1438,7 @@ static int process_nrcpus(struct perf_file_section *section __maybe_unused,
 	if (ph->needs_swap)
 		nr = bswap_32(nr);
 
-	ph->env.nr_cpus_online = nr;
+	ph->env.nr_cpus_avail = nr;
 
 	ret = readn(fd, &nr, sizeof(nr));
 	if (ret != sizeof(nr))

@@ -1447,7 +1447,7 @@ static int process_nrcpus(struct perf_file_section *section __maybe_unused,
 	if (ph->needs_swap)
 		nr = bswap_32(nr);
 
-	ph->env.nr_cpus_avail = nr;
+	ph->env.nr_cpus_online = nr;
 	return 0;
 }
 

Quoted 4 lines

You inverted it, no?

So, could you please check if the below patch can have your Acked-by? Namhyung?

Looks good to me.

Acked-by Namhyung Kim <[email protected]>

From: Arnaldo Carvalho de Melo <[email protected]>

Since we were not setting it to at least 3 chars ('CPU'), it was being reset to zero when recalculating the columns width when refreshing the screen, in 'perf top'. Fix it.

Cc Adrian Hunter <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc David Ahern <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Kan Liang <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Wang Nan <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
  tools/perf/util/hist.c | 1 +
  1 file changed, 1 insertion(+)

tools/perf/util/hist.c +1 -0

index 67b48616ab31..b3567a25f0c4 100644
@@ -146,6 +146,7 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
  		hists__set_unres_dso_col_len(hists, HISTC_MEM_DADDR_DSO);
  	}
 
+	hists__new_col_len(hists, HISTC_CPU, 3);
  	hists__new_col_len(hists, HISTC_SOCKET, 6);
  	hists__new_col_len(hists, HISTC_MEM_LOCKED, 6);
  	hists__new_col_len(hists, HISTC_MEM_TLB, 22);

From: Arnaldo Carvalho de Melo <[email protected]>

As al.cpu may be -1, i.e. no PERF_SAMPLE_CPU, and env->cpu may be NULL.

Rely instead on the work now done in perf_event__preprocess_sample(), that does all those checks.

Reported-by Wang Nan <[email protected]>

Based-on-a-patch-by Jiri Olsa <[email protected]>

Cc Adrian Hunter <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc David Ahern <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Kan Liang <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Wang Nan <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
  tools/perf/builtin-report.c | 4 ----
  1 file changed, 4 deletions(-)

tools/perf/builtin-report.c +0 -1

index 4b432453922f..9b5083630a56 100644
@@ -150,7 +150,6 @@ static int process_sample_event(struct perf_tool *tool,
  		.add_entry_cb 		= hist_iter__report_callback,
  	};
  	int ret = 0;
-	struct perf_env *env = evsel->evlist->env;
 
  	if (perf_event__preprocess_sample(event, machine, &al, sample) < 0) {
  		pr_debug("problem processing %d event, skipping it.\n",

From: Arnaldo Carvalho de Melo <[email protected]>

This reverts commit d49e4695077278ee3016cd242967de23072ec331.

We don't need it, using machine->env seems to be enough.

Cc Adrian Hunter <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc David Ahern <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Kan Liang <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Wang Nan <[email protected]>

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/evlist.c | 2 --
 tools/perf/util/evsel.c  | 2 --
 tools/perf/util/evsel.h  | 4 ----
 3 files changed, 8 deletions(-)

tools/perf/util/evlist.c index renamed from tools/perf/util/evlist.c +0 -0

d51a5200c8af..09b4ec221dda 100644

tools/perf/util/evlist.c +0 -2

@@ -98,7 +98,6 @@ static void perf_evlist__purge(struct perf_evlist
 *evlist)
 
 	evlist__for_each_safe(evlist, n, pos) {
 		list_del_init(&pos->node);
-		pos->evlist = NULL;
 		perf_evsel__delete(pos);
 	}

@@ -126,7 +125,6 @@ void perf_evlist__delete(struct perf_evlist *evlist)
 
 void perf_evlist__add(struct perf_evlist *evlist, struct perf_evsel *entry)
 {
-	entry->evlist = evlist;
 	list_add_tail(&entry->node, &evlist->entries);
 	entry->idx = evlist->nr_entries;
 	entry->tracking = !entry->idx;

tools/perf/util/evsel.c index renamed from tools/perf/util/evsel.c +0 -0

771ade4d5966..8f4d45002461 100644

tools/perf/util/evsel.c +0 -2

@@ -206,7 +206,6 @@ void perf_evsel__init(struct perf_evsel *evsel,
 	evsel->leader	   = evsel;
 	evsel->unit	   = "";
 	evsel->scale	   = 1.0;
-	evsel->evlist	   = NULL;
 	INIT_LIST_HEAD(&evsel->node);
 	INIT_LIST_HEAD(&evsel->config_terms);
 	perf_evsel__object.init(evsel);

@@ -1027,7 +1026,6 @@ void perf_evsel__close_fd(struct perf_evsel
 *evsel, int ncpus, int nthreads)  void perf_evsel__exit(struct perf_evsel
 *evsel)  {
 	assert(list_empty(&evsel->node));
-	assert(evsel->evlist == NULL);
 	perf_evsel__free_fd(evsel);
 	perf_evsel__free_id(evsel);
 	perf_evsel__free_config_terms(evsel);

tools/perf/util/evsel.h index renamed from tools/perf/util/evsel.h +0 -0

298e6bbca200..93ac6b128149 100644

tools/perf/util/evsel.h +0 -3

@@ -60,9 +60,6 @@ struct perf_evsel_config_term {
 
 /** struct perf_evsel - event selector
  *
- * @evlist - evlist this evsel is in, if it is in one.
- * @node - To insert it into evlist->entries or in other list_heads, say in
- *         the event parsing routines.
  * @name - Can be set to retain the original event name passed by the
 user,
  *         so that when showing results in tools such as 'perf stat', we

* show the name used, not some alias.

tools/perf/util/evsel.h +0 -2

@@ -76,7 +73,6 @@ struct perf_evsel_config_term {
  */
 struct perf_evsel {
 	struct list_head	node;
-	struct perf_evlist	*evlist;
 	struct perf_event_attr	attr;
 	char			*filter;
 	struct xyarray		*fd;
--

2.1.0

Hi,

Please take a look at these changes to fix the problems reported by
Wang Nan wrt accesses to the cpu_topology_map information.

The fixes are present on these following two csets:

perf event: Use machine->env to find the cpu -> socket mapping perf report: Do not blindly use env->cpu[al.cpu].socket_id

The rest are fixes made while working on this, infrastructure to enable
the fixes, reverts for things that ended up not being necessary and some cleanups.

It is available at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/env

Please let me know if I can have your Acked-by, Tested-by or
Reviewed-by.

Quoted 12 lines

Please take a look at these changes to fix the problems reported by
Wang Nan wrt accesses to the cpu_topology_map information.

The fixes are present on these following two csets:

perf event: Use machine->env to find the cpu -> socket mapping perf report: Do not blindly use env->cpu[al.cpu].socket_id

The rest are fixes made while working on this, infrastructure to enable
the fixes, reverts for things that ended up not being necessary and some cleanups.

Quoted 6 lines

It is available at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/env

Please let me know if I can have your Acked-by, Tested-by or
Reviewed-by.

perf_env holds the data for perf.data session.. is the plan to keep

it like this, or it's to be used in some other way? Moving it out of session/header real suggest that.. just asking ;-)

Hi,

Please take a look at these changes to fix the problems reported by
Wang Nan wrt accesses to the cpu_topology_map information.

The fixes are present on these following two csets:

perf event: Use machine->env to find the cpu -> socket mapping perf report: Do not blindly use env->cpu[al.cpu].socket_id

The rest are fixes made while working on this, infrastructure to enable
the fixes, reverts for things that ended up not being necessary and some cleanups.

It is available at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/env

Please let me know if I can have your Acked-by, Tested-by or
Reviewed-by.

- Arnaldo

Arnaldo Carvalho de Melo (13):
perf env: Move perf_env out of header.h and session.c into separate object perf env: Rename some leftovers from rename to perf_env
perf env: Adopt perf_header__set_cmdline
perf env: Introduce read_cpu_topology_map() method
perf sort: Set flag stating if the "socket" key is being used perf top: Cache the cpu topology info when "-s socket" is used perf hists browser: Fixup the "cpu" column width calculation perf machine: Add pointer to sample's environment
perf event: Use machine->env to find the cpu -> socket mapping perf report: Do not blindly use env->cpu[al.cpu].socket_id Revert "perf evsel: Add a backpointer to the evlist a evsel is in" perf evsel: Remove forward declaration of 'struct perf_evlist' Revert "perf evlist: Add backpointer for perf_env to evlist"

I have tested patch 1 to 10. They looks good to me except patch 4/13. Please

see my email in that thread.

However, during the testing I found a limitation related to cpu online/offline and 'perf top' that, if I offline most of cores before 'perf top', then online them during 'perf top' running, 'perf top' dooesn't report new CPUs. It still reports the CPUs which are online

However, It is relatively a rare case. I don't think we have to fix it in this patchset.

Em Fri, Sep 11, 2015 at 08:20:54PM +0800, Wangnan (F) escreveu:

Quoted 1 line

I have tested patch 1 to 10. They looks good to me except patch 4/13. Please

Ok, I'll take that as a Tested-by: you for 1-10 with 4/13 having the checks added, ok?

Quoted 1 line

see my email in that thread.

I add those checks.

Em Fri, Sep 11, 2015 at 10:03:39AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 5 lines

Em Fri, Sep 11, 2015 at 08:20:54PM +0800, Wangnan (F) escreveu:

Quoted 1 line

I have tested patch 1 to 10. They looks good to me except patch 4/13. Please

Ok, I'll take that as a Tested-by: you for 1-10 with 4/13 having the checks added, ok?

Quoted 3 lines

Quoted 1 line

see my email in that thread.

I add those checks.

Ok, below is the diff for adding the checks. The get_{core,socket}_id functions should be moved to tools/lib/api/cpu.[ch], using the same interface as cpu__get_max_freq(&value), using the int return value to propagate the precise error, etc. Will do it in a follow up patch.

- Arnaldo

tools/perf/util/env.c +11 -4

index 6af4f7c36820..2e4cad84197b 100644
@@ -60,7 +60,7 @@ out_enomem:
 
 int perf_env__read_cpu_topology_map(struct perf_env *env)
 {
-	int cpu, nr_cpus;
+	int cpu, nr_cpus, err;
 
 	if (env->cpu != NULL)
 		return 0;

@@ -77,10 +77,17 @@ int perf_env__read_cpu_topology_map(struct perf_env *env)
 		return -ENOMEM;
 
 	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);
+		err = env->cpu[cpu].core_id = cpu_map__get_core_id(cpu);
+		if (err < 0)
+			goto out_free;
+		err = env->cpu[cpu].socket_id = cpu_map__get_socket_id(cpu);
+		if (err < 0)
+			goto out_free;
 	}
 
-	env->nr_cpus_avail = nr_cpus;
 	return 0;
+
+out_free:
+	zfree(&env->cpu);
+	return err;
 }

Em Fri, Sep 11, 2015 at 10:29:37AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 9 lines

Em Fri, Sep 11, 2015 at 10:03:39AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 7 lines

Em Fri, Sep 11, 2015 at 08:20:54PM +0800, Wangnan (F) escreveu:

Quoted 1 line

I have tested patch 1 to 10. They looks good to me except patch 4/13. Please

Ok, I'll take that as a Tested-by: you for 1-10 with 4/13 having the checks added, ok?

Quoted 1 line

see my email in that thread.

Quoted 6 lines

Quoted 1 line

I add those checks.

Ok, below is the diff for adding the checks. The get_{core,socket}_id functions should be moved to tools/lib/api/cpu.[ch], using the same interface as cpu__get_max_freq(&value), using the int return value to propagate the precise error, etc. Will do it in a follow up patch.

Humm, but then, what happens if a CPU is offline? I'm checking it now...

Quoted 34 lines

tools/perf/util/env.c +11 -4

index 6af4f7c36820..2e4cad84197b 100644
@@ -60,7 +60,7 @@ out_enomem:
 
 int perf_env__read_cpu_topology_map(struct perf_env *env)
 {
-	int cpu, nr_cpus;
+	int cpu, nr_cpus, err;
 
 	if (env->cpu != NULL)
 		return 0;

@@ -77,10 +77,17 @@ int perf_env__read_cpu_topology_map(struct perf_env *env)
 		return -ENOMEM;
 
 	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);
+		err = env->cpu[cpu].core_id = cpu_map__get_core_id(cpu);
+		if (err < 0)
+			goto out_free;
+		err = env->cpu[cpu].socket_id = cpu_map__get_socket_id(cpu);
+		if (err < 0)
+			goto out_free;
 	}
 
-	env->nr_cpus_avail = nr_cpus;
 	return 0;
+
+out_free:
+	zfree(&env->cpu);
+	return err;
 }

Em Fri, Sep 11, 2015 at 10:30:52AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 7 lines

Em Fri, Sep 11, 2015 at 10:29:37AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 6 lines

Em Fri, Sep 11, 2015 at 10:03:39AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 4 lines

Em Fri, Sep 11, 2015 at 08:20:54PM +0800, Wangnan (F) escreveu:

Quoted 1 line

I have tested patch 1 to 10. They looks good to me except patch 4/13. Please

Ok, I'll take that as a Tested-by: you for 1-10 with 4/13 having the checks added, ok?

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

see my email in that thread.

I add those checks.

Ok, below is the diff for adding the checks. The get_{core,socket}_id functions should be moved to tools/lib/api/cpu.[ch], using the same interface as cpu__get_max_freq(&value), using the int return value to propagate the precise error, etc. Will do it in a follow up patch.

Humm, but then, what happens if a CPU is offline? I'm checking it now...

   # cat /sys/devices/system/cpu/cpu3/topology/core_id
   3
   # cat
   /sys/devices/system/cpu/cpu3/topology/physical_package_id
   0
   # echo 0 > /sys/devices/system/cpu/cpu3/online
   # cat
   /sys/devices/system/cpu/cpu3/topology/physical_package_id
   cat: /sys/devices/system/cpu/cpu3/topology/physical_package_id: No such file or directory
   # cat /sys/devices/system/cpu/cpu3/topology/core_id
   cat: /sys/devices/system/cpu/cpu3/topology/core_id: No such file or directory
   #

So we shouldn't check the result, right? We could further validate it by checking:

   # cat /sys/devices/system/cpu/cpu3/online
   0
   #

But assuming that not being able to access it means it is offline looks almost reasonable, if not strictly correct, so I'm removing the tests and will revisit this when I move those functions to tools/lib/api/cpu.[ch].

- Arnaldo

Quoted 34 lines

Quoted 34 lines

tools/perf/util/env.c +11 -4

index 6af4f7c36820..2e4cad84197b 100644
@@ -60,7 +60,7 @@ out_enomem:
 
  int perf_env__read_cpu_topology_map(struct perf_env *env)
  {
-	int cpu, nr_cpus;
+	int cpu, nr_cpus, err;
 
  	if (env->cpu != NULL)
  		return 0;

@@ -77,10 +77,17 @@ int perf_env__read_cpu_topology_map(struct perf_env *env)
  		return -ENOMEM;
 
  	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);
+		err = env->cpu[cpu].core_id = cpu_map__get_core_id(cpu);
+		if (err < 0)
+			goto out_free;
+		err = env->cpu[cpu].socket_id = cpu_map__get_socket_id(cpu);
+		if (err < 0)
+			goto out_free;
  	}
 
-	env->nr_cpus_avail = nr_cpus;
  	return 0;
+
+out_free:
+	zfree(&env->cpu);
+	return err;
  }

Em Fri, Sep 11, 2015 at 08:20:54PM +0800, Wangnan (F) escreveu:

Quoted 1 line

I have tested patch 1 to 10. They looks good to me except patch 4/13. Please

Ok, I'll take that as a Tested-by: you for 1-10 with 4/13 having the checks added, ok?

Quoted 1 line

see my email in that thread.

I add those checks.

Quoted 4 lines

However, during the testing I found a limitation related to cpu online/offline and 'perf top' that, if I offline most of cores before 'perf top', then online them during 'perf top' running, 'perf top' dooesn't report new CPUs. It still reports the CPUs which are online

<SNIP>

Quoted 2 lines

However, It is relatively a rare case. I don't think we have to fix it in this patchset.

Yup, unrelated to this patchset. But we need to seamlessly support that situation, even telling the user that a CPU went offline/online.

- Arnaldo