Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Tzvetomir Stoyanov <tstoyanov@vmware.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>,
	Linux Trace Devel <linux-trace-devel@vger.kernel.org>
Subject: Re: [PATCH v18 16/18] trace-cmd: Basic infrastructure for host - guest timestamp synchronization
Date: Fri, 31 Jan 2020 09:53:40 +0000
Message-ID: <CACqStoe7eoDN4bCBNf7oQBUJC10Rk8QB8S9EVmbkUss7Fntz7A@mail.gmail.com> (raw)
In-Reply-To: <20191220234829.55000e54@rorschach.local.home>

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

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-13 15:30 [PATCH v18 00/18]Timestamp synchronization of host - guest tracing session Tzvetomir Stoyanov (VMware)
2019-12-13 15:30 ` [PATCH v18 01/18] trace-cmd: Implement new lib API: tracecmd_local_events_system() Tzvetomir Stoyanov (VMware)
2019-12-13 15:30 ` [PATCH v18 02/18] trace-cmd: Add support for negative time offsets in trace.dat file Tzvetomir Stoyanov (VMware)
2019-12-13 15:30 ` [PATCH v18 03/18] trace-cmd: Add implementations of htonll() and ntohll() Tzvetomir Stoyanov (VMware)
2019-12-21  2:34   ` Steven Rostedt
2019-12-13 15:30 ` [PATCH v18 04/18] trace-cmd: Add new library APIs for ftrace instances Tzvetomir Stoyanov (VMware)
2019-12-21  3:03   ` Steven Rostedt
2020-01-06 14:47     ` Tzvetomir Stoyanov
2019-12-13 15:30 ` [PATCH v18 05/18] trace-cmd: Add new library API for local CPU count Tzvetomir Stoyanov (VMware)
2019-12-13 15:30 ` [PATCH v18 06/18] trace-cmd: Add new library API for reading ftrace buffers Tzvetomir Stoyanov (VMware)
2019-12-13 15:30 ` [PATCH v18 07/18] trace-cmd: Find and store pids of tasks, which run virtual CPUs of given VM Tzvetomir Stoyanov (VMware)
2019-12-13 15:30 ` [PATCH v18 08/18] trace-cmd: Implement new API tracecmd_add_option_v() Tzvetomir Stoyanov (VMware)
2019-12-13 15:30 ` [PATCH v18 09/18] trace-cmd: Add new API to generate a unique ID of the tracing session Tzvetomir Stoyanov (VMware)
2019-12-13 15:30 ` [PATCH v18 10/18] trace-cmd: Store the session tracing ID in the trace.dat file Tzvetomir Stoyanov (VMware)
2019-12-20 20:04   ` Steven Rostedt
2020-01-06 14:33     ` Tzvetomir Stoyanov
2019-12-13 15:30 ` [PATCH v18 11/18] trace-cmd: Exchange tracing IDs between host and guest Tzvetomir Stoyanov (VMware)
2019-12-13 15:30 ` [PATCH v18 12/18] trace-cmd: Implement new option in trace.dat file: TRACECMD_OPTION_TIME_SHIFT Tzvetomir Stoyanov (VMware)
2019-12-21  3:19   ` Steven Rostedt
2019-12-13 15:30 ` [PATCH v18 13/18] trace-cmd: Add guest information in host's trace.dat file Tzvetomir Stoyanov (VMware)
2019-12-20 20:52   ` Steven Rostedt
2020-01-06 14:43     ` Tzvetomir Stoyanov
2020-01-06 14:55       ` Steven Rostedt
2019-12-13 15:30 ` [PATCH v18 14/18] trace-cmd: Add host trace clock as guest trace argument Tzvetomir Stoyanov (VMware)
2019-12-13 15:30 ` [PATCH v18 15/18] trace-cmd: Refactor few trace-cmd internal functions Tzvetomir Stoyanov (VMware)
2019-12-13 15:30 ` [PATCH v18 16/18] trace-cmd: Basic infrastructure for host - guest timestamp synchronization Tzvetomir Stoyanov (VMware)
2019-12-21  4:48   ` Steven Rostedt
2020-01-31  9:53     ` Tzvetomir Stoyanov [this message]
2020-01-31 14:49       ` Steven Rostedt
2019-12-13 15:30 ` [PATCH v18 17/18] trace-cmd: [POC] PTP-like algorithm " Tzvetomir Stoyanov (VMware)
2019-12-13 15:30 ` [PATCH v18 18/18] trace-cmd: Debug scripts for " Tzvetomir Stoyanov (VMware)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACqStoe7eoDN4bCBNf7oQBUJC10Rk8QB8S9EVmbkUss7Fntz7A@mail.gmail.com \
    --to=tstoyanov@vmware.com \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=tz.stoyanov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org
	public-inbox-index linux-trace-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git