Messages in this thread | | | Date | Tue, 28 Feb 2023 16:43:46 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH 3/3] ring_buffer: Use try_cmpxchg instead of cmpxchg |
| |
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.
> } > > /* > @@ -2055,7 +2052,7 @@ rb_insert_pages(struct ring_buffer_per_cpu *cpu_buffer) > retries = 10; > success = false; > while (retries--) { > - struct list_head *head_page, *prev_page, *r; > + struct list_head *head_page, *prev_page; > struct list_head *last_page, *first_page; > struct list_head *head_page_with_bit; > > @@ -2073,9 +2070,8 @@ rb_insert_pages(struct ring_buffer_per_cpu *cpu_buffer) > last_page->next = head_page_with_bit; > first_page->prev = prev_page; > > - r = cmpxchg(&prev_page->next, head_page_with_bit, first_page); > - > - if (r == head_page_with_bit) { > + if (try_cmpxchg(&prev_page->next, > + &head_page_with_bit, first_page)) {
No. head_page_with_bit should not be updated.
> /* > * yay, we replaced the page pointer to our new list, > * now, we just have to update to head page's prev > @@ -4061,10 +4057,10 @@ void ring_buffer_record_off(struct trace_buffer *buffer) > unsigned int rd; > unsigned int new_rd; > > + rd = atomic_read(&buffer->record_disabled); > do { > - rd = atomic_read(&buffer->record_disabled); > new_rd = rd | RB_BUFFER_OFF; > - } while (atomic_cmpxchg(&buffer->record_disabled, rd, new_rd) != rd); > + } while (!atomic_try_cmpxchg(&buffer->record_disabled, &rd, new_rd));
This change is OK.
> } > EXPORT_SYMBOL_GPL(ring_buffer_record_off); > > @@ -4084,10 +4080,10 @@ void ring_buffer_record_on(struct trace_buffer *buffer) > unsigned int rd; > unsigned int new_rd; > > + rd = atomic_read(&buffer->record_disabled); > do { > - rd = atomic_read(&buffer->record_disabled); > new_rd = rd & ~RB_BUFFER_OFF; > - } while (atomic_cmpxchg(&buffer->record_disabled, rd, new_rd) != rd); > + } while (!atomic_try_cmpxchg(&buffer->record_disabled, &rd, new_rd));
And so is this one.
So I will not take this patch as is.
-- Steve
> } > EXPORT_SYMBOL_GPL(ring_buffer_record_on); >
| |