On Mon, 18 Dec 2023 23:24:55 +0900
Masami Hiramatsu (Google) <mhira...@kernel.org> wrote:
> On Fri, 15 Dec 2023 11:55:13 -0500
> Steven Rostedt <rost...@goodmis.org> wrote:
>
> > From: "Steven Rostedt (Google)" <rost...@goodmis.org>
> >
> > There's only one place that performs a 64-bit cmpxchg for the timestamp
> > processing. The cmpxchg is only to set the write_stamp equal to the
> > before_stamp, and if it doesn't get set, then the next event will simply
> > be forced to add an absolute timestamp.
> >
> > Given that 64-bit cmpxchg is expensive on 32-bit, and the current
> > workaround uses 3 consecutive 32-bit cmpxchg doesn't make it any faster.
> > It's best to just not do the cmpxchg as a simple compare works for the
> > accuracy of the timestamp. The only thing that will happen without the
> > cmpxchg is the prepended absolute timestamp on the next event which is not
> > that big of a deal as the path where this happens is seldom hit because it
> > requires an interrupt to happen between a few lines of code that also
> > writes an event into the same buffer.
> >
> > With this change, the 32-bit rb_time_t workaround can be removed.
> >
>
> Hmm, but this patch itself is just moving rb_time_cmpxchg() in the new
> rb_time_cmp_and_update() function. The actual change has been done
> in the next patch.
Exactly. Which is why I said above "with this change, the 32-bit rb_time_t
workaround can be removed". It can't be removed without this change.
> I think there is no reason to split this from the
> second one...
I originally had it as one patch, but I disliked the removal of the
workaround touching the main logic code (which this patch does).
Basically I broke it into:
1. Remove workaround exposure from the main logic. (this patch)
2. Remove the workaround. (next patch).
>
> Isn't this part actual change?
This part is abstracted out from the main logic. Which is why I made this
patch.
>
> > static bool rb_time_cmp_and_update(rb_time_t *t, u64 expect, u64 set)
> > {
> > - return rb_time_cmpxchg(t, expect, set);
> > +#ifdef RB_TIME_32
> > + return expect == READ_ONCE(t->time);
And I need to make a v2 as the above is wrong. It should have been:
return expect == local64_read(&t->time);
-- Steve
> > +#else
> > + return local64_try_cmpxchg(&t->time, &expect, set);
> > +#endif
> > }
