[PATCH v4] perf record: encode -k clockid frequency into Perf trace

[PATCH v4] perf record: encode -k clockid frequency into Perf trace

[Alexey Budankov]

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,
 };

Re: [PATCH v4] perf record: encode -k clockid frequency into Perf trace

[Arnaldo Carvalho de Melo]

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,
>  };

Re: [PATCH v4] perf record: encode -k clockid frequency into Perf trace

[Alexey Budankov]

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,
>>  };
>