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.budan...@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,
>> };
>
