Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Tzvetomir Stoyanov <tstoyanov@vmware.com>
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:49:54 -0500
Message-ID: <20200131094954.7c553e85@gandalf.local.home> (raw)
In-Reply-To: <CACqStoe7eoDN4bCBNf7oQBUJC10Rk8QB8S9EVmbkUss7Fntz7A@mail.gmail.com>

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

  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
2020-01-31 14:49       ` Steven Rostedt [this message]
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=20200131094954.7c553e85@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=tstoyanov@vmware.com \
    --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