Re: [PATCH V9 1/6] perf,tools: introduce generic FEAT for CPU attributes

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Tue, Sep 08, 2015 at 03:32:44PM -0400, kan.liang@intel.com escreveu:
> From: Kan Liang <kan.liang@intel.com>
> 
> This patch introduces generic FEAT for CPU attributes. For the patch
> set, we only need cpu max frequency. But it can be easily extented to
> support more other CPU attributes.
> The cpu max frequency is from the first online cpu.

Ok, but don't we have to do error handling? i.e. you are returning 0 for
any error in trying to read the cpu max freq, shouldn't we bail out
somewhere?

And please move this get_cpu_max_freq() thing out of the cpumap.[ch]
files, it is not even a need completely specific to perf tooling, there
must be somewhere in tools/lib/api/ (kernel APIs) where this fits, no?

More comments below.

- Arnaldo
 
> Signed-off-by: Kan Liang <kan.liang@intel.com>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/cpumap.c | 32 ++++++++++++++++++++++++++
>  tools/perf/util/cpumap.h |  1 +
>  tools/perf/util/header.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/header.h | 12 ++++++++++
>  4 files changed, 104 insertions(+)
> 
> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index a05d76a..671ee83 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -514,3 +514,35 @@ int cpu__setup_cpunode_map(void)
>  	closedir(dir1);
>  	return 0;
>  }
> +
> +u64 get_cpu_max_freq(void)
> +{
> +	const char *mnt;
> +	char path[PATH_MAX], tmp;
> +	FILE *fp;
> +	u64 freq;
> +	int cpu = 0;
> +	int ret;
> +
> +	mnt = sysfs__mountpoint();
> +	if (!mnt)
> +		return 0;

See? Can't find the sysfs mount point? No problem, return 0 max freq.

> +
> +	snprintf(path, PATH_MAX, "%s/devices/system/cpu/online", mnt);
> +	fp = fopen(path, "r");
> +	if (fp) {
> +		ret = fscanf(fp, "%u%c", &cpu, &tmp);
> +		fclose(fp);
> +		if (ret < 1)
> +			return 0;

Can't parse it? 0

> +	}
> +
> +	snprintf(path, PATH_MAX, "%s/devices/system/cpu/cpu%d/cpufreq/cpuinfo_max_freq", mnt, cpu);
> +	fp = fopen(path, "r");
> +	if (!fp)
> +		return 0;

Ditto. Return soem error here please.

> +	ret = fscanf(fp, "%lu", &freq);
> +	fclose(fp);
> +
> +	return (ret == 1) ? freq : 0;
> +}
> diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
> index 8982d53..06cd2c4 100644
> --- a/tools/perf/util/cpumap.h
> +++ b/tools/perf/util/cpumap.h
> @@ -60,6 +60,7 @@ int max_node_num;
>  int *cpunode_map;
>  
>  int cpu__setup_cpunode_map(void);
> +u64 get_cpu_max_freq(void);

>  
>  static inline int cpu__max_node(void)
>  {
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 8fd7b7d..3535dcb 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -885,6 +885,23 @@ write_it:
>  	return do_write_string(fd, buffer);
>  }
>  
> +static int write_cpu_attributes(int fd, struct perf_header *h __maybe_unused,
> +				struct perf_evlist *evlist __maybe_unused)
> +{
> +	u32 tag_id;
> +	u64 max_freq;
> +	int ret;
> +
> +	tag_id = PERF_HEADER_CPU_MAX_FREQ;
> +	ret = do_write(fd, &tag_id, sizeof(tag_id));
> +	if (ret < 0)
> +		return ret;
> +
> +	max_freq = get_cpu_max_freq();

Here, do the error handling, return whatever errno code this function
returned, probably the feature code will use it to warn the user
somehow.

> +
> +	return do_write(fd, &max_freq, sizeof(max_freq));
> +}
> +
>  static int write_branch_stack(int fd __maybe_unused,
>  			      struct perf_header *h __maybe_unused,
>  		       struct perf_evlist *evlist __maybe_unused)
> @@ -1185,6 +1202,11 @@ static void print_cpuid(struct perf_header *ph, int fd __maybe_unused, FILE *fp)
>  	fprintf(fp, "# cpuid : %s\n", ph->env.cpuid);
>  }
>  
> +static void print_cpu_attributes(struct perf_header *ph, int fd __maybe_unused, FILE *fp)
> +{
> +	fprintf(fp, "# CPU attributes: max frequency = %lu KHz\n", ph->env.cpuattr.max_freq);
> +}
> +
>  static void print_branch_stack(struct perf_header *ph __maybe_unused,
>  			       int fd __maybe_unused, FILE *fp)
>  {
> @@ -1498,6 +1520,42 @@ static int process_cpuid(struct perf_file_section *section __maybe_unused,
>  	return ph->env.cpuid ? 0 : -ENOMEM;
>  }
>  
> +static int process_cpu_attributes(struct perf_file_section *section __maybe_unused,
> +				  struct perf_header *ph, int fd,
> +				  void *data __maybe_unused)
> +{
> +	ssize_t ret;
> +	u32 i, tag_id;
> +	u64 nr;
> +
> +	for (i = 0; i < PERF_HEADER_CPU_ATTR_MAX; i++) {
> +
> +		ret = readn(fd, &tag_id, sizeof(tag_id));
> +		if (ret != sizeof(tag_id))
> +			return -1;

Return some sensible errno... Its not because
perf_header__process_sections() doesn't check them that we shouldn't
return ;-\ I'll fix perf_header__process_sections() to stop the process
and warn the user if some error happens...

> +
> +		if (ph->needs_swap)
> +			nr = bswap_32(tag_id);
> +
> +		if (tag_id >= PERF_HEADER_CPU_ATTR_MAX) {
> +			pr_debug("The number of cpu attributes is not expected. "
> +				 "You may need to upgrade the perf tool.\n");
> +			return -1;


ditto
> +		}
> +
> +		ret = readn(fd, &nr, sizeof(nr));
> +		if (ret != sizeof(nr))
> +			return -1;


ditto

> +
> +		if (ph->needs_swap)
> +			nr = bswap_64(nr);
> +
> +		ph->env.cpu_attr[tag_id] = nr;
> +	}
> +
> +	return 0;
> +}
> +
>  static int process_total_mem(struct perf_file_section *section __maybe_unused,
>  			     struct perf_header *ph, int fd,
>  			     void *data __maybe_unused)
> @@ -1983,6 +2041,7 @@ static const struct feature_ops feat_ops[HEADER_LAST_FEATURE] = {
>  	FEAT_OPP(HEADER_PMU_MAPPINGS,	pmu_mappings),
>  	FEAT_OPP(HEADER_GROUP_DESC,	group_desc),
>  	FEAT_OPP(HEADER_AUXTRACE,	auxtrace),
> +	FEAT_OPP(HEADER_CPU_ATTR,	cpu_attributes),
>  };
>  
>  struct header_print_data {
> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
> index 975d803..dd9f6b0 100644
> --- a/tools/perf/util/header.h
> +++ b/tools/perf/util/header.h
> @@ -31,6 +31,7 @@ enum {
>  	HEADER_PMU_MAPPINGS,
>  	HEADER_GROUP_DESC,
>  	HEADER_AUXTRACE,
> +	HEADER_CPU_ATTR,
>  	HEADER_LAST_FEATURE,
>  	HEADER_FEAT_BITS	= 256,
>  };
> @@ -71,6 +72,11 @@ struct cpu_topology_map {
>  	int	core_id;
>  };
>  
> +enum perf_header_cpu_attr {
> +	PERF_HEADER_CPU_MAX_FREQ	= 0,
> +	PERF_HEADER_CPU_ATTR_MAX,
> +};
> +
>  struct perf_env {
>  	char			*hostname;
>  	char			*os_release;
> @@ -95,6 +101,12 @@ struct perf_env {
>  	char			*numa_nodes;
>  	char			*pmu_mappings;
>  	struct cpu_topology_map	*cpu;
> +	union  {
> +		u64		cpu_attr[PERF_HEADER_CPU_ATTR_MAX];
> +		struct {
> +			u64	max_freq;
> +		} cpuattr;
> +	};

Ok, these need moving to env.h, but lets first get that patchkit
merged... Looking at your other patches

>  };
>  
>  struct perf_header {
> -- 
> 1.8.3.1