Messages in this thread | | | From | Joel Fernandes <> | Date | Wed, 1 Mar 2023 10:50:35 -0500 | Subject | Re: [PATCH 3/3] ring_buffer: Use try_cmpxchg instead of cmpxchg |
| |
On Wed, Mar 1, 2023 at 10:49 AM Joel Fernandes <joel@joelfernandes.org> wrote: > > On Wed, Mar 1, 2023 at 4:37 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > On Tue, Feb 28, 2023 at 10:43 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > On Tue, 28 Feb 2023 18:59:29 +0100 > > > Uros Bizjak <ubizjak@gmail.com> 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@goodmis.org> > > > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com> > > > > --- > > > > 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.]
Ah I need to be more careful how I type.
> 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).
It does modify it, but I meant it does not use it.
> 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
Useful only for fast paths...
> at least cmpxchg_succeeded() is more readable and less "misleading" > than try_cmpxchg() IMO. >
Proofreading emails properly from here on! Not after the fact!
- Joel
| |