| 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
| ||
|
Date: Tue, 28 May 2019 15:06:16 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Jiri Olsa <jolsa@...hat.com>
Cc: acme@...nel.org, jolsa@...nel.org, mingo@...hat.com,
linux-kernel@...r.kernel.org, peterz@...radead.org,
ak@...ux.intel.com
Subject: Re: [PATCH 1/3] perf header: Add die information in CPU topology
On 5/28/2019 5:00 AM, Jiri Olsa wrote:
> On Thu, May 23, 2019 at 01:41:19PM -0700, kan.liang@...ux.intel.com wrote:
>
> SNIP
>
>> diff --git a/tools/perf/util/cputopo.c b/tools/perf/util/cputopo.c
>> index ece0710..f6e7db7 100644
>> --- a/tools/perf/util/cputopo.c
>> +++ b/tools/perf/util/cputopo.c
>> @@ -1,5 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0
>> #include <sys/param.h>
>> +#include <sys/utsname.h>
>> #include <inttypes.h>
>> #include <api/fs/fs.h>
>>
>> @@ -8,9 +9,10 @@
>> #include "util.h"
>> #include "env.h"
>>
>> -
>> #define CORE_SIB_FMT \
>> "%s/devices/system/cpu/cpu%d/topology/core_siblings_list"
>> +#define DIE_SIB_FMT \
>> + "%s/devices/system/cpu/cpu%d/topology/die_cpus_list"
>> #define THRD_SIB_FMT \
>> "%s/devices/system/cpu/cpu%d/topology/thread_siblings_list"
>> #define NODE_ONLINE_FMT \
>> @@ -20,7 +22,26 @@
>> #define NODE_CPULIST_FMT \
>> "%s/devices/system/node/node%d/cpulist"
>>
>> -static int build_cpu_topology(struct cpu_topology *tp, int cpu)
>> +bool check_x86_die_exists(void)
>> +{
>> + char filename[MAXPATHLEN];
>> + struct utsname uts;
>> +
>> + if (uname(&uts) < 0)
>> + return false;
>> +
>> + if (strncmp(uts.machine, "x86_64", 6))
>> + return false;
>> +
>> + scnprintf(filename, MAXPATHLEN, DIE_SIB_FMT,
>> + sysfs__mountpoint(), 0);
>> + if (access(filename, F_OK) == -1)
>> + return false;
>> +
>> + return true;
>> +}
>
> we could rename this to:
>
> static bool has_die(void)
> {
> static bool has_die;
>
> if (initialized)
> return has_die;
>
> has_die = ...
> initialized = true;
> }
>
> and got rid of all those 'has_die' arguments below
Yes, we can rename the function to has_die(). It looks like all the
'has_die' arguments can be replaced either.
But why we want a "initialized" here? to cache the value? It looks like
we only need to call has_die() once.
Thanks,
Kan
>
>> +
>> +static int build_cpu_topology(struct cpu_topology *tp, int cpu, bool has_die)
>> {
>> FILE *fp;
>> char filename[MAXPATHLEN];
>> @@ -34,12 +55,12 @@ static int build_cpu_topology(struct cpu_topology *tp, int cpu)
>> sysfs__mountpoint(), cpu);
>> fp = fopen(filename, "r");
>> if (!fp)
>> - goto try_threads;
>> + goto try_dies;
>>
>> sret = getline(&buf, &len, fp);
>> fclose(fp);
>> if (sret <= 0)
>> - goto try_threads;
>> + goto try_dies;
>>
>> p = strchr(buf, '\n');
>> if (p)
>> @@ -57,6 +78,35 @@ static int build_cpu_topology(struct cpu_topology *tp, int cpu)
>> }
>> ret = 0;
>>
>> +try_dies:
>> + if (has_die) {
>> + scnprintf(filename, MAXPATHLEN, DIE_SIB_FMT,
>> + sysfs__mountpoint(), cpu);
>> + fp = fopen(filename, "r");
>> + if (!fp)
>> + goto try_threads;
>> +
>> + sret = getline(&buf, &len, fp);
>> + fclose(fp);
>> + if (sret <= 0)
>> + goto try_threads;
>> +
>> + p = strchr(buf, '\n');
>> + if (p)
>> + *p = '\0';
>> +
>> + for (i = 0; i < tp->die_sib; i++) {
>> + if (!strcmp(buf, tp->die_siblings[i]))
>> + break;
>> + }
>> + if (i == tp->die_sib) {
>> + tp->die_siblings[i] = buf;
>> + tp->die_sib++;
>> + buf = NULL;
>> + len = 0;
>> + }
>> + ret = 0;
>> + }
>> try_threads:
>> scnprintf(filename, MAXPATHLEN, THRD_SIB_FMT,
>> sysfs__mountpoint(), cpu);
>> @@ -88,7 +138,7 @@ static int build_cpu_topology(struct cpu_topology *tp, int cpu)
>> return ret;
>> }
>>
>> -void cpu_topology__delete(struct cpu_topology *tp)
>> +void cpu_topology__delete(struct cpu_topology *tp, bool has_die)
>> {
>> u32 i;
>>
>> @@ -98,17 +148,22 @@ void cpu_topology__delete(struct cpu_topology *tp)
>> for (i = 0 ; i < tp->core_sib; i++)
>> zfree(&tp->core_siblings[i]);
>>
>> + if (has_die) {
>> + for (i = 0 ; i < tp->die_sib; i++)
>> + zfree(&tp->die_siblings[i]);
>> + }
>> +
>> for (i = 0 ; i < tp->thread_sib; i++)
>> zfree(&tp->thread_siblings[i]);
>>
>> free(tp);
>> }
>>
>> -struct cpu_topology *cpu_topology__new(void)
>> +struct cpu_topology *cpu_topology__new(bool has_die)
>> {
>> struct cpu_topology *tp = NULL;
>> void *addr;
>> - u32 nr, i;
>> + u32 nr, i, nr_addr;
>> size_t sz;
>> long ncpus;
>> int ret = -1;
>> @@ -126,7 +181,11 @@ struct cpu_topology *cpu_topology__new(void)
>> nr = (u32)(ncpus & UINT_MAX);
>>
>> sz = nr * sizeof(char *);
>> - addr = calloc(1, sizeof(*tp) + 2 * sz);
>> + if (has_die)
>> + nr_addr = 3;
>> + else
>> + nr_addr = 2;
>> + addr = calloc(1, sizeof(*tp) + nr_addr * sz);
>> if (!addr)
>> goto out_free;
>>
>> @@ -134,13 +193,17 @@ struct cpu_topology *cpu_topology__new(void)
>> addr += sizeof(*tp);
>> tp->core_siblings = addr;
>> addr += sz;
>> + if (has_die) {
>> + tp->die_siblings = addr;
>> + addr += sz;
>> + }
>> tp->thread_siblings = addr;
>>
>> for (i = 0; i < nr; i++) {
>> if (!cpu_map__has(map, i))
>> continue;
>>
>> - ret = build_cpu_topology(tp, i);
>> + ret = build_cpu_topology(tp, i, has_die);
>> if (ret < 0)
>> break;
>> }
>> @@ -148,7 +211,7 @@ struct cpu_topology *cpu_topology__new(void)
>> out_free:
>> cpu_map__put(map);
>> if (ret) {
>> - cpu_topology__delete(tp);
>> + cpu_topology__delete(tp, has_die);
>> tp = NULL;
>> }
>> return tp;
>> diff --git a/tools/perf/util/cputopo.h b/tools/perf/util/cputopo.h
>> index 47a97e7..cb4e6fe 100644
>> --- a/tools/perf/util/cputopo.h
>> +++ b/tools/perf/util/cputopo.h
>> @@ -7,8 +7,10 @@
>>
>> struct cpu_topology {
>> u32 core_sib;
>> + u32 die_sib;
>> u32 thread_sib;
>> char **core_siblings;
>> + char **die_siblings;
>> char **thread_siblings;
>> };
>>
>> @@ -24,8 +26,9 @@ struct numa_topology {
>> struct numa_topology_node nodes[0];
>> };
>>
>> -struct cpu_topology *cpu_topology__new(void);
>> -void cpu_topology__delete(struct cpu_topology *tp);
>> +bool check_x86_die_exists(void);
>> +struct cpu_topology *cpu_topology__new(bool has_die);
>> +void cpu_topology__delete(struct cpu_topology *tp, bool has_die);
>>
>> struct numa_topology *numa_topology__new(void);
>> void numa_topology__delete(struct numa_topology *tp);
>> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
>> index 6a3eaf7..1cc7a18 100644
>> --- a/tools/perf/util/env.c
>> +++ b/tools/perf/util/env.c
>> @@ -246,6 +246,7 @@ int perf_env__read_cpu_topology_map(struct perf_env *env)
>> 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);
>> + env->cpu[cpu].die_id = cpu_map__get_die_id(cpu);
>> }
>>
>> env->nr_cpus_avail = nr_cpus;
>> diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
>> index 271a90b..d5d9865 100644
>> --- a/tools/perf/util/env.h
>> +++ b/tools/perf/util/env.h
>> @@ -9,6 +9,7 @@
>>
>> struct cpu_topology_map {
>> int socket_id;
>> + int die_id;
>> int core_id;
>> };
>>
>> @@ -49,6 +50,7 @@ struct perf_env {
>>
>> int nr_cmdline;
>> int nr_sibling_cores;
>> + int nr_sibling_dies;
>> int nr_sibling_threads;
>> int nr_numa_nodes;
>> int nr_memory_nodes;
>> @@ -57,6 +59,7 @@ struct perf_env {
>> char *cmdline;
>> const char **cmdline_argv;
>> char *sibling_cores;
>> + char *sibling_dies;
>> char *sibling_threads;
>> char *pmu_mappings;
>> struct cpu_topology_map *cpu;
>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>> index 847ae51..faa1e38 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -561,8 +561,11 @@ static int write_cpu_topology(struct feat_fd *ff,
>> struct cpu_topology *tp;
>> u32 i;
>> int ret, j;
>> + bool has_die;
>>
>> - tp = cpu_topology__new();
>> + has_die = check_x86_die_exists();
>> +
>> + tp = cpu_topology__new(has_die);
>> if (!tp)
>> return -1;
>>
>> @@ -599,8 +602,29 @@ static int write_cpu_topology(struct feat_fd *ff,
>> if (ret < 0)
>> return ret;
>> }
>> +
>> + if (!has_die)
>> + goto done;
>
> also the has_die() could stay static in util/cputopo.c,
> we can get the 'has_die state' from tp->die_sib != 0
>
>> +
>> + ret = do_write(ff, &tp->die_sib, sizeof(tp->die_sib));
>> + if (ret < 0)
>> + goto done;
>> +
>> + for (i = 0; i < tp->die_sib; i++) {
>> + ret = do_write_string(ff, tp->die_siblings[i]);
>> + if (ret < 0)
>> + goto done;
>> + }
>> +
>> + for (j = 0; j < perf_env.nr_cpus_avail; j++) {
>> + ret = do_write(ff, &perf_env.cpu[j].die_id,
>> + sizeof(perf_env.cpu[j].die_id));
>> + if (ret < 0)
>> + return ret;
>> + }
>> +
>> done:
>> - cpu_topology__delete(tp);
>> + cpu_topology__delete(tp, has_die);
>> return ret;
>> }
>>
>> @@ -1428,6 +1452,8 @@ static void print_cmdline(struct feat_fd *ff, FILE *fp)
>> fputc('\n', fp);
>> }
>>
>> +static bool has_die;
>
> same here.. you can use env->nr_sibling_dies instead of static bool
>
>> +
>> static void print_cpu_topology(struct feat_fd *ff, FILE *fp)
>> {
>> struct perf_header *ph = ff->ph;
>> @@ -1443,6 +1469,16 @@ static void print_cpu_topology(struct feat_fd *ff, FILE *fp)
>> str += strlen(str) + 1;
>> }
>>
>> + if (has_die) {
>> + nr = ph->env.nr_sibling_dies;
>> + str = ph->env.sibling_dies;
>> +
>> + for (i = 0; i < nr; i++) {
>> + fprintf(fp, "# sibling dies : %s\n", str);
>> + str += strlen(str) + 1;
>> + }
>> + }
>> +
>> nr = ph->env.nr_sibling_threads;
>> str = ph->env.sibling_threads;
>>
>> @@ -1451,12 +1487,28 @@ static void print_cpu_topology(struct feat_fd *ff, FILE *fp)
>> str += strlen(str) + 1;
>> }
>>
>> - if (ph->env.cpu != NULL) {
>> - for (i = 0; i < cpu_nr; i++)
>> - fprintf(fp, "# CPU %d: Core ID %d, Socket ID %d\n", i,
>> - ph->env.cpu[i].core_id, ph->env.cpu[i].socket_id);
>> - } else
>> - fprintf(fp, "# Core ID and Socket ID information is not available\n");
>> + if (has_die) {
>> + if (ph->env.cpu != NULL) {
>> + for (i = 0; i < cpu_nr; i++)
>> + fprintf(fp, "# CPU %d: Core ID %d, "
>> + "Die ID %d, Socket ID %d\n",
>> + i, ph->env.cpu[i].core_id,
>> + ph->env.cpu[i].die_id,
>> + ph->env.cpu[i].socket_id);
>> + } else
>> + fprintf(fp, "# Core ID, Die ID and Socket ID "
>> + "information is not available\n");
>> + } else {
>> + if (ph->env.cpu != NULL) {
>> + for (i = 0; i < cpu_nr; i++)
>> + fprintf(fp, "# CPU %d: Core ID %d, "
>> + "Socket ID %d\n",
>> + i, ph->env.cpu[i].core_id,
>> + ph->env.cpu[i].socket_id);
>> + } else
>> + fprintf(fp, "# Core ID and Socket ID "
>> + "information is not available\n");
>> + }
>> }
>
> SNIP
>
> thanks,
> jirka
>
Powered by blists - more mailing lists