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

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

Forgot to comment on that in the previous reply :

On Tue, Dec 10, 2019 at 8:39 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
[ ... ]
> > +
> > +unsigned int tracecmd_guest_tsync(unsigned int tsync_protos, 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);
> > +     if (!proto)
> > +             return 0;
> > +#ifdef VSOCK
>
> Instead of this, perhaps we should not have this file get compiled if
> VSOCK is not defined?

I would recommend to isolate VSOCK defines inside the time synchronization code.
The caller might not be aware of time synchronization internal dependencies.
The current implementation relies on vsoks, but we may change this in
the future and
this should not affect the callers.

>
> -- Steve
>
>
> > +     fd = make_vsock(VMADDR_PORT_ANY);
> > +     if (fd < 0)
> > +             goto error;
> > +
> > +     ret = get_vsock_port(fd, tsync_port);
> > +     if (ret < 0)
> > +             goto error;
> > +#else
> > +     return 0;
> > +#endif
> > +
> > +     tsync = calloc(1, sizeof(struct tracecmd_time_sync));
> > +     tsync->msg_handle = tracecmd_msg_handle_alloc(fd, 0);
> > +     if (clock)
> > +             tsync->clock_str = strdup(clock);
> > +
> > +     pthread_attr_init(&attrib);
> > +     tsync->sync_proto = proto;
> > +     pthread_attr_setdetachstate(&attrib, PTHREAD_CREATE_JOINABLE);
> > +     if (!get_first_cpu(&pin_mask, &mask_size))
> > +             pthread_attr_setaffinity_np(&attrib, mask_size, pin_mask);
> > +
> > +     ret = pthread_create(thr_id, &attrib, tsync_agent_thread, tsync);
> > +
> > +     if (pin_mask)
> > +             CPU_FREE(pin_mask);
> > +     pthread_attr_destroy(&attrib);
> > +
> > +     if (ret)
> > +             goto error;
> > +
> > +     return proto;
> > +
> > +error:
> > +     if (tsync) {
> > +             if (tsync->msg_handle)
> > +                     tracecmd_msg_handle_close(tsync->msg_handle);
> > +             if (tsync->clock_str)
> > +                     free(tsync->clock_str);
> > +             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