Re: [PATCH v18 16/18] trace-cmd: Basic infrastructure for host - guest timestamp synchronization

[Tzvetomir Stoyanov <tstoyanov@vmware.com>]

On Sat, Dec 21, 2019 at 6:48 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
 [ .. ]
> > +#define PROTO_MASK_SIZE (sizeof(char))
>
> I'm thinking that declaring a mask the size of 1 is a bit overkill.
>
This define is a leftover from the previous implementation of this
bitmask, where
the size was 4 bytes. It was very easy to switch to 1 byte using the
define, that's
why I would prefer to keep it - in case we decide to change the size again.
The code below is written with the assumption that the size could be
more than 1 byte.

[ .. ]
> > + */
> > +void tracecmd_tsync_free(struct tracecmd_time_sync *tsync)
> > +{
> > +     struct clock_sync_context *tsync_context;
> > +     struct tsync_proto *proto;
> > +
> > +     if (!tsync->context)
> > +             return;
> > +     tsync_context = (struct clock_sync_context *)tsync->context;
> > +
> > +     proto = tsync_proto_find(tsync->sync_proto);
> > +     if (proto && proto->clock_sync_free)
> > +             proto->clock_sync_free(tsync);
> > +
> > +     clock_synch_delete_instance(tsync_context->vinst);
> > +     tsync_context->vinst = NULL;
> > +
> > +     free(tsync_context->sync_ts);
> > +     free(tsync_context->sync_offsets);
> > +     tsync_context->sync_ts = NULL;
> > +     tsync_context->sync_offsets = NULL;
> > +     tsync_context->sync_count = 0;
> > +     tsync_context->sync_size = 0;
> > +     pthread_mutex_destroy(&tsync->lock);
> > +     pthread_cond_destroy(&tsync->cond);
> > +     free(tsync->clock_str);
>
> I would think we would want a free(tsync) here. As the name of the
> function suggests.
>
There is no API to allocate it, that's why this function does not free
the memory. There is one use case where this memory is not allocated,
and free(tsync) will not work for it.

[ .. ]
> > +
> > +unsigned int tracecmd_guest_tsync(char *tsync_protos,
> > +                               unsigned int tsync_protos_size, char *clock,
> > +                               unsigned int *tsync_port, pthread_t *thr_id)
> > +{
> > +     struct tracecmd_time_sync *tsync = NULL;
> > +     cpu_set_t *pin_mask = NULL;
> > +     pthread_attr_t attrib;
> > +     size_t mask_size = 0;
> > +     unsigned int proto;
> > +     int ret;
> > +     int fd;
> > +
> > +     fd = -1;
> > +     proto = tracecmd_tsync_proto_select(tsync_protos, tsync_protos_size);
> > +     if (!proto)
> > +             return 0;
> > +#ifdef VSOCK
> > +     fd = trace_make_vsock(VMADDR_PORT_ANY);
> > +     if (fd < 0)
> > +             goto error;
> > +
> > +     ret = trace_get_vsock_port(fd, tsync_port);
> > +     if (ret < 0)
> > +             goto error;
> > +#else
> > +     return 0;
>
> If we have no synchronization support, shouldn't this return an error?

This function returns the id of negotiated time sync protocol. 0 means the
negotiation was not successful, no protocol is selected. When the caller
receives 0, it means we have no synchronization with the peer.

>
[ .. ]
>
> You don't need the if statement. free() can take a NULL pointer.
>
> Anyway, it's looking good. I like a lot of what you did. Especially
> with the helpers you created.
>
> I still need to take a deeper look at this and the following patches.
> I'm thinking that this patch could possibly be broken up into two or
> three patches. One patch to create the protocol, another one or two
> that use the protocol.
>
> Although we are very close to getting this in, I've been thinking more
> that we should release 2.9, without the time sync. We may need to just
> up the protocol when we implement the synchronization.
>
> I'll look more at this on Monday.
>
> Cheers!
>
> -- Steve
>
Thanks, Steven!
I'll  send the next  version, addressing your comments.
>
>
> > +             free(tsync);
> > +     }
> > +     if (fd > 0)
> > +             close(fd);
> > +     return 0;
> > +}
> > diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
> > index 05ec021..9fa61e1 100644
> > --- a/tracecmd/trace-usage.c
> > +++ b/tracecmd/trace-usage.c
> > @@ -60,6 +60,10 @@ static struct usage_help usage_help[] = {
> >               "          --no-filter include trace-cmd threads in the trace\n"
> >               "          --proc-map save the traced processes address map into the trace.dat file\n"
> >               "          --user execute the specified [command ...] as given user\n"
> > +             "          --tsync-interval set the loop interval, in ms, for timestamps synchronization with guests:"
> > +             "               If a negative number is specified, timestamps synchronization is disabled"
> > +             "               If 0 is specified, no loop is performed - timestamps offset is calculated only twice,"
> > +             "                                                         at the beginnig and at the end of the trace\n"
> >       },
> >       {
> >               "start",
>


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