Messages in this thread | | | Subject | Re: [PATCH 1/3] perf header: Add die information in CPU topology | From | "Liang, Kan" <> | Date | Tue, 28 May 2019 15:06:16 -0400 |
| |
On 5/28/2019 5:00 AM, Jiri Olsa wrote: > On Thu, May 23, 2019 at 01:41:19PM -0700, kan.liang@linux.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 >
| |