[RFC PATCH] tracing/ring-buffer: Drop inappropriate WARN in rb_set_head_page()

[RFC PATCH] tracing/ring-buffer: Drop inappropriate WARN in rb_set_head_page()

[Zheng Yejian]

Following WARNING appears several times during test on v5.10 but
mainline kernel should have the same problem. However I currently
can't find the reproduction method.

WARNING: CPU: 29 PID: 686834 at kernel/trace/ring_buffer.c:1357
           rb_set_head_page+0x168/0x264
Link: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/kernel/trace/ring_buffer.c?h=linux-5.10.y#n1357
Call trace:
 rb_set_head_page+0x168/0x264
 rb_per_cpu_empty+0x34/0x15c
 ring_buffer_empty_cpu.part.0.isra.0+0x1a4/0x3f0
 ring_buffer_empty_cpu+0x74/0xb4
 __find_next_entry+0x14c/0x2f4
 trace_find_next_entry_inc+0x48/0x13c
 tracing_read_pipe+0x2c8/0x6b4
 vfs_read+0x144/0x324
 ksys_read+0x104/0x220
 __arm64_sys_read+0x54/0x70
 el0_svc_common.constprop.0+0xd8/0x37c
 do_el0_svc+0x50/0x120
 el0_svc+0x24/0x3c
 el0_sync_handler+0x17c/0x180
 el0_sync+0x160/0x180

The WARNING appears because rb_set_head_page() didn't grab the header
after three loops traversing buffer pages. This was not considered
to be expected, as comment said, writer possibly moves the header in
one loop.

However, supposing writer keeps moving the header, we may miss more
loops and it seems normal not to grab the header within three loops
in rb_set_head_page(). Therefore drop that RB_WARN_ON().

Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
---
 kernel/trace/ring_buffer.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index af50d931b020..cbfa306570d3 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -1471,9 +1471,7 @@ rb_set_head_page(struct ring_buffer_per_cpu *cpu_buffer)
 	page = head = cpu_buffer->head_page;
 	/*
 	 * It is possible that the writer moves the header behind
-	 * where we started, and we miss in one loop.
-	 * A second loop should grab the header, but we'll do
-	 * three loops just because I'm paranoid.
+	 * where we started, so we try three loops to grab the header.
 	 */
 	for (i = 0; i < 3; i++) {
 		do {
@@ -1485,8 +1483,6 @@ rb_set_head_page(struct ring_buffer_per_cpu *cpu_buffer)
 		} while (page != head);
 	}
 
-	RB_WARN_ON(cpu_buffer, 1);
-
 	return NULL;
 }
 
-- 
2.25.1

Re: [RFC PATCH] tracing/ring-buffer: Drop inappropriate WARN in rb_set_head_page()

[Steven Rostedt]

On Wed, 1 Mar 2023 11:47:02 +0800
Zheng Yejian <zhengyejian1@huawei.com> wrote:

> Following WARNING appears several times during test on v5.10 but
> mainline kernel should have the same problem. However I currently
> can't find the reproduction method.
> 
> WARNING: CPU: 29 PID: 686834 at kernel/trace/ring_buffer.c:1357
>            rb_set_head_page+0x168/0x264
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/kernel/trace/ring_buffer.c?h=linux-5.10.y#n1357
> Call trace:
>  rb_set_head_page+0x168/0x264
>  rb_per_cpu_empty+0x34/0x15c
>  ring_buffer_empty_cpu.part.0.isra.0+0x1a4/0x3f0
>  ring_buffer_empty_cpu+0x74/0xb4
>  __find_next_entry+0x14c/0x2f4
>  trace_find_next_entry_inc+0x48/0x13c
>  tracing_read_pipe+0x2c8/0x6b4
>  vfs_read+0x144/0x324
>  ksys_read+0x104/0x220
>  __arm64_sys_read+0x54/0x70
>  el0_svc_common.constprop.0+0xd8/0x37c
>  do_el0_svc+0x50/0x120
>  el0_svc+0x24/0x3c
>  el0_sync_handler+0x17c/0x180
>  el0_sync+0x160/0x180
> 
> The WARNING appears because rb_set_head_page() didn't grab the header
> after three loops traversing buffer pages. This was not considered
> to be expected, as comment said, writer possibly moves the header in
> one loop.
> 
> However, supposing writer keeps moving the header, we may miss more
> loops and it seems normal not to grab the header within three loops
> in rb_set_head_page(). Therefore drop that RB_WARN_ON().

It's normal to grab the header in two loops. I only made it three in case I
was wrong. If it took 4 tries, something is wrong. Just returning NULL
without setting the header will cause bugs elsewhere.

-- Steve


> 
> Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
> ---
>  kernel/trace/ring_buffer.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index af50d931b020..cbfa306570d3 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -1471,9 +1471,7 @@ rb_set_head_page(struct ring_buffer_per_cpu *cpu_buffer)
>  	page = head = cpu_buffer->head_page;
>  	/*
>  	 * It is possible that the writer moves the header behind
> -	 * where we started, and we miss in one loop.
> -	 * A second loop should grab the header, but we'll do
> -	 * three loops just because I'm paranoid.
> +	 * where we started, so we try three loops to grab the header.
>  	 */
>  	for (i = 0; i < 3; i++) {
>  		do {
> @@ -1485,8 +1483,6 @@ rb_set_head_page(struct ring_buffer_per_cpu *cpu_buffer)
>  		} while (page != head);
>  	}
>  
> -	RB_WARN_ON(cpu_buffer, 1);
> -
>  	return NULL;
>  }
>