| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150911144028.GN23511@kernel.org>
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