On Wed, Mar 24, 2021 at 5:20 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Wed, 24 Mar 2021 15:04:01 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> 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.
Actually, instance->clock is never freed - because there is no
function to free an instance. There is allocate_instance(), but no API
to free. That's why both are valid. I was wondering if to use strdup
or simple assignment, and decided to allocate a memory. One day we may
implement free_instance(), there are a lot of resources in an instance
that should be freed.
>
> > + } 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;
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
|