On Tue, Mar 19, 2019 at 7:14 PM Slavomir Kaslev <kaslevs@xxxxxxxxxx> wrote:
>
> On Tue, Mar 19, 2019 at 5:56 PM Tzvetomir Stoyanov <tstoyanov@xxxxxxxxxx> wrote:
> >
> > This is a POC patch, implementing an algorithm for syncing timestamps between
> > host and guest machines, using vsock trace events to catch the host / guest time.
> >
> > Signed-off-by: Tzvetomir Stoyanov <tstoyanov@xxxxxxxxxx>
> > ---
> > include/trace-cmd/trace-cmd.h | 21 +-
> > tracecmd/Makefile | 1 +
> > tracecmd/include/trace-local.h | 33 +-
> > tracecmd/trace-agent.c | 13 +-
> > tracecmd/trace-msg.c | 208 ++++++++-
> > tracecmd/trace-record.c | 85 +++-
> > tracecmd/trace-timesync.c | 825 +++++++++++++++++++++++++++++++++
> > 7 files changed, 1150 insertions(+), 36 deletions(-)
> > create mode 100644 tracecmd/trace-timesync.c
> >
> > --- a/tracecmd/Makefile
> > +++ b/tracecmd/Makefile
> > @@ -32,6 +32,7 @@ TRACE_CMD_OBJS += trace-list.o
> > TRACE_CMD_OBJS += trace-output.o
> > TRACE_CMD_OBJS += trace-usage.o
> > TRACE_CMD_OBJS += trace-msg.o
> > +TRACE_CMD_OBJS += trace-timesync.o
>
> Shouldn't this be on only if $VSOCK_DEFINED, same as trace-agent.o below?
By design, trace-agent.c should handle all timestamp sync logic, not
only vsock relatated one.
Currently we rely only on vsock events, but we might have timestamp
sync which does not
depend on vsocks in the future.
>
> >
> > ifeq ($(VSOCK_DEFINED), 1)
> > TRACE_CMD_OBJS += trace-agent.o
> > +//#define TSYNC_RBUFFER_DEBUG
> > +static int find_raw_events(struct tep_handle *tep,
> > + struct buffer_instance *instance,
> > + struct tracecmd_time_sync_event *events)
> > +{
> > + struct tracecmd_time_sync_event *events_array = NULL;
> > + int events_count = 0;
> > + int events_size = 0;
> > + unsigned int p_size;
> > + int ts = 0;
> > + void *page;
> > + char *path;
> > + char *file;
> > + DIR *dir;
>
> `dir` is unused besides getting opened and closed below, remove it?
> Or is it supposed to check for the existence of such folder? If so,
> maybe a stat() will suffice or rely on that opening files inside this
> directory will fail too.
>
I'll remove it, I think there is no need to check if it exists.
> > + path = get_instance_file(instance, "per_cpu");
> > + if (!path)
> > + return ts;
> > +
> > + dir = opendir(path);
> > + if (!dir)
> > + goto out;
> > +
> > + len = strlen(path);
> > + file = malloc(len + strlen("trace_pipe_raw") + 32);
>
> How about having file defined as
>
> char file[PATH_MAX];
>
> and avoid allocation and computing how much space we need for it?
Thanks, I'll add it in the next version of the patch.
>
> > +static void get_vsocket_params(int fd, unsigned int *lcid, unsigned int *lport,
> > + unsigned int *rcid, unsigned int *rport)
>
> This static function is called only once in this file and never with
> NULL output arguments. I think we can safely drop all NULL pointer
> checks in it and simplify it.
>
That's true, the reason for those checks is that in one of the
previous versions of
the patch it was called with NULL pointers. I'll remove the checks.
> > +tracecmd_clock_context_new(struct tracecmd_msg_handle *msg_handle,
> > + const char *clock_str,
> > + enum clock_sync_context id)
> > +{
> > + struct tracecmd_clock_sync *clock_context;
> > + struct tep_event *event;
> > + unsigned int i = 0;
> > +
> > + switch (id) {
> > +#ifdef VSOCK
> > + case CLOCK_KVM_X86_VSOCK_HOST:
> > + case CLOCK_KVM_X86_VSOCK_GUEST:
> > + break;
> > +#endif
> > + default: /* not supported clock sync context */
> > + return NULL;
> > + }
> > +
> > + if (id >= CLOCK_CONTEXT_MAX || NULL == msg_handle)
> > + return NULL;
> > + clock_context = calloc(1, sizeof(struct tracecmd_clock_sync));
> > + if (!clock_context)
> > + return NULL;
> > + clock_context->clock_context_id = id;
> > + clock_context_copy(clock_context,
> > + clock_sync[id].ftrace_params, clock_sync[id].events);
> > +
> > + get_vsocket_params(msg_handle->fd,
> > + &(clock_context->local_cid),
> > + &(clock_context->local_port),
> > + &(clock_context->remote_cid),
> > + &(clock_context->remote_port));
>
> Nit: parenthesis are not needed:
>
> get_vsocket_params(msg_handle->fd,
> &clock_context->local_cid,
> &clock_context->local_port,
> &clock_context->remote_cid,
> &clock_context->remote_port);
>
> > +
Thanks Slavi
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
|