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

[Uros Bizjak]

On Wed, Mar 1, 2023 at 5:28 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Wed, 1 Mar 2023 11:18:50 -0500
> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> > But with gcc 12.2.0 I don't really see the benefit. And I'm worried that
> > the side effect of modifying the old variable could cause a bug in the
> > future, if it is used after the try_cmpxchg(). At least for the second case.
>
> Actually, I like Joel's recommendation of adding a cmpxchg_succeeded()
> function, that does the try_cmpxchg() without needing to save the old
> variable. That's my main concern, as it does have that side effect that
> could be missed when updating the code.

The "controversial" part of the patch would then look like the
attached patch. As expected, the compiler again produces expected
code:

     eb8:    48 8b 0e                 mov    (%rsi),%rcx
     ebb:    48 83 e1 fc              and    $0xfffffffffffffffc,%rcx
     ebf:    48 83 c9 01              or     $0x1,%rcx
     ec3:    48 89 c8                 mov    %rcx,%rax
     ec6:    f0 48 0f b1 3e           lock cmpxchg %rdi,(%rsi)
     ecb:    48 39 c1                 cmp    %rax,%rcx
     ece:    74 2d                    je     efd <rb_get_reader_page+0x12d>

to:

     eb8:    48 8b 01                 mov    (%rcx),%rax
     ebb:    48 83 e0 fc              and    $0xfffffffffffffffc,%rax
     ebf:    48 83 c8 01              or     $0x1,%rax
     ec3:    f0 48 0f b1 31           lock cmpxchg %rsi,(%rcx)
     ec8:    74 2d                    je     ef7 <rb_get_reader_page+0x127>

Uros.
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index af50d931b020..7ad855f54371 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -163,6 +163,12 @@ enum {
 #define extended_time(event) \
 	(event->type_len >= RINGBUF_TYPE_TIME_EXTEND)
 
+#define cmpxchg_success(ptr, old, new)				\
+({								\
+	typeof(*(ptr)) __tmp = (old);				\
+	try_cmpxchg((ptr), &__tmp, (new));			\
+})
+
 static inline int rb_null_event(struct ring_buffer_event *event)
 {
 	return event->type_len == RINGBUF_TYPE_PADDING && !event->time_delta;
@@ -1495,14 +1501,11 @@ static int 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 cmpxchg_success(ptr, val, (unsigned long)&new->list);
 }
 
 /*
@@ -2061,7 +2064,7 @@ rb_insert_pages(struct ring_buffer_per_cpu *cpu_buffer)
 	retries = 10;
 	success = 0;
 	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;
 
@@ -2079,9 +2082,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 (cmpxchg_success(&prev_page->next,
+				    head_page_with_bit, first_page)) {
 			/*
 			 * yay, we replaced the page pointer to our new list,
 			 * now, we just have to update to head page's prev