From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752735AbbIKOkh (ORCPT ); Fri, 11 Sep 2015 10:40:37 -0400 Received: from mail.kernel.org ([198.145.29.136]:39103 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752525AbbIKOkg (ORCPT ); Fri, 11 Sep 2015 10:40:36 -0400 Date: Fri, 11 Sep 2015 11:40:28 -0300 From: Arnaldo Carvalho de Melo To: "Wangnan (F)" Cc: "Liang, Kan" , Ingo Molnar , "linux-kernel@vger.kernel.org" , "Hunter, Adrian" , Borislav Petkov , David Ahern , Frederic Weisbecker , Jiri Olsa , Namhyung Kim , Stephane Eranian , pi3orama@163.com Subject: Re: [PATCH 04/13] perf env: Introduce read_cpu_topology_map() method Message-ID: <20150911144028.GN23511@kernel.org> References: <1441828225-667-1-git-send-email-acme@kernel.org> <1441828225-667-5-git-send-email-acme@kernel.org> <37D7C6CF3E00A74B8858931C1DB2F07701922457@SHSMSX103.ccr.corp.intel.com> <20150910131250.GW3475@kernel.org> <55F2AAD3.7040506@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55F2AAD3.7040506@huawei.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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@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 # node0 meminfo : total = 8085412 kB, free = 5317596 kB # node0 cpu list : 0-1,3 $ 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 # 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; > > } >