Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Linux Trace Devel <linux-trace-devel@vger.kernel.org>
Subject: Re: [PATCH v24 01/10] trace-cmd: [POC] PTP-like algorithm for host - guest timestamp synchronization
Date: Mon, 26 Oct 2020 09:38:03 +0200
Message-ID: <CAPpZLN6C8Q1EJLurdmTTZ44m-eA6Jk0yE8qGA9B+fa7=hbY5sg@mail.gmail.com> (raw)
In-Reply-To: <20201015172420.699a4cdc@gandalf.local.home>

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

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-09 14:03 [PATCH v24 00/10] Timestamp synchronization of host - guest tracing session Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 01/10] trace-cmd: [POC] PTP-like algorithm for host - guest timestamp synchronization Tzvetomir Stoyanov (VMware)
2020-10-15 21:24   ` Steven Rostedt
2020-10-26  7:38     ` Tzvetomir Stoyanov [this message]
2020-10-09 14:03 ` [PATCH v24 02/10] trace-cmd: Debug scripts for " Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 03/10] trace-cmd: Add trace-cmd library APIs for ftrace clock name Tzvetomir Stoyanov (VMware)
2020-10-22  1:26   ` Steven Rostedt
2020-10-22  1:31     ` Steven Rostedt
2020-10-09 14:03 ` [PATCH v24 04/10] trace-cmd: Move VM related logic in a separate file Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 05/10] trace-cmd: Add clock parameter to timestamp synchronization plugins Tzvetomir Stoyanov (VMware)
2020-10-22  1:39   ` Steven Rostedt
2020-10-09 14:03 ` [PATCH v24 06/10] trace-cmd: Add role " Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 07/10] trace-cmd: Add host / guest role in timestamp synchronization context Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 08/10] trace-cmd: Add guest CPU count PID in tracecmd_time_sync struct Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 09/10] trace-cmd: Fixed bitmask logic tracecmd_tsync_proto_getall() Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 10/10] trace-cmd [POC]: Add KVM timestamp synchronization plugin 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='CAPpZLN6C8Q1EJLurdmTTZ44m-eA6Jk0yE8qGA9B+fa7=hbY5sg@mail.gmail.com' \
    --to=tz.stoyanov@gmail.com \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    /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