Re: [RFC][PATCH] ring-buffer: Have nested events still record running time stamp

[Korben Rusek]

On Thu, Jun 25, 2020 at 7:38 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Thu, 25 Jun 2020 09:53:15 -0400 (EDT)
> Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> wrote:
>
> > ----- On Jun 25, 2020, at 9:44 AM, rostedt rostedt@xxxxxxxxxxx wrote:
> >
> > > From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
> > >
> > > [ SEVEN YEAR PROBLEM SOLVED! ]
> > >
> > > Up until now, if an event is interrupted while it is recorded by an
> > > interrupt, and that interrupt records events, the time of those events will
> > > all be the same. This is because events only record the delta of the time
> > > since the previous event (or beginning of a page), and to handle updating
> > > the time keeping for that of nested events is extremely racy. After years of
> > > thinking about this and several failed attempts, I finally have a solution
> > > to solve this puzzle.
> >
> > Out of curiosity, considering that LTTng has solved this problem 10+ years ago
> > with a simpler concurrency-friendly time-stamping model, why not simply use it
> > rather than add complexity to the current ftrace timestamp scheme ?
>
> Because it requires updating all the tools that read this from user
> space.
>
> I found a solution that works, so why change it and cause the backward
> compatibility pain now?
>
> -- Steve

Great work! I'm not exactly qualified to review the code, but the
logic seems correct. I'm curious how unlikely a zero delta is now and
how you quantify it. Also does it negate the patch that I emailed out
last week that adds a `force_abs_timestamp` trace/option in an attempt
to get around this particular issue?

In reading through, I did notice a couple simple typos in the comments
that are probably worth pointing out:

> If preempting an event time update, we may need absolute timestamp.

Not a big deal, but it should be "may need *an* absolute timestamp"

> * Preempted beween C and E:
> * Lost the previous events time stamp. Just set the
> * delta to zero, and this will be the same time as
> * the veent this event preempted. And the events that
> * came after this will still be correct (as they would
> * have built their delta on the previous event.

Should be "the *event* this event preempted." It also needs a
parenthesis at the end of the comment to close the parenthetical
statement.

Thanks, Korben