| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150910135838.GX3475@kernel.org>
Date: Thu, 10 Sep 2015 10:58:38 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: kan.liang@...el.com
Cc: jolsa@...nel.org, a.p.zijlstra@...llo.nl, luto@...nel.org,
mingo@...hat.com, eranian@...gle.com, ak@...ux.intel.com,
mark.rutland@....com, adrian.hunter@...el.com, namhyung@...nel.org,
linux-kernel@...r.kernel.org
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@...el.com escreveu:
> From: Kan Liang <kan.liang@...el.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@...el.com>
> Acked-by: Jiri Olsa <jolsa@...nel.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists