linux-kernel - Re: [PATCH 3/3] perf tools: Introduce timestamp_in

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8d5bccf4-df81-c4f0-a836-d717b7ebb137@codeaurora.org>
Date:   Mon, 24 Oct 2016 13:56:07 -0700
From:   Joonwoo Park <joonwoop@...eaurora.org>
To:     Namhyung Kim <namhyung@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Jiri Olsa <jolsa@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCH 3/3] perf tools: Introduce timestamp_in_usec()



On 10/23/2016 07:02 PM, Namhyung Kim wrote:
> Joonwoo reported that there's a mismatch between timestamps in script
> and sched commands.  This was because of difference in printing the
> timestamp.  Factor out the code and share it so that they can be in
> sync.  Also I found that sched map has similar problem, fix it too.
>
> Reported-by: Joonwoo Park <joonwoop@...eaurora.org>

Sorry I was busy with something else so didn't have chance to follow up 
on my initial proposal and thanks for take caring of this.

> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
>  tools/perf/builtin-sched.c  | 9 ++++++---
>  tools/perf/builtin-script.c | 9 ++++++---
>  tools/perf/util/util.c      | 9 +++++++++
>  tools/perf/util/util.h      | 3 +++
>  4 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 97d6cbf486bb..c88d64ae997e 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -1191,6 +1191,7 @@ static void output_lat_thread(struct perf_sched *sched, struct work_atoms *work_
>  	int i;
>  	int ret;
>  	u64 avg;
> +	char buf[32];
>
>  	if (!work_list->nb_atoms)
>  		return;
> @@ -1213,11 +1214,11 @@ static void output_lat_thread(struct perf_sched *sched, struct work_atoms *work_
>
>  	avg = work_list->total_lat / work_list->nb_atoms;
>
> -	printf("|%11.3f ms |%9" PRIu64 " | avg:%9.3f ms | max:%9.3f ms | max at: %13.6f s\n",
> +	printf("|%11.3f ms |%9" PRIu64 " | avg:%9.3f ms | max:%9.3f ms | max at: %13s s\n",
>  	      (double)work_list->total_runtime / NSEC_PER_MSEC,
>  		 work_list->nb_atoms, (double)avg / NSEC_PER_MSEC,
>  		 (double)work_list->max_lat / NSEC_PER_MSEC,
> -		 (double)work_list->max_lat_at / NSEC_PER_SEC);
> +		 timestamp_in_usec(buf, sizeof(buf), work_list->max_lat));

This should be :
s/work_list->max_lat/work_list->max_lat_at/

>  }
>
>  static int pid_cmp(struct work_atoms *l, struct work_atoms *r)
> @@ -1402,6 +1403,7 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,
>  	int cpus_nr;
>  	bool new_cpu = false;
>  	const char *color = PERF_COLOR_NORMAL;
> +	char buf[32];
>
>  	BUG_ON(this_cpu >= MAX_CPUS || this_cpu < 0);
>
> @@ -1492,7 +1494,8 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,
>  	if (sched->map.cpus && !cpu_map__has(sched->map.cpus, this_cpu))
>  		goto out;
>
> -	color_fprintf(stdout, color, "  %12.6f secs ", (double)timestamp / NSEC_PER_SEC);
> +	color_fprintf(stdout, color, "  %12s secs ",
> +		      timestamp_in_usec(buf, sizeof(buf), timestamp));
>  	if (new_shortname || (verbose && sched_in->tid)) {
>  		const char *pid_color = color;
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 7228d141a789..c848c74bdc90 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -437,7 +437,6 @@ static void print_sample_start(struct perf_sample *sample,
>  {
>  	struct perf_event_attr *attr = &evsel->attr;
>  	unsigned long secs;
> -	unsigned long usecs;
>  	unsigned long long nsecs;
>
>  	if (PRINT_FIELD(COMM)) {
> @@ -464,14 +463,18 @@ static void print_sample_start(struct perf_sample *sample,
>  	}
>
>  	if (PRINT_FIELD(TIME)) {
> +		char buf[32];
> +		size_t sz = sizeof(buf);
> +
>  		nsecs = sample->time;
>  		secs = nsecs / NSEC_PER_SEC;
>  		nsecs -= secs * NSEC_PER_SEC;
> -		usecs = nsecs / NSEC_PER_USEC;
> +
>  		if (nanosecs)
>  			printf("%5lu.%09llu: ", secs, nsecs);
>  		else
> -			printf("%5lu.%06lu: ", secs, usecs);
> +			printf("%12s: ", timestamp_in_usec(buf, sz,
> +							   sample->time));
>  	}
>  }
>
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 85c56800f17a..aa3e778989ce 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -433,6 +433,15 @@ int parse_nsec_time(const char *str, u64 *ptime)
>  	return 0;
>  }
>
> +char *timestamp_in_usec(char *buf, size_t sz, u64 timestamp)

I agree with Jirka.  timestamp_usec__scnprintf looks better.

Thanks,
Joonwoo

> +{
> +	u64  sec = timestamp / NSEC_PER_SEC;
> +	u64 usec = (timestamp % NSEC_PER_SEC) / NSEC_PER_USEC;
> +
> +	scnprintf(buf, sz, "%"PRIu64".%06"PRIu64, sec, usec);
> +	return buf;
> +}
> +
>  unsigned long parse_tag_value(const char *str, struct parse_tag *tags)
>  {
>  	struct parse_tag *i = tags;
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 71b6992f1d98..ece974f1c538 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -362,4 +362,7 @@ extern int sched_getcpu(void);
>  #endif
>
>  int is_printable_array(char *p, unsigned int len);
> +
> +char *timestamp_in_usec(char *buf, size_t sz, u64 timestamp);
> +
>  #endif /* GIT_COMPAT_UTIL_H */
>