* [PATCH v4] perf record: encode -k clockid frequency into Perf trace @ 2018-10-09 6:59 Alexey Budankov 2018-10-09 13:41 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 3+ messages in thread From: Alexey Budankov @ 2018-10-09 6:59 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo Cc: Alexander Shishkin, Jiri Olsa, Namhyung Kim, Andi Kleen, linux-kernel Store -k clockid frequency into Perf trace to enable timestamps derived metrics conversion into wall clock time on reporting stage. Below is the example of perf report output: tools/perf/perf record -k raw -- ../../matrix/linux/matrix.gcc ... [ perf record: Captured and wrote 31.222 MB perf.data (818054 samples) ] tools/perf/perf report --header # ======== ... # event : name = cycles:ppp, , size = 112, { sample_period, sample_freq } = 4000, sample_type = IP|TID|TIME|PERIOD, disabled = 1, inherit = 1, mmap = 1, comm = 1, freq = 1, enable_on_exec = 1, task = 1, precise_ip = 3, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1, use_clockid = 1, clockid = 4 ... # clockid frequency: 1000 MHz ... # ======== Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com> --- Changes in v4: - addressed compilation issues on 32bit platforms Changes in v3: - moved header's clockid_res_ns initialization out of record__init_features() - added explicit warning for case of failed clock_getres() call Changes in v2: - renamed clockid_freq to clockid_res_ns and get_clockid_freq() to get_clockid_res() - avoided redundant define of NSEC_IN_SEC, reused linux/time64.h:NSEC_PER_SEC - moved MHz conversion into print_clockid() and shortened write_clockid() --- tools/perf/builtin-record.c | 24 ++++++++++++++++++++++-- tools/perf/perf.h | 1 + tools/perf/util/env.h | 1 + tools/perf/util/header.c | 28 ++++++++++++++++++++++++++++ tools/perf/util/header.h | 1 + 5 files changed, 53 insertions(+), 2 deletions(-) diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index 0980dfe3396b..d803d3264465 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -592,6 +592,9 @@ static void record__init_features(struct record *rec) if (!rec->opts.full_auxtrace) perf_header__clear_feat(&session->header, HEADER_AUXTRACE); + if (!(rec->opts.use_clockid && rec->opts.clockid_res_ns)) + perf_header__clear_feat(&session->header, HEADER_CLOCKID); + perf_header__clear_feat(&session->header, HEADER_STAT); } @@ -897,6 +900,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv) record__init_features(rec); + if (rec->opts.use_clockid && rec->opts.clockid_res_ns) + session->header.env.clockid_res_ns = rec->opts.clockid_res_ns; + if (forks) { err = perf_evlist__prepare_workload(rec->evlist, &opts->target, argv, data->is_pipe, @@ -1337,6 +1343,19 @@ static const struct clockid_map clockids[] = { CLOCKID_END, }; +static int get_clockid_res(clockid_t clk_id, size_t *res_ns) +{ + struct timespec res; + + *res_ns = 0; + if (!clock_getres(clk_id, &res)) + *res_ns = res.tv_nsec + res.tv_sec * NSEC_PER_SEC; + else + pr_warning("WARNING: Failed to determine specified clock resolution.\n"); + + return 0; +} + static int parse_clockid(const struct option *opt, const char *str, int unset) { struct record_opts *opts = (struct record_opts *)opt->value; @@ -1360,7 +1379,7 @@ static int parse_clockid(const struct option *opt, const char *str, int unset) /* if its a number, we're done */ if (sscanf(str, "%d", &opts->clockid) == 1) - return 0; + return get_clockid_res(opts->clockid, &opts->clockid_res_ns); /* allow a "CLOCK_" prefix to the name */ if (!strncasecmp(str, "CLOCK_", 6)) @@ -1369,7 +1388,8 @@ static int parse_clockid(const struct option *opt, const char *str, int unset) for (cm = clockids; cm->name; cm++) { if (!strcasecmp(str, cm->name)) { opts->clockid = cm->clockid; - return 0; + return get_clockid_res(opts->clockid, + &opts->clockid_res_ns); } } diff --git a/tools/perf/perf.h b/tools/perf/perf.h index 21bf7f5a3cf5..981db3c2ed60 100644 --- a/tools/perf/perf.h +++ b/tools/perf/perf.h @@ -81,6 +81,7 @@ struct record_opts { unsigned initial_delay; bool use_clockid; clockid_t clockid; + size_t clockid_res_ns; unsigned int proc_map_timeout; }; diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h index 1f3ccc368530..b167a54d635f 100644 --- a/tools/perf/util/env.h +++ b/tools/perf/util/env.h @@ -63,6 +63,7 @@ struct perf_env { struct numa_node *numa_nodes; struct memory_node *memory_nodes; unsigned long long memory_bsize; + size_t clockid_res_ns; }; extern struct perf_env perf_env; diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c index 1ec1d9bc2d63..702e0cce63da 100644 --- a/tools/perf/util/header.c +++ b/tools/perf/util/header.c @@ -1034,6 +1034,14 @@ static int write_auxtrace(struct feat_fd *ff, return err; } +static int write_clockid(struct feat_fd *ff, + struct perf_evlist *evlist __maybe_unused) +{ + u64 clockid_res_ns = ff->ph->env.clockid_res_ns; + + return do_write(ff, &clockid_res_ns, sizeof(clockid_res_ns)); +} + static int cpu_cache_level__sort(const void *a, const void *b) { struct cpu_cache_level *cache_a = (struct cpu_cache_level *)a; @@ -1508,6 +1516,12 @@ static void print_cpu_topology(struct feat_fd *ff, FILE *fp) fprintf(fp, "# Core ID and Socket ID information is not available\n"); } +static void print_clockid(struct feat_fd *ff, FILE *fp) +{ + fprintf(fp, "# clockid frequency: %zd MHz\n", + ff->ph->env.clockid_res_ns * 1000); +} + static void free_event_desc(struct perf_evsel *events) { struct perf_evsel *evsel; @@ -2531,6 +2545,19 @@ static int process_mem_topology(struct feat_fd *ff, return ret; } +static int process_clockid(struct feat_fd *ff, + void *data __maybe_unused) +{ + u64 clockid_res_ns; + + if (do_read_u64(ff, &clockid_res_ns)) + return -1; + + ff->ph->env.clockid_res_ns = *(size_t *)&clockid_res_ns; + + return 0; +} + struct feature_ops { int (*write)(struct feat_fd *ff, struct perf_evlist *evlist); void (*print)(struct feat_fd *ff, FILE *fp); @@ -2590,6 +2617,7 @@ static const struct feature_ops feat_ops[HEADER_LAST_FEATURE] = { FEAT_OPN(CACHE, cache, true), FEAT_OPR(SAMPLE_TIME, sample_time, false), FEAT_OPR(MEM_TOPOLOGY, mem_topology, true), + FEAT_OPR(CLOCKID, clockid, false) }; struct header_print_data { diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h index e17903caa71d..0d553ddca0a3 100644 --- a/tools/perf/util/header.h +++ b/tools/perf/util/header.h @@ -38,6 +38,7 @@ enum { HEADER_CACHE, HEADER_SAMPLE_TIME, HEADER_MEM_TOPOLOGY, + HEADER_CLOCKID, HEADER_LAST_FEATURE, HEADER_FEAT_BITS = 256, }; ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4] perf record: encode -k clockid frequency into Perf trace 2018-10-09 6:59 [PATCH v4] perf record: encode -k clockid frequency into Perf trace Alexey Budankov @ 2018-10-09 13:41 ` Arnaldo Carvalho de Melo 2018-10-09 14:36 ` Alexey Budankov 0 siblings, 1 reply; 3+ messages in thread From: Arnaldo Carvalho de Melo @ 2018-10-09 13:41 UTC (permalink / raw) To: Alexey Budankov Cc: Peter Zijlstra, Ingo Molnar, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Andi Kleen, linux-kernel Em Tue, Oct 09, 2018 at 09:59:03AM +0300, Alexey Budankov escreveu: > > Store -k clockid frequency into Perf trace to enable timestamps > derived metrics conversion into wall clock time on reporting stage. > > Below is the example of perf report output: Some questions below, left the code for easier reference. > tools/perf/perf record -k raw -- ../../matrix/linux/matrix.gcc > ... > [ perf record: Captured and wrote 31.222 MB perf.data (818054 samples) ] > > tools/perf/perf report --header > # ======== > ... > # event : name = cycles:ppp, , size = 112, { sample_period, sample_freq } = 4000, sample_type = IP|TID|TIME|PERIOD, disabled = 1, inherit = 1, mmap = 1, comm = 1, freq = 1, enable_on_exec = 1, task = 1, precise_ip = 3, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1, use_clockid = 1, clockid = 4 > ... > # clockid frequency: 1000 MHz > ... > # ======== > > Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com> > --- > Changes in v4: > - addressed compilation issues on 32bit platforms > Changes in v3: > - moved header's clockid_res_ns initialization out of record__init_features() > - added explicit warning for case of failed clock_getres() call > Changes in v2: > - renamed clockid_freq to clockid_res_ns and get_clockid_freq() to get_clockid_res() > - avoided redundant define of NSEC_IN_SEC, reused linux/time64.h:NSEC_PER_SEC > - moved MHz conversion into print_clockid() and shortened write_clockid() > --- > tools/perf/builtin-record.c | 24 ++++++++++++++++++++++-- > tools/perf/perf.h | 1 + > tools/perf/util/env.h | 1 + > tools/perf/util/header.c | 28 ++++++++++++++++++++++++++++ > tools/perf/util/header.h | 1 + > 5 files changed, 53 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > index 0980dfe3396b..d803d3264465 100644 > --- a/tools/perf/builtin-record.c > +++ b/tools/perf/builtin-record.c > @@ -592,6 +592,9 @@ static void record__init_features(struct record *rec) > if (!rec->opts.full_auxtrace) > perf_header__clear_feat(&session->header, HEADER_AUXTRACE); > > + if (!(rec->opts.use_clockid && rec->opts.clockid_res_ns)) > + perf_header__clear_feat(&session->header, HEADER_CLOCKID); > + > perf_header__clear_feat(&session->header, HEADER_STAT); > } > > @@ -897,6 +900,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv) > > record__init_features(rec); > > + if (rec->opts.use_clockid && rec->opts.clockid_res_ns) > + session->header.env.clockid_res_ns = rec->opts.clockid_res_ns; > + > if (forks) { > err = perf_evlist__prepare_workload(rec->evlist, &opts->target, > argv, data->is_pipe, > @@ -1337,6 +1343,19 @@ static const struct clockid_map clockids[] = { > CLOCKID_END, > }; > > +static int get_clockid_res(clockid_t clk_id, size_t *res_ns) > +{ > + struct timespec res; > + > + *res_ns = 0; > + if (!clock_getres(clk_id, &res)) > + *res_ns = res.tv_nsec + res.tv_sec * NSEC_PER_SEC; > + else > + pr_warning("WARNING: Failed to determine specified clock resolution.\n"); > + > + return 0; > +} > + > static int parse_clockid(const struct option *opt, const char *str, int unset) > { > struct record_opts *opts = (struct record_opts *)opt->value; > @@ -1360,7 +1379,7 @@ static int parse_clockid(const struct option *opt, const char *str, int unset) > > /* if its a number, we're done */ > if (sscanf(str, "%d", &opts->clockid) == 1) > - return 0; > + return get_clockid_res(opts->clockid, &opts->clockid_res_ns); > > /* allow a "CLOCK_" prefix to the name */ > if (!strncasecmp(str, "CLOCK_", 6)) > @@ -1369,7 +1388,8 @@ static int parse_clockid(const struct option *opt, const char *str, int unset) > for (cm = clockids; cm->name; cm++) { > if (!strcasecmp(str, cm->name)) { > opts->clockid = cm->clockid; > - return 0; > + return get_clockid_res(opts->clockid, > + &opts->clockid_res_ns); > } > } > > diff --git a/tools/perf/perf.h b/tools/perf/perf.h > index 21bf7f5a3cf5..981db3c2ed60 100644 > --- a/tools/perf/perf.h > +++ b/tools/perf/perf.h > @@ -81,6 +81,7 @@ struct record_opts { > unsigned initial_delay; > bool use_clockid; > clockid_t clockid; > + size_t clockid_res_ns; > unsigned int proc_map_timeout; > }; > > diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h > index 1f3ccc368530..b167a54d635f 100644 > --- a/tools/perf/util/env.h > +++ b/tools/perf/util/env.h > @@ -63,6 +63,7 @@ struct perf_env { > struct numa_node *numa_nodes; > struct memory_node *memory_nodes; > unsigned long long memory_bsize; > + size_t clockid_res_ns; > }; > > extern struct perf_env perf_env; > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c > index 1ec1d9bc2d63..702e0cce63da 100644 > --- a/tools/perf/util/header.c > +++ b/tools/perf/util/header.c > @@ -1034,6 +1034,14 @@ static int write_auxtrace(struct feat_fd *ff, > return err; > } > > +static int write_clockid(struct feat_fd *ff, > + struct perf_evlist *evlist __maybe_unused) > +{ > + u64 clockid_res_ns = ff->ph->env.clockid_res_ns; > + > + return do_write(ff, &clockid_res_ns, sizeof(clockid_res_ns)); > +} > + > static int cpu_cache_level__sort(const void *a, const void *b) > { > struct cpu_cache_level *cache_a = (struct cpu_cache_level *)a; > @@ -1508,6 +1516,12 @@ static void print_cpu_topology(struct feat_fd *ff, FILE *fp) > fprintf(fp, "# Core ID and Socket ID information is not available\n"); > } > > +static void print_clockid(struct feat_fd *ff, FILE *fp) > +{ > + fprintf(fp, "# clockid frequency: %zd MHz\n", > + ff->ph->env.clockid_res_ns * 1000); > +} > + > static void free_event_desc(struct perf_evsel *events) > { > struct perf_evsel *evsel; > @@ -2531,6 +2545,19 @@ static int process_mem_topology(struct feat_fd *ff, > return ret; > } > > +static int process_clockid(struct feat_fd *ff, > + void *data __maybe_unused) > +{ > + u64 clockid_res_ns; > + > + if (do_read_u64(ff, &clockid_res_ns)) > + return -1; > + > + ff->ph->env.clockid_res_ns = *(size_t *)&clockid_res_ns; What is wrong with: ff->ph->env.clockid_res_ns = clockid_res_ns; ? Also why have you picked 'size_t' for that nanoseconds variable (clockid_res_ns)? When one looks at things like: struct timespec { time_t tv_sec; /* seconds */ long tv_nsec; /* nanoseconds */ }; that would lead one to use 'long' for a variable holding nanoseconds, no? And 'timespec' is what clock_getres() uses: int clock_getres(clockid_t clk_id, struct timespec *res); Since you're storing this as: +static int get_clockid_res(clockid_t clk_id, size_t *res_ns) +{ + struct timespec res; + + *res_ns = 0; + if (!clock_getres(clk_id, &res)) + *res_ns = res.tv_nsec + res.tv_sec * NSEC_PER_SEC; Why don't you make ff->ph->env.clockid_res_ns an 'u64'? Would simplify the code a bit, no? Am I missing something? - Arnaldo > + return 0; > +} > + > struct feature_ops { > int (*write)(struct feat_fd *ff, struct perf_evlist *evlist); > void (*print)(struct feat_fd *ff, FILE *fp); > @@ -2590,6 +2617,7 @@ static const struct feature_ops feat_ops[HEADER_LAST_FEATURE] = { > FEAT_OPN(CACHE, cache, true), > FEAT_OPR(SAMPLE_TIME, sample_time, false), > FEAT_OPR(MEM_TOPOLOGY, mem_topology, true), > + FEAT_OPR(CLOCKID, clockid, false) > }; > > struct header_print_data { > diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h > index e17903caa71d..0d553ddca0a3 100644 > --- a/tools/perf/util/header.h > +++ b/tools/perf/util/header.h > @@ -38,6 +38,7 @@ enum { > HEADER_CACHE, > HEADER_SAMPLE_TIME, > HEADER_MEM_TOPOLOGY, > + HEADER_CLOCKID, > HEADER_LAST_FEATURE, > HEADER_FEAT_BITS = 256, > }; ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v4] perf record: encode -k clockid frequency into Perf trace 2018-10-09 13:41 ` Arnaldo Carvalho de Melo @ 2018-10-09 14:36 ` Alexey Budankov 0 siblings, 0 replies; 3+ messages in thread From: Alexey Budankov @ 2018-10-09 14:36 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Peter Zijlstra, Ingo Molnar, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Andi Kleen, linux-kernel Hi, On 09.10.2018 16:41, Arnaldo Carvalho de Melo wrote: > Em Tue, Oct 09, 2018 at 09:59:03AM +0300, Alexey Budankov escreveu: >> >> Store -k clockid frequency into Perf trace to enable timestamps >> derived metrics conversion into wall clock time on reporting stage. >> >> Below is the example of perf report output: > > Some questions below, left the code for easier reference. > >> tools/perf/perf record -k raw -- ../../matrix/linux/matrix.gcc >> ... >> [ perf record: Captured and wrote 31.222 MB perf.data (818054 samples) ] >> >> tools/perf/perf report --header >> # ======== >> ... >> # event : name = cycles:ppp, , size = 112, { sample_period, sample_freq } = 4000, sample_type = IP|TID|TIME|PERIOD, disabled = 1, inherit = 1, mmap = 1, comm = 1, freq = 1, enable_on_exec = 1, task = 1, precise_ip = 3, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1, use_clockid = 1, clockid = 4 >> ... >> # clockid frequency: 1000 MHz >> ... >> # ========>> >> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com> >> --- >> Changes in v4: >> - addressed compilation issues on 32bit platforms >> Changes in v3: >> - moved header's clockid_res_ns initialization out of record__init_features() >> - added explicit warning for case of failed clock_getres() call >> Changes in v2: >> - renamed clockid_freq to clockid_res_ns and get_clockid_freq() to get_clockid_res() >> - avoided redundant define of NSEC_IN_SEC, reused linux/time64.h:NSEC_PER_SEC >> - moved MHz conversion into print_clockid() and shortened write_clockid() >> --- >> tools/perf/builtin-record.c | 24 ++++++++++++++++++++++-- >> tools/perf/perf.h | 1 + >> tools/perf/util/env.h | 1 + >> tools/perf/util/header.c | 28 ++++++++++++++++++++++++++++ >> tools/perf/util/header.h | 1 + >> 5 files changed, 53 insertions(+), 2 deletions(-) >> >> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c >> index 0980dfe3396b..d803d3264465 100644 >> --- a/tools/perf/builtin-record.c >> +++ b/tools/perf/builtin-record.c >> @@ -592,6 +592,9 @@ static void record__init_features(struct record *rec) >> if (!rec->opts.full_auxtrace) >> perf_header__clear_feat(&session->header, HEADER_AUXTRACE); >> >> + if (!(rec->opts.use_clockid && rec->opts.clockid_res_ns)) >> + perf_header__clear_feat(&session->header, HEADER_CLOCKID); >> + >> perf_header__clear_feat(&session->header, HEADER_STAT); >> } >> >> @@ -897,6 +900,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv) >> >> record__init_features(rec); >> >> + if (rec->opts.use_clockid && rec->opts.clockid_res_ns) >> + session->header.env.clockid_res_ns = rec->opts.clockid_res_ns; >> + >> if (forks) { >> err = perf_evlist__prepare_workload(rec->evlist, &opts->target, >> argv, data->is_pipe, >> @@ -1337,6 +1343,19 @@ static const struct clockid_map clockids[] = { >> CLOCKID_END, >> }; >> >> +static int get_clockid_res(clockid_t clk_id, size_t *res_ns) >> +{ >> + struct timespec res; >> + >> + *res_ns = 0; >> + if (!clock_getres(clk_id, &res)) >> + *res_ns = res.tv_nsec + res.tv_sec * NSEC_PER_SEC; >> + else >> + pr_warning("WARNING: Failed to determine specified clock resolution.\n"); >> + >> + return 0; >> +} >> + >> static int parse_clockid(const struct option *opt, const char *str, int unset) >> { >> struct record_opts *opts = (struct record_opts *)opt->value; >> @@ -1360,7 +1379,7 @@ static int parse_clockid(const struct option *opt, const char *str, int unset) >> >> /* if its a number, we're done */ >> if (sscanf(str, "%d", &opts->clockid) == 1) >> - return 0; >> + return get_clockid_res(opts->clockid, &opts->clockid_res_ns); >> >> /* allow a "CLOCK_" prefix to the name */ >> if (!strncasecmp(str, "CLOCK_", 6)) >> @@ -1369,7 +1388,8 @@ static int parse_clockid(const struct option *opt, const char *str, int unset) >> for (cm = clockids; cm->name; cm++) { >> if (!strcasecmp(str, cm->name)) { >> opts->clockid = cm->clockid; >> - return 0; >> + return get_clockid_res(opts->clockid, >> + &opts->clockid_res_ns); >> } >> } >> >> diff --git a/tools/perf/perf.h b/tools/perf/perf.h >> index 21bf7f5a3cf5..981db3c2ed60 100644 >> --- a/tools/perf/perf.h >> +++ b/tools/perf/perf.h >> @@ -81,6 +81,7 @@ struct record_opts { >> unsigned initial_delay; >> bool use_clockid; >> clockid_t clockid; >> + size_t clockid_res_ns; >> unsigned int proc_map_timeout; >> }; >> >> diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h >> index 1f3ccc368530..b167a54d635f 100644 >> --- a/tools/perf/util/env.h >> +++ b/tools/perf/util/env.h >> @@ -63,6 +63,7 @@ struct perf_env { >> struct numa_node *numa_nodes; >> struct memory_node *memory_nodes; >> unsigned long long memory_bsize; >> + size_t clockid_res_ns; >> }; >> >> extern struct perf_env perf_env; >> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c >> index 1ec1d9bc2d63..702e0cce63da 100644 >> --- a/tools/perf/util/header.c >> +++ b/tools/perf/util/header.c >> @@ -1034,6 +1034,14 @@ static int write_auxtrace(struct feat_fd *ff, >> return err; >> } >> >> +static int write_clockid(struct feat_fd *ff, >> + struct perf_evlist *evlist __maybe_unused) >> +{ >> + u64 clockid_res_ns = ff->ph->env.clockid_res_ns; >> + >> + return do_write(ff, &clockid_res_ns, sizeof(clockid_res_ns)); >> +} >> + >> static int cpu_cache_level__sort(const void *a, const void *b) >> { >> struct cpu_cache_level *cache_a = (struct cpu_cache_level *)a; >> @@ -1508,6 +1516,12 @@ static void print_cpu_topology(struct feat_fd *ff, FILE *fp) >> fprintf(fp, "# Core ID and Socket ID information is not available\n"); >> } >> >> +static void print_clockid(struct feat_fd *ff, FILE *fp) >> +{ >> + fprintf(fp, "# clockid frequency: %zd MHz\n", >> + ff->ph->env.clockid_res_ns * 1000); >> +} >> + >> static void free_event_desc(struct perf_evsel *events) >> { >> struct perf_evsel *evsel; >> @@ -2531,6 +2545,19 @@ static int process_mem_topology(struct feat_fd *ff, >> return ret; >> } >> >> +static int process_clockid(struct feat_fd *ff, >> + void *data __maybe_unused) >> +{ >> + u64 clockid_res_ns; >> + >> + if (do_read_u64(ff, &clockid_res_ns)) >> + return -1; >> + >> + ff->ph->env.clockid_res_ns = *(size_t *)&clockid_res_ns; > > What is wrong with: > > ff->ph->env.clockid_res_ns = clockid_res_ns; > > ? It can lead to implicit type size reduction, so compiler can report warning what breaks compilation. > > Also why have you picked 'size_t' for that nanoseconds variable > (clockid_res_ns)? > > When one looks at things like: > > struct timespec { > time_t tv_sec; /* seconds */ > long tv_nsec; /* nanoseconds */ > }; > > that would lead one to use 'long' for a variable holding nanoseconds, > no? Yes, long type for clockid_res_ns is what it was before I had started to prepare the patch :) > > And 'timespec' is what clock_getres() uses: > > int clock_getres(clockid_t clk_id, struct timespec *res); > > Since you're storing this as: > > +static int get_clockid_res(clockid_t clk_id, size_t *res_ns) > +{ > + struct timespec res; > + > + *res_ns = 0; > + if (!clock_getres(clk_id, &res)) > + *res_ns = res.tv_nsec + res.tv_sec * NSEC_PER_SEC; but then I thought a little bit more and came to conclusion that for this multiplication with addition would probably better have larger type than long. > > Why don't you make ff->ph->env.clockid_res_ns an 'u64'? Would simplify > the code a bit, no? and now I think that keeping things simple is the best approach. :) > > Am I missing something? Please see v5. Thanks! Have a good day, Alexey > > - Arnaldo > >> + return 0; >> +} >> + >> struct feature_ops { >> int (*write)(struct feat_fd *ff, struct perf_evlist *evlist); >> void (*print)(struct feat_fd *ff, FILE *fp); >> @@ -2590,6 +2617,7 @@ static const struct feature_ops feat_ops[HEADER_LAST_FEATURE] = { >> FEAT_OPN(CACHE, cache, true), >> FEAT_OPR(SAMPLE_TIME, sample_time, false), >> FEAT_OPR(MEM_TOPOLOGY, mem_topology, true), >> + FEAT_OPR(CLOCKID, clockid, false) >> }; >> >> struct header_print_data { >> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h >> index e17903caa71d..0d553ddca0a3 100644 >> --- a/tools/perf/util/header.h >> +++ b/tools/perf/util/header.h >> @@ -38,6 +38,7 @@ enum { >> HEADER_CACHE, >> HEADER_SAMPLE_TIME, >> HEADER_MEM_TOPOLOGY, >> + HEADER_CLOCKID, >> HEADER_LAST_FEATURE, >> HEADER_FEAT_BITS = 256, >> }; > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-10-09 14:36 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-09 6:59 [PATCH v4] perf record: encode -k clockid frequency into Perf trace Alexey Budankov 2018-10-09 13:41 ` Arnaldo Carvalho de Melo 2018-10-09 14:36 ` Alexey Budankov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).