Re: [PATCH V2 1/2] perf,tools: store cpu socket_id and core_id in perf.date

[Jiri Olsa <jolsa@redhat.com>]

On Fri, Aug 28, 2015 at 09:43:38AM -0400, Kan Liang wrote:
> From: Kan Liang <kan.liang@intel.com>
> 
> This patch stores cpu socket_id and core_id in perf.date, and read them
> to perf_env in header process.
> 
> Signed-off-by: Kan Liang <kan.liang@intel.com>
> ---
> 
> Changes since V1:
>  - Store core_id and socket_id in perf.date
> 
>  tools/perf/util/header.c  | 97 +++++++++++++++++++++++++++++++++++++++++++++--
>  tools/perf/util/header.h  |  6 +++
>  tools/perf/util/session.c |  1 +
>  3 files changed, 100 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 4181454..482749f 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -439,14 +439,42 @@ static int write_cmdline(int fd, struct perf_header *h __maybe_unused,
>  	"/sys/devices/system/cpu/cpu%d/topology/core_siblings_list"
>  #define THRD_SIB_FMT \
>  	"/sys/devices/system/cpu/cpu%d/topology/thread_siblings_list"
> +#define CORE_ID_FMT \
> +	"/sys/devices/system/cpu/cpu%d/topology/core_id"
> +#define PHY_PKG_ID_FMT \
> +	"/sys/devices/system/cpu/cpu%d/topology/physical_package_id"

hardcoded /sys

>  
>  struct cpu_topo {
> +	u32 cpu_nr;
>  	u32 core_sib;
>  	u32 thread_sib;
>  	char **core_siblings;
>  	char **thread_siblings;
> +	int *core_id;
> +	int *phy_pkg_id;
>  };
>  
> +static int read_id(const char *path, int cpu)
> +{
> +	FILE *fp;
> +	char filename[MAXPATHLEN];
> +	char *buf = NULL;
> +	size_t len = 0;
> +	int ret = -1;
> +
> +	sprintf(filename, path, cpu);
> +	fp = fopen(filename, "r");
> +	if (fp == NULL)
> +		return ret;
> +
> +	if (getline(&buf, &len, fp) > 0)
> +		ret = atoi(buf);
> +
> +	fclose(fp);
> +	free(buf);
> +	return ret;
> +}
> +
>  static int build_cpu_topo(struct cpu_topo *tp, int cpu)
>  {
>  	FILE *fp;
> @@ -507,6 +535,9 @@ try_threads:
>  	}
>  	ret = 0;
>  done:
> +	tp->core_id[cpu] = read_id(CORE_ID_FMT, cpu);
> +	tp->phy_pkg_id[cpu] = read_id(PHY_PKG_ID_FMT, cpu);

hu,m dont we read this already somehow in cpumap.c ?

> +
>  	if(fp)
>  		fclose(fp);
>  	free(buf);
> @@ -534,7 +565,7 @@ static struct cpu_topo *build_cpu_topology(void)
>  	struct cpu_topo *tp;
>  	void *addr;
>  	u32 nr, i;
> -	size_t sz;
> +	size_t sz, sz_id;
>  	long ncpus;
>  	int ret = -1;
>  

SNIP

> @@ -1631,10 +1686,44 @@ static int process_cpu_topology(struct perf_file_section *section __maybe_unused
>  		free(str);
>  	}
>  	ph->env.sibling_threads = strbuf_detach(&sb, NULL);
> +
> +	for (i = 0; i < (u32)cpu_nr; i++) {
> +		ret = readn(fd, &nr, sizeof(nr));
> +		if (ret != sizeof(nr))
> +			goto free_cpu;
> +
> +		if (ph->needs_swap)
> +			nr = bswap_32(nr);
> +
> +		if (nr > (u32)cpu_nr) {
> +			pr_debug("core_id number is too big."
> +				 "You may need to upgrade the perf tool.\n");
> +			goto free_cpu;
> +		}
> +		ph->env.cpu[i].core_id = nr;
> +
> +		ret = readn(fd, &nr, sizeof(nr));
> +		if (ret != sizeof(nr))
> +			goto free_cpu;
> +
> +		if (ph->needs_swap)
> +			nr = bswap_32(nr);
> +
> +		if (nr > (u32)cpu_nr) {
> +			pr_debug("socket_id number is too big."
> +				 "You may need to upgrade the perf tool.\n");
> +			goto free_cpu;

we should keep some degree of backward compatibility,
you changed the CPU_TOPOLOGY feature.. are we still able
read older perf.data? can old perf read new perf.data?

this needs to be explained in some comment, or if it's
breakage we need separate feature or event


jirka