From: Steven Rostedt <rostedt@goodmis.org>
To: Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
Cc: Linux Trace Devel <linux-trace-devel@vger.kernel.org>
Subject: Re: [PATCH v2 2/3] trace-cmd library: Add check before applying tsc2nsec offset
Date: Mon, 19 Apr 2021 11:14:07 -0400 [thread overview]
Message-ID: <20210419111407.29a90f86@gandalf.local.home> (raw)
In-Reply-To: <20210419094543.15c131c2@gandalf.local.home>
On Mon, 19 Apr 2021 09:45:43 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> That is:
>
> max = 0;
> for each stream:
> ret = read first event, compared to offset
> if (!ret) {
> /* ret will be how much off by offset */
> if (ret > max)
> max = ret;
> }
> if (max) {
> for each stream:
> Update offset by subtracting max
> }
>
> Look at each stream, and have some callback give you how much ahead the
> first event is from its given offset. Take the biggest value from reading
> all the streams, then tell all the streams to subtract its offset by that
> max value. The end result is that all streams now start at a positive value.
From our call, here's the pseudo code that I was talking about:
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index b17b36e0..f03dadd3 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -1302,7 +1302,7 @@ static unsigned long long timestamp_host_sync(unsigned long long ts, int cpu,
> &tsync->ts_samples[mid+1]);
> }
>
> -static unsigned long long timestamp_calc(unsigned long long ts, int cpu,
> +static unsigned long long pre_timestamp_calc(unsigned long long ts, int cpu,
> struct tracecmd_input *handle)
I pulled out the timestamp_calc into a helper function.
> {
> /* do not modify raw timestamps */
> @@ -1318,17 +1318,44 @@ static unsigned long long timestamp_calc(unsigned long long ts, int cpu,
> ts *= handle->ts2secs;
> } else if (handle->tsc_calc.mult) {
> /* auto calculated TSC clock frequency */
> - ts -= handle->tsc_calc.offset;
And removed the calc offset.
> ts = mul_u64_u32_shr(ts, handle->tsc_calc.mult, handle->tsc_calc.shift);
> }
>
> /* User specified time offset with --ts-offset or --date options */
> - if (handle->ts_offset)
> - ts += handle->ts_offset;
> + ts += handle->ts_offset;
As we mentioned (and this can be a separate patch), the if statement is
useless.
>
> return ts;
> }
>
> +static unsigned long long timestamp_calc(unsigned long long ts, int cpu,
> + struct tracecmd_input *handle)
> +{
> + static int once;
> +
> + ts = pre_timestamp_calc(ts, cpu, handle);
> + if (!once && ts > handle->start_ts_offset) {
> + once++;
> + tracecmd_warning();
> + }
> + ts -= handle->start_ts_offset;
After looking at this more, I think we should just have the ts_offset and
start_ts_offset be the same. And remove the ts += handle->ts_offset, from
the pre_timestamp_calc() above, and have this check test just ts_offset.
So now the timestamp_calc() will get the timestamp and then apply the
ts_offset separately (and warn if the offset is greater than the ts).
> +}
> +
> +
> +
> +long long tracecmd_cpu_first_ts_offset(struct tracecmd_input *handle, int cpu)
> +}
> + struct tep_record rec;
> +
> + rec = first_event(handle, cpu);
> + return pre_timestamp_calc(rec->ts, cpu, handle) - handle->ts_offset;
> +}
Add an API that shows the difference between the first stream event
timestamp against the user supplied (or file supplied) ts_offset.
> +
> +int tracecmd_modify_ts_offset(struct tracecmd_input *handle, long long offset)
> +{
> + handle->ts_offset += offset;
> +}
Allow the user to tweak that offset. As we already have:
tracecmd_set_ts_offset(handle, offset) to set ts_offset, if the user found
that the offset was before, it could tweak it.
> +
> +
> /*
> * Page is mapped, now read in the page header info.
> */
>
That is, in the options, we would need to have the calc offset from the
file (doing the sync), do:
handle->ts_offset -= start_offset.
Or something like that. I'll let you look at this code and see what you
come up with, and we can discuss this further.
-- Steve
next prev parent reply other threads:[~2021-04-19 15:14 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-16 10:34 [PATCH v2 0/3] Fix overflow when applying tsc2nsec calculations Tzvetomir Stoyanov (VMware)
2021-04-16 10:34 ` [PATCH v2 1/3] trace-cmd library: Add new trace-cmd library APIs for guest ts corrections Tzvetomir Stoyanov (VMware)
2021-04-16 10:34 ` [PATCH v2 2/3] trace-cmd library: Add check before applying tsc2nsec offset Tzvetomir Stoyanov (VMware)
2021-04-16 20:12 ` Steven Rostedt
2021-04-19 8:08 ` Tzvetomir Stoyanov
2021-04-19 13:45 ` Steven Rostedt
2021-04-19 15:14 ` Steven Rostedt [this message]
2021-04-28 12:31 ` Tzvetomir Stoyanov
2021-04-16 10:34 ` [PATCH v2 3/3] trace-cmd: Get the timestamp of the first recorded event as TSC offset Tzvetomir Stoyanov (VMware)
2021-04-16 19:28 ` Steven Rostedt
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=20210419111407.29a90f86@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).