Re: [PATCH 3/3] ring_buffer: Use try_cmpxchg instead of cmpxchg

[Joel Fernandes]

On Wed, Mar 1, 2023 at 4:37 AM Uros Bizjak <ubizjak@xxxxxxxxx> wrote:
>
> On Tue, Feb 28, 2023 at 10:43 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> >
> > On Tue, 28 Feb 2023 18:59:29 +0100
> > Uros Bizjak <ubizjak@xxxxxxxxx> wrote:
> >
> > > Use try_cmpxchg instead of cmpxchg (*ptr, old, new) == old.
> > > x86 CMPXCHG instruction returns success in ZF flag, so this change
> > > saves a compare after cmpxchg (and related move instruction in
> > > front of cmpxchg).
> > >
> > > Also, try_cmpxchg implicitly assigns old *ptr value to "old" when cmpxchg
> > > fails. There is no need to re-read the value in the loop.
> > >
> > > No functional change intended.
> >
> > As I mentioned in the RCU thread, I have issues with some of the changes
> > here.
> >
> > >
> > > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > > Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > > Signed-off-by: Uros Bizjak <ubizjak@xxxxxxxxx>
> > > ---
> > >  kernel/trace/ring_buffer.c | 20 ++++++++------------
> > >  1 file changed, 8 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > > index 4188af7d4cfe..8f0ef7d12ddd 100644
> > > --- a/kernel/trace/ring_buffer.c
> > > +++ b/kernel/trace/ring_buffer.c
> > > @@ -1493,14 +1493,11 @@ static bool rb_head_page_replace(struct buffer_page *old,
> > >  {
> > >       unsigned long *ptr = (unsigned long *)&old->list.prev->next;
> > >       unsigned long val;
> > > -     unsigned long ret;
> > >
> > >       val = *ptr & ~RB_FLAG_MASK;
> > >       val |= RB_PAGE_HEAD;
> > >
> > > -     ret = cmpxchg(ptr, val, (unsigned long)&new->list);
> > > -
> > > -     return ret == val;
> > > +     return try_cmpxchg(ptr, &val, (unsigned long)&new->list);
> >
> > No, val should not be updated.
>
> Please see the definition of try_cmpxchg. The definition is written in
> such a way that benefits loops as well as linear code and in the later
> case depends on the compiler to eliminate assignment to val as a dead
> assignment.
>
> The above change was done under the assumption that val is unused
> after try_cmpxchg, and can be considered as a temporary
> [Alternatively, the value could be copied to a local temporary and a
> pointer to this local temporary could be passed to try_cmpxchg
> instead. Compiler is smart enough to eliminate the assignment in any
> case.]

If I understood Steve correctly, I think the "misleading" part is that
you are passing a variable by address to try_cmpxchg() but not really
modifying it (unlike in the loop patterns).

Perhaps it is more meaningful to have an API that looks like:
bool cmpxchg_succeeded(TYPE ptr, TYPE old, TYPE new)
Where old is not a pointer (unlike try_cmpxchg), and the API returns bool.

Both cleaner to read and has the optimization you want, I believe.

However, the other point is, this is useful only for slow paths, but
at least cmpxchg_succeeded() is more readable and less "misleading"
than try_cmpxchg() IMO.

 - Joel

[...]