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

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 31 Jan 2020 09:53:41 +0000
Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:

> 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.

OK, that makes more sense.

> 
> [ .. ]
> > > + */
> > > +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.

Then perhaps we should call it: tracecmd_tsync_cleanup(), as that's
what it is doing. _free() usually means you are actually freeing what
you pass in.


> 
> [ .. ]
> > > +
> > > +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.

OK, so this function definitely needs a "kernel doc" header explaining
this.

-- Steve