Re: [PATCH v17 12/18] trace-cmd: Implement new option in trace.dat file: TRACECMD_OPTION_TIME_SHIFT

[Tzvetomir Stoyanov <tz.stoyanov@gmail.com>]

On Thu, Dec 5, 2019 at 2:46 AM Steven Rostedt <rostedt@goodmis.org> wrote:
[ ... ]
> > +static inline unsigned long long
> > +timestamp_correction_calc(unsigned long long ts, struct ts_offset_sample *min,
> > +                       struct ts_offset_sample *max)
> > +{
> > +     long long tscor = min->offset +
> > +                     (((((long long)ts) - min->time)*
> > +                     (max->offset-min->offset))/(max->time-min->time));
>
> When every I see a division like this, I'd like to think we should add:
>
>         long long offset = ((long long)ts - min->time) *
>                         (max->offset - min->offset);
>         long long delta = max->time - min->time;
>         long long tscor = min->offset +
>                         (offset + delta / 2) / delta;
>
> This handles rounding of delta instead of truncating. Also, it's best
> to have spaces around operators, as it's hard to see where the variable
> stops and the operation beings with max->time-min->time.
>
> > +
> > +     if (tscor < 0)
> > +             return ts - llabs(tscor);
>
> I'm curious to why the test and using llabs, instead of just returning:
>
>         ts + tscor ?
>
> Is there a difference between that when tscor is negative and using
> llabs?
>
The only reason to use llabs() is mixed signed - unsigned calculation. As time
correction tscor can be negative,  will it be converted to unsigned in
"ts + tscor" ?

> > +
> > +     return ts + tscor;
> > +}
> > +
> > +static unsigned long long timestamp_correct(unsigned long long ts,
> > +                                         struct tracecmd_input *handle)
> > +{
> > +     struct host_trace_info  *host = &handle->host;
> > +     int min, mid, max;
> > +
> > +     if (handle->ts_offset)
> > +             return ts + handle->ts_offset;
> > +
> > +     if (!host->sync_enable || !host->ts_samples_count || !host->ts_samples)
> > +             return ts;
>
> Hmm, perhaps we should make host->sync_enable false when
> host->ts_samples_count or host->ts_samples are NULL and remove the
> extra checks? (slight optimization)
>
> > +
> > +     /* We have one sample, nothing to calc here */
> > +     if (host->ts_samples_count == 1)
> > +             return ts + host->ts_samples[0].offset;
> > +
> > +     /* We have two samples, nothing to search here */
> > +     if (host->ts_samples_count == 2)
> > +             return timestamp_correction_calc(ts, &host->ts_samples[0],
> > +                                              &host->ts_samples[1]);
> > +
> > +     /* We have more than two samples */
> > +     if (ts <= host->ts_samples[0].time)
> > +             return timestamp_correction_calc(ts,
> > +                                              &host->ts_samples[0],
> > +                                              &host->ts_samples[1]);
> > +     else if (ts >= host->ts_samples[host->ts_samples_count-1].time)
> > +             return timestamp_correction_calc(ts,
> > +                                              &host->ts_samples[host->ts_samples_count-2],
> > +                                              &host->ts_samples[host->ts_samples_count-1]);
> > +     min = 0;
> > +     max = host->ts_samples_count-1;
> > +     mid = (min + max)/2;
> > +     while (min <= max) {
> > +             if (ts < host->ts_samples[mid].time)
> > +                     max = mid - 1;
> > +             else if (ts > host->ts_samples[mid].time)
> > +                     min = mid + 1;
> > +             else
> > +                     break;
> > +             mid = (min + max)/2;
> > +     }
>
> Hmm, probably should libc's bsearch() instead of open coding a binary
> search.
>

I do not use bsearch() as there is no exact match it that search - only the
interval in which the timestamp fits should be found. May be there is a way
to use bsearch() in this scenario, I can think about it.

[ ...]

> >  }
> >
> > +static int tsync_offset_cmp(const void *a, const void *b)
> > +{
> > +     struct ts_offset_sample *ts_a = (struct ts_offset_sample *)a;
> > +     struct ts_offset_sample *ts_b = (struct ts_offset_sample *)b;
> > +
> > +     if (ts_a->time > ts_b->time)
> > +             return 1;
> > +     if (ts_a->time < ts_b->time)
> > +             return -1;
> > +     return 0;
> > +}
>
> We have the compare function for bsearch() here.
>

We can use it only for exact match, cannot use the same function for finding the
interval.

[ ... ]

> >  static int trace_pid_map_cmp(const void *a, const void *b)
> >  {
> >       struct tracecmd_proc_addr_map *m_a = (struct tracecmd_proc_addr_map *)a;
> > @@ -2323,6 +2438,7 @@ static int handle_options(struct tracecmd_input *handle)
> >       struct input_buffer_instance *buffer;
> >       struct hook_list *hook;
> >       char *buf;
> > +     int sampes_size;
>
> "sampes"? Is this short for "samples"? Just use "samples_size" then.
>

It's a typo, fixed it :)

>
> >       int cpus;
> >
> >       /* By default, use usecs, unless told otherwise */
> > @@ -2370,6 +2486,28 @@ static int handle_options(struct tracecmd_input *handle)
> >                       offset = strtoll(buf, NULL, 0);
> >                       handle->ts_offset += offset;
> >                       break;
> > +             case TRACECMD_OPTION_TIME_SHIFT:
> > +                     /*
> > +                      * long long int (8 bytes) trace session ID
> > +                      * int (4 bytes) count of timestamp offsets.
> > +                      * long long array of size [count] of times,
> > +                      *      when the offsets were calculated.
> > +                      * long long array of size [count] of timestamp offsets.
> > +                      */
> > +                     if (size < 12 || handle->flags & TRACECMD_FL_IGNORE_DATE)
> > +                             break;
> > +                     handle->host.trace_id = tep_read_number(handle->pevent,
> > +                                                             buf, 8);
> > +                     handle->host.ts_samples_count = tep_read_number(handle->pevent,
> > +                                                                     buf + 8, 4);
> > +                     sampes_size = (8 * handle->host.ts_samples_count);
> > +                     if (size != (12 + (2 * sampes_size)))
> > +                             break;
> > +                     handle->host.ts_samples = malloc(2 * sampes_size);
> > +                     if (!handle->host.ts_samples)
> > +                             return -ENOMEM;
> > +                     tsync_offset_load(handle, buf + 12);
> > +                     break;
> >               case TRACECMD_OPTION_CPUSTAT:
> >                       buf[size-1] = '\n';
> >                       cpustats = realloc(cpustats, cpustats_size + size + 1);
> > @@ -3078,6 +3216,8 @@ void tracecmd_close(struct tracecmd_input *handle)
> >       trace_pid_map_free(handle->pid_maps);
> >       handle->pid_maps = NULL;
> >
> > +     trace_tsync_offset_free(&handle->host);
> > +
> >       if (handle->flags & TRACECMD_FL_BUFFER_INSTANCE)
> >               tracecmd_close(handle->parent);
> >       else {
> > @@ -3532,3 +3672,29 @@ unsigned long long tracecmd_get_traceid(struct tracecmd_input *handle)
> >  {
> >       return handle->trace_id;
> >  }
> > +
> > +/**
> > + * tracecmd_get_tsync_peer - get the trace session id of the peer host
> > + * @handle: input handle for the trace.dat file
> > + *
> > + * Returns the trace id of the peer host, written in the trace file
> > + *
> > + * This information is stored in guest trace.dat file
> > + */
> > +unsigned long long tracecmd_get_tsync_peer(struct tracecmd_input *handle)
> > +{
> > +     return handle->host.trace_id;
> > +}
> > +
> > +/**
> > + * tracecmd_enable_tsync - enable / disable the timestamps correction
> > + * @handle: input handle for the trace.dat file
> > + * @enable: enable / disable the timestamps correction
> > + *
> > + * Enables or disables timestamps correction on file load, using the array of
> > + * recorded time offsets
> > + */
> > +void tracecmd_enable_tsync(struct tracecmd_input *handle, bool enable)
> > +{
>
> Perhaps here we should check if samples are allocated already, and only
> allow it to be enabled if they are.
>
> -- Steve
>
>
> > +     handle->host.sync_enable = enable;
> > +}
>

Thanks !

-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center