Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH v17 16/18] trace-cmd: Basic infrastructure for host - guest timestamp synchronization
Date: Thu, 12 Dec 2019 09:54:11 -0500
Message-ID: <20191212095411.05cc1318@gandalf.local.home> (raw)
In-Reply-To: <CAPpZLN5A9Am1v+yrpXwyoGQkeqvSza5QxRon61oSbyMZ5K4ghg@mail.gmail.com>

On Thu, 12 Dec 2019 14:34:39 +0200
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

>  Thanks, Steven!
> I will prepare the next version addressing the comments.
> 
> On Tue, Dec 10, 2019 at 8:39 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >  
> [ ... ]
> > >
> > > +static int tracecmd_msg_send_nofree(int fd, struct tracecmd_msg *msg)  
> >
> > We probably shouldn't have a static function that starts with
> > "tracecmd_".  
> 
> I changed this one, but noticed few other static "tracecmd_" functions
> in the legacy code, will rename them in a separate patch.

Yeah, those should probably be changed too.

> >  
> [ ... ]
> > > +
> > > +/**
> > > + * tracecmd_tsync_proto_select - Select time sync protocol, to be used for
> > > + *           timestamp synchronization with a peer.
> > > + *
> > > + * @protos: Bitmask of time sync protocols, supported by the peer.  
> >
> > Hmm, so we are limiting the number of protocols that we will ever
> > support to 32? This makes me nervous (I always think of the famous
> > quote of "Who will ever need more than 64K of RAM?")
> >
> > To be safe, we should probably make this an array:
> >
> >         int tracecmd_tsync_proto_select(unsigned int *protos, int words)
> >
> > The proto_id should be a bit, not a mask, that is:
> >
> >         int word;
> >
> >         for (word = 0; word < words; word++) {
> >
> >                 for (proto = tsync_proto_list; proto; proto = proto->next) {
> >                         if (proto->proto_id < word * 32)
> >                                 continue;
> >
> >                         id = proto->proto_id - word * 32;
> >                         if (id >= 32)
> >                                 continue;
> >
> >                         if ((1 << id) & protos[word]) {
> >                                 if (selected)
> >                                         [...]
> >
> > While we only have a single word, callers would just do:
> >
> >         unsigned int protos;
> >
> >         ret = tracecmd_tsync_proto_select(&protos, 1);
> >
> > At least this will never limit us to just 32 protocols, even though we
> > will likely never need more. But we are safe if we ever do ;-)
> >  
> 
> I changed this and tracecmd_tsync_proto_getall() to work with
> unlimited bitmasks,
> but in order to support more than 32 protocols the trace request  packet must be
> changed. In the current patch I added field
>                                  be32 tsync_protos;
> in struct tracecmd_msg_trace_req, which is bitmask for all time sync
> protocols, supported
> by the host. Defining this as a fixed size bitmap simplifies a lot the
> protocol. We can add a
> dynamic bitmask as part of the trace request  packet payload, but that
> payload is already used
> for the trace arguments. We can extend the payload to hold both
> dynamic bitmask and trace arguments,
> but I think it will complicate a lot the packet handling and the
> benefit will be only the posibility to support
> more than 32 time sync protocols.

Perhaps instead, we can add an option to which 32bit word the protocols
are used. That is, we pass an index of what the tsync_protos are, and
the response can pick one of them, and even say how many more words it
has.

That is, I don't think we should change the protocol to pass the entire
bitmask. Just part of it, and if we find a match, we use that. That
should keep it from being too complicated, right?

-- Steve


> 
> > > + *  
> [ ... ]
> 


  reply index

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

Reply instructions:

You may reply publically 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=20191212095411.05cc1318@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=linux-trace-devel@vger.kernel.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