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

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

 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.

>
[ ... ]
> >  static int make_trace_req(struct tracecmd_msg *msg, int argc, char **argv,
> > -                       bool use_fifos, unsigned long long trace_id)
> > +                       bool use_fifos, unsigned long long trace_id,
> > +                       unsigned int tsync_protos)
> >  {
> >       size_t args_size = 0;
> >       char *p;
> > @@ -824,7 +848,11 @@ static int make_trace_req(struct tracecmd_msg *msg, int argc, char **argv,
> >               args_size += strlen(argv[i]) + 1;
> >
> >       msg->hdr.size = htonl(ntohl(msg->hdr.size) + args_size);
> > -     msg->trace_req.flags = use_fifos ? htonl(MSG_TRACE_USE_FIFOS) : htonl(0);
> > +     msg->trace_req.flags = 0;
> > +     if (use_fifos)
> > +             msg->trace_req.flags |= MSG_TRACE_USE_FIFOS;
>
> Why the change to flags here?

Fixed, It was a leftover from a previous version of this patch, where
flags were extended with time sync specific one.
However, I removed that time sync specific flag but forgot about this change.

>
[ ... ]
> > +
> > +/**
> > + * 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.

> > + *
[ ... ]

-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center