Re: [PATCH v2] trace-cmd: Add record --clock option to specify a trace clock

[Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>]

Hi Ian,

(2013/12/13 8:30), Ian Webster wrote:
> This change adds a --clock option to trace-cmd record. It simply writes
> trace_clock on debugfs. Examples of valid choices on most systems are: local,
> global, counter, and any other choice compatible with ftrace.

Nice work!
Please see my following comments.

> This complements Yoshihiro Yunomae's change 8c9867c (trace-cmd: Add support for
> extracting trace_clock in report).
>
> Signed-off-by: Ian Webster <ian@planetaryresources.com>
>
> ---
> Changes per Steven's comments:
>    * add missing const
>    * preserve tab whitespace
>
>   Documentation/trace-cmd-record.1.txt |    5 +++++
>   trace-record.c                       |   19 +++++++++++++++++++
>   trace-usage.c                        |    1 +
>   3 files changed, 25 insertions(+)
>
> diff --git a/Documentation/trace-cmd-record.1.txt b/Documentation/trace-cmd-record.1.txt
> index 832a257..5e83454 100644
> --- a/Documentation/trace-cmd-record.1.txt
> +++ b/Documentation/trace-cmd-record.1.txt
> @@ -240,6 +240,11 @@ OPTIONS
>       timestamp to gettimeofday which will allow wall time output from the
>       timestamps reading the created 'trace.dat' file.
>
> +*--clock* 'clock_name'::
> +    When an event is recorded into the ring buffer, a timestamp is added. This
> +    stamp comes from the specified clock. Ftrace uses the "local" clock by
> +    default. Other common clocks include: global, counter, uptime, perf, x86-tsc.
> +
>   EXAMPLES
>   --------
>
> diff --git a/trace-record.c b/trace-record.c
> index 0199627..2143080 100644
> --- a/trace-record.c
> +++ b/trace-record.c
> @@ -399,6 +399,20 @@ static int set_ftrace(int set, int use_proc)
>   	return 0;
>   }
>
> +static void set_trace_clock(const char *clock_choice)
> +{
> +	FILE *fp;
> +	char *path;
> +
> +	path = tracecmd_get_tracing_file("trace_clock");
> +	fp = fopen(path, "w");
> +	if (!fp)
> +		die("writing to '%s'", path);
> +	tracecmd_put_tracing_file(path);
> +	fwrite(clock_choice, 1, strlen(clock_choice), fp);

Please add an error check for fwrite(). If clock_choice is invalid,
trace-cmd would better output the error report and die.

> +	fclose(fp);
> +}
> +
>   static char *
>   get_instance_file(struct buffer_instance *instance, const char *file)
>   {
> @@ -2279,6 +2293,7 @@ static void record_all_events(void)
>   }
>
>   enum {
> +	OPT_clock = 252,
>   	OPT_nosplice	= 253,
>   	OPT_funcstack	= 254,
>   	OPT_date	= 255,
> @@ -2351,6 +2366,7 @@ void trace_record (int argc, char **argv)
>   			{"func-stack", no_argument, NULL, OPT_funcstack},
>   			{"nosplice", no_argument, NULL, OPT_nosplice},
>   			{"help", no_argument, NULL, '?'},
> +			{"clock", required_argument, NULL, OPT_clock},
>   			{NULL, 0, NULL, 0}
>   		};
>
> @@ -2530,6 +2546,9 @@ void trace_record (int argc, char **argv)
>   		case OPT_nosplice:
>   			recorder_flags |= TRACECMD_RECORD_NOSPLICE;
>   			break;
> +		case OPT_clock:
> +			set_trace_clock(optarg);
> +			break;

This implementation does not support multiple buffers.
trace_clock had better be changed after sub-buffers are created, I
think.

Example:
trace-cmd record -e sched --clock local -B foo -e block --clock x86-tsc

Thanks,
Yoshihiro YUNOMAE

-- 
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@hitachi.com