Re: [PATCH 1/3] perf header: Add die information in CPU topology

[Liang, Kan]

On 5/28/2019 5:00 AM, Jiri Olsa wrote:
> On Thu, May 23, 2019 at 01:41:19PM -0700, kan.liang@xxxxxxxxxxxxxxx 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