Messages in this thread | | | Date | Thu, 10 Sep 2015 10:58:38 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH V9 1/6] perf,tools: introduce generic FEAT for CPU attributes |
| |
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
| |