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

[Jiri Olsa]

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

> +
> +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