From: "Liang, Kan" <kan.liang@linux.intel.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: acme@kernel.org, jolsa@kernel.org, mingo@redhat.com,
linux-kernel@vger.kernel.org, peterz@infradead.org,
ak@linux.intel.com
Subject: Re: [PATCH 1/3] perf header: Add die information in CPU topology
Date: Tue, 28 May 2019 15:06:16 -0400 [thread overview]
Message-ID: <03a95846-5ccd-bbfa-ec95-b2cb8a83607d@linux.intel.com> (raw)
In-Reply-To: <20190528090001.GD27906@krava>
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
>
next prev parent reply other threads:[~2019-05-28 19:06 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-23 20:41 [PATCH 1/3] perf header: Add die information in CPU topology kan.liang
2019-05-23 20:41 ` [PATCH 2/3] perf stat: Support per-die aggregation kan.liang
2019-05-28 9:00 ` Jiri Olsa
2019-05-23 20:41 ` [PATCH 3/3] perf header: Rename "sibling cores" to "sibling sockets" kan.liang
2019-05-28 8:59 ` Jiri Olsa
2019-05-28 19:05 ` Liang, Kan
2019-05-28 8:59 ` [PATCH 1/3] perf header: Add die information in CPU topology Jiri Olsa
2019-05-28 8:59 ` Jiri Olsa
2019-05-28 9:00 ` Jiri Olsa
2019-05-28 19:06 ` Liang, Kan [this message]
2019-05-28 19:29 ` Jiri Olsa
2019-05-28 9:00 ` Jiri Olsa
2019-05-28 9:00 ` Jiri Olsa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=03a95846-5ccd-bbfa-ec95-b2cb8a83607d@linux.intel.com \
--to=kan.liang@linux.intel.com \
--cc=acme@kernel.org \
--cc=ak@linux.intel.com \
--cc=jolsa@kernel.org \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.