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

[Liang, Kan]

> On Fri, Aug 28, 2015 at 09:43:38AM -0400, Kan Liang wrote:
> > From: Kan Liang <kan.liang@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> >
> > 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 ?

Yes, but we need to modify the functions in cpumap.c a little bit.
I will write a new patch for it in next version.

> 
> > +
> >  	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?
> 
The old perf can definitely read new perf.data, because the new codes are
added at the end of the section. It can ignore the extra data.
The new perf can read part of the old perf.data. It's because it will error
out  when core_id and socket_id are not detected. So the following sections
will not be read. But we have message for them to let them upgrade
the perf tool. It should be enough.


> this needs to be explained in some comment, or if it's breakage we need
> separate feature or event
>
OK. I will describe it in the changelog.
 
Thanks,
Kan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/