Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 18 Dec 2023 10:15:31 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH 1/2] ring-buffer: Replace rb_time_cmpxchg() with rb_time_cmp_and_update() |
| |
On Mon, 18 Dec 2023 23:24:55 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> On Fri, 15 Dec 2023 11:55:13 -0500 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > From: "Steven Rostedt (Google)" <rostedt@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 > > }
| ![\](/images/icornerr.gif) |