Re: [PATCH 1/2] ring-buffer: Replace rb_time_cmpxchg() with rb_time_cmp_and_update()

[Steven Rostedt]

On Mon, 18 Dec 2023 23:24:55 +0900
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> wrote:

> On Fri, 15 Dec 2023 11:55:13 -0500
> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> 
> > From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
> > 
> > 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
> >  }