Re: [PATCH v24 01/10] trace-cmd: [POC] PTP-like algorithm for host - guest timestamp synchronization

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

On Fri, Oct 16, 2020 at 12:24 AM Steven Rostedt <rostedt@goodmis.org> wrote:
[...]
> > +
> > +static void ptp_probe_store(struct ptp_markers_context *ctx,
> > +                         struct ptp_marker *marker,
> > +                         unsigned long long ts)
> > +{
> > +     int index = -1;
> > +
> > +     if (marker->data.packet_id == 'r' &&
> > +         marker->data.count <= ctx->size) {
> > +             index = marker->data.count - 1;
> > +     } else if (marker->data.packet_id == 's' &&
> > +               marker->data.count*2 <= ctx->size){
> > +             index = ctx->size / 2 + marker->data.count - 1;
>
> These calculations should be turned into macros, or at the very least have
> comments to why this is done.
>
> Also, data.count for both should always be less than or equal ctx->size /
> 2, right?
>
> If the ctx->size is for both, then the count should only be half. Wouldn't
> the 'r' packet start writing over the 's' packets if it is not? If this is
> the case, then we could simplify this to:
>
>         if (marker->data.count > ctx->size / 2)
>                 return;
>
>         index = marker->data_count - 1;
>
>         switch (marker->data.packet_id) {
>         case 'r':
>                 break;
>         case 's':
>                 index += ctx->size / 2;
>                 break;
>         default:
>                 return;
>         }
>
>         ctx->msg.samples[index].id = marker->data.count;
>         ctx->msg.samples[index].ts = ts;
>         ctx->msg.count++;
>
> BTW, when would the samples[index].id ever equal to something other than
> the index + 1, or index + size / 2 + 1?
>
I'll add comment to the ptp_probe_store() function, describing its
logic. It is a little
bit confusing, as it handles both cases - server and client. The
server tracks both sent
and returned probes, in that case the array size is 2 * max data count
- first are stored
returned probes, after them are sent probes. In the client context,
there are only returned
probes and the array size is max data count. The samples[index].id is
always the count
of the current probe, it could be index + 1 or index + size / 2 + 1
only, no other values.
I store it in the ID field, as in the server context there are two
entries (sent and received)
with the same probe count, and it is easier to match both based on
this ID when do the
calculations. I can use the index only, to find both sent and returned
probes and to assume
that both probes match, but I prefered to use this ID to verify if
both records are from
the same probe.

[...]
> > +
> > +     bin = (offset_max - offset_min) / PTP_SYNC_LOOP;
> > +     for (i = 0; i < k; i++) {
> > +             ind = (llabs(offsets[i]) - offset_min) / bin;
> > +             if (ind < PTP_SYNC_LOOP) {
> > +                     hist[ind]++;
> > +                     if (max < hist[ind]) {
> > +                             max = hist[ind];
> > +                             *offset_ret = offsets[i];
> > +                             *ts_ret = timestamps[i];
> > +                     }
>
> I'm curious to how accurate this is?
>
Using the histogram logic, an accuracy from 200ns up to 15 000ns can
be achieved,
results vary a lot. The accuracy of the fastest response logic is
around 1000ns - 3000ns usually.

> -- Steve

Thanks Steve!
I'll send the next version of the patch set addressing these comments.

>
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +



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