Re: [PATCH v3 06/23] trace-cmd: Add new trace-cmd clock tsc2nsec

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 24 Mar 2021 15:04:01 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> @@ -199,6 +200,12 @@ struct filter_pids {
>  	int exclude;
>  };
>  
> +struct tsc_nsec {
> +	int mult;
> +	int shift;
> +	unsigned long long offset;
> +};
> +
>  struct buffer_instance {
>  	struct buffer_instance	*next;
>  	char			*name;
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 5f7f5b3d..2a594736 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -57,6 +57,8 @@
>  #define MAX_LATENCY	"tracing_max_latency"
>  #define STAMP		"stamp"
>  #define FUNC_STACK_TRACE "func_stack_trace"
> +#define TSC_CLOCK	"x86-tsc"
> +#define TSCNSEC_CLOCK	"tsc2nsec"
>  
>  enum trace_type {
>  	TRACE_TYPE_RECORD	= 1,
> @@ -198,6 +200,7 @@ struct common_record_context {
>  	char *date2ts;
>  	char *user;
>  	int data_flags;
> +	struct tsc_nsec tsc2nsec;

struct tsc_nsec is 16 bytes in size, ending with a 8 byte word. It should
be added before the "int data_flags" to prevent a hole in the middle of
this struct.


>  
>  	int record_all;
>  	int total_disable;


> @@ -5904,7 +5965,15 @@ static void parse_record_options(int argc,
>  			break;
>  		case 'C':
>  			check_instance_die(ctx->instance, "-C");
> -			ctx->instance->clock = optarg;
> +			if (!strncmp(optarg, TSCNSEC_CLOCK, strlen(TSCNSEC_CLOCK))) {

Hmm, why the strncmp()? Shouldn't it be a full match, not a partial one?

> +				ret = get_tsc_nsec(&ctx->tsc2nsec.shift,
> +						   &ctx->tsc2nsec.mult);
> +				if (ret || !clock_is_supported(NULL, TSC_CLOCK))
> +					die("Clock %s is not supported", optarg);
> +				ctx->instance->flags |= BUFFER_FL_TSC2NSEC;
> +				ctx->instance->clock = strdup(TSC_CLOCK);

Hmm, why the strdup? below we have clock = optarg, one of them is wrong. If
we free instance->clock, then we can't have it set to optarg. As that was
the way it was before, it looks to be a separate bug that probably needs
its own patch.

> +			} else
> +				ctx->instance->clock = optarg;

Actually, I think we should have the above be:

		case 'C':
			if (strcmp(optarg, TSCNSEC_CLOCK) == 0) {
				ret = get_tsc_nsec(&ctx->tsc2nsec.shift,
						   &ctx->tsc2nsec.mult);
				if (ret)
					die("TSC to nanosecond is not supported");

				ctx->instance->clock = TSC_CLOCK;
			} else {
				ctx->instance->clock = optarg;
			}
			if (!clock_is_supported(ctx->instance->clock))
				die("Clock %s is not supported",
				    ctx->instance->clock);
			ctx->instance->clock = strdup(ctx->instance->clock);
			if (!ctx->instance->clock)
				die("Failed allocation");

The above is more informative about the reason for the error, and also
removes duplicate code in the check of supported clocks.

-- Steve


>  			ctx->instance->flags |= BUFFER_FL_HAS_CLOCK;
>  			if (is_top_instance(ctx->instance))
>  				guest_sync_set = true;
> @@ -6159,6 +6228,13 @@ static void parse_record_options(int argc,
>  				die("--fork option used for 'start' command only");
>  			fork_process = true;
>  			break;
> +		case OPT_tsc2nsec:
> +			ret = get_tsc_nsec(&ctx->tsc2nsec.shift,
> +					   &ctx->tsc2nsec.mult);
> +			if (ret)
> +				die("TSC to nanosecond is not supported");
> +			ctx->instance->flags |= BUFFER_FL_TSC2NSEC;
> +			break;
>  		case OPT_quiet:
>  		case 'q':
>  			quiet = true;