Date:	Fri, 11 Sep 2015 11:40:28 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	"Wangnan (F)" <wangnan0@...wei.com>
Cc:	"Liang, Kan" <kan.liang@...el.com>, Ingo Molnar <mingo@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Hunter, Adrian" <adrian.hunter@...el.com>,
	Borislav Petkov <bp@...e.de>, David Ahern <dsahern@...il.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Jiri Olsa <jolsa@...hat.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Stephane Eranian <eranian@...gle.com>, pi3orama@....com
Subject: Re: [PATCH 04/13] perf env: Introduce read_cpu_topology_map() method

Em Fri, Sep 11, 2015 at 06:20:03PM +0800, Wangnan (F) escreveu:
> 
> 
> On 2015/9/10 21:12, Arnaldo Carvalho de Melo wrote:
> 
> [SNIP]
> 
> >+
> >+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);
> >+	}
> 
> Shouldn't we check the failure of these two functions?

Humm, the original code, in build_cpu_topology() was not checking
that, i.e. if you tried it with or without my patches the result will be
the same, no?
 
> 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
>  # ========

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@...icio 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!

- Arnaldo
 
 
> >+
> >+	env->nr_cpus_avail = nr_cpus;
> >+	return 0;
> >+}
> >diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
> >index 70124d9a1624..c4e36323d91e 100644
> >--- a/tools/perf/util/env.h
> >+++ b/tools/perf/util/env.h
> >@@ -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 */
> >diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> >index 151b8310ac70..d6437465f70f 100644
> >--- a/tools/perf/util/header.c
> >+++ b/tools/perf/util/header.c
> >@@ -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;
> >  	}
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists