Hi,

Here is the 19th version of improvement patches for making persistent
ring buffers robust to failures.
The previous version is here:

https://lore.kernel.org/all/177701351903.2223789.17087009302463188638.stgit@mhiramat.tok.corp.google.com/

This version rebased on v7.1-rc1 and cleanup code.

[2/7] Limit max number of loops to nr_pages in __rb_get_reader_page()
      and update comment.
[3/7] Cleanup rb_validate_buffer() so that it is more readable,
      and add a comment about the timestamp validation.
[6/7] Add a kerneldoc about rb_validate_buffer().

Thank you,

Masami Hiramatsu (Google) (7):
      ring-buffer: Flush and stop persistent ring buffer on panic
      ring-buffer: Skip invalid sub-buffers when validating persistent ring buffer
      ring-buffer: Skip invalid sub-buffers when rewinding persistent ring buffer
      ring-buffer: Add persistent ring buffer invalid-page inject test
      ring-buffer: Show commit numbers in buffer_meta file
      ring-buffer: Cleanup persistent ring buffer validation
      ring-buffer: Cleanup buffer_data_page related code


 arch/alpha/include/asm/Kbuild        |    1 
 arch/arc/include/asm/Kbuild          |    1 
 arch/arm/include/asm/Kbuild          |    1 
 arch/arm64/include/asm/ring_buffer.h |   10 +
 arch/csky/include/asm/Kbuild         |    1 
 arch/hexagon/include/asm/Kbuild      |    1 
 arch/loongarch/include/asm/Kbuild    |    1 
 arch/m68k/include/asm/Kbuild         |    1 
 arch/microblaze/include/asm/Kbuild   |    1 
 arch/mips/include/asm/Kbuild         |    1 
 arch/nios2/include/asm/Kbuild        |    1 
 arch/openrisc/include/asm/Kbuild     |    1 
 arch/parisc/include/asm/Kbuild       |    1 
 arch/powerpc/include/asm/Kbuild      |    1 
 arch/riscv/include/asm/Kbuild        |    1 
 arch/s390/include/asm/Kbuild         |    1 
 arch/sh/include/asm/Kbuild           |    1 
 arch/sparc/include/asm/Kbuild        |    1 
 arch/um/include/asm/Kbuild           |    1 
 arch/x86/include/asm/Kbuild          |    1 
 arch/xtensa/include/asm/Kbuild       |    1 
 include/asm-generic/ring_buffer.h    |   13 +
 include/linux/ring_buffer.h          |    1 
 kernel/trace/Kconfig                 |   34 ++
 kernel/trace/ring_buffer.c           |  490 +++++++++++++++++++++++-----------
 kernel/trace/trace.c                 |    4 
 26 files changed, 415 insertions(+), 157 deletions(-)
 create mode 100644 arch/arm64/include/asm/ring_buffer.h
 create mode 100644 include/asm-generic/ring_buffer.h


base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Hi Masami,

I applied your patches and enabled your ptracingtest code. I noticed
that when there's dropped pages, the trace output is not in order:

 # trace-cmd start -B ptracingtest -e all -v -e '*lock*'
 # taskset -c 5 echo c > /proc/sysrq-trigger

On reboot, I ran:

 # trace-cmd show -B ptracingtest > /tmp/trace.out

Then executed the attached perl program:

  # ./read-ts.pl < /tmp/trace.out

And it errors our:

 30.212495 < 30.213534
           <...>-1048    [005] d....    30.212495: irq_enable: caller=irqentry_exit+0xf5/0x710 parent=0x0

That is, I think the zero timestamps may be messing with the order.

-- Steve

On Sat, 2 May 2026 15:23:04 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Hi Masami,
> 
> I applied your patches and enabled your ptracingtest code. I noticed
> that when there's dropped pages, the trace output is not in order:
> 
>  # trace-cmd start -B ptracingtest -e all -v -e '*lock*'
>  # taskset -c 5 echo c > /proc/sysrq-trigger
> 
> On reboot, I ran:
> 
>  # trace-cmd show -B ptracingtest > /tmp/trace.out
> 
> Then executed the attached perl program:
> 
>   # ./read-ts.pl < /tmp/trace.out
> 
> And it errors our:
> 
>  30.212495 < 30.213534
>            <...>-1048    [005] d....    30.212495: irq_enable: caller=irqentry_exit+0xf5/0x710 parent=0x0
> 
> That is, I think the zero timestamps may be messing with the order.
> 

Ah, I think I found the problem. The iterator needs the same logic you
added for the consuming read:

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 7bfbed0ac90c..90a7fa772fe3 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6105,12 +6105,14 @@ rb_iter_peek(struct ring_buffer_iter *iter, u64 *ts)
 	struct ring_buffer_per_cpu *cpu_buffer;
 	struct ring_buffer_event *event;
 	int nr_loops = 0;
+	int max_loops;
 
 	if (ts)
 		*ts = 0;
 
 	cpu_buffer = iter->cpu_buffer;
 	buffer = cpu_buffer->buffer;
+	max_loops = cpu_buffer->ring_meta ? cpu_buffer->nr_pages : 3;
 
 	/*
 	 * Check if someone performed a consuming read to the buffer
@@ -6133,7 +6135,7 @@ rb_iter_peek(struct ring_buffer_iter *iter, u64 *ts)
 	 * the ring buffer with an active write as the consumer is.
 	 * Do not warn if the three failures is reached.
 	 */
-	if (++nr_loops > 3)
+	if (++nr_loops > max_loops)
 		return NULL;
 
 	if (rb_per_cpu_empty(cpu_buffer))


I'll test this some more, and make a proper patch.

-- Steve

On Sat, 2 May 2026 18:17:06 -0400
Steven Rostedt <rostedt@kernel.org> wrote:

> On Sat, 2 May 2026 15:23:04 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Hi Masami,
> > 
> > I applied your patches and enabled your ptracingtest code. I noticed
> > that when there's dropped pages, the trace output is not in order:
> > 
> >  # trace-cmd start -B ptracingtest -e all -v -e '*lock*'
> >  # taskset -c 5 echo c > /proc/sysrq-trigger
> > 
> > On reboot, I ran:
> > 
> >  # trace-cmd show -B ptracingtest > /tmp/trace.out
> > 
> > Then executed the attached perl program:
> > 
> >   # ./read-ts.pl < /tmp/trace.out
> > 
> > And it errors our:
> > 
> >  30.212495 < 30.213534
> >            <...>-1048    [005] d....    30.212495: irq_enable: caller=irqentry_exit+0xf5/0x710 parent=0x0
> > 
> > That is, I think the zero timestamps may be messing with the order.
> > 
> 
> Ah, I think I found the problem. The iterator needs the same logic you
> added for the consuming read:
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 7bfbed0ac90c..90a7fa772fe3 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -6105,12 +6105,14 @@ rb_iter_peek(struct ring_buffer_iter *iter, u64 *ts)
>  	struct ring_buffer_per_cpu *cpu_buffer;
>  	struct ring_buffer_event *event;
>  	int nr_loops = 0;
> +	int max_loops;
>  
>  	if (ts)
>  		*ts = 0;
>  
>  	cpu_buffer = iter->cpu_buffer;
>  	buffer = cpu_buffer->buffer;
> +	max_loops = cpu_buffer->ring_meta ? cpu_buffer->nr_pages : 3;
>  
>  	/*
>  	 * Check if someone performed a consuming read to the buffer
> @@ -6133,7 +6135,7 @@ rb_iter_peek(struct ring_buffer_iter *iter, u64 *ts)
>  	 * the ring buffer with an active write as the consumer is.
>  	 * Do not warn if the three failures is reached.
>  	 */
> -	if (++nr_loops > 3)
> +	if (++nr_loops > max_loops)
>  		return NULL;
>  
>  	if (rb_per_cpu_empty(cpu_buffer))
> 
> 
> I'll test this some more, and make a proper patch.

Ah, indeed. Thanks for fixing!

BTW, shouldn't we unify common logic of those functions?

Thank you,

> 
> -- Steve
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

On Thu, 7 May 2026 13:14:16 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > I'll test this some more, and make a proper patch.  
> 
> Ah, indeed. Thanks for fixing!
> 
> BTW, shouldn't we unify common logic of those functions?

Hmm, there's not much common between the two. One is a consuming read and
the other is a non-consuming read that needs to test for a bunch of race
conditions.

If you see something that can be shared, I'm all for it.

-- Steve

On Mon, 11 May 2026 12:29:43 -0400
Steven Rostedt <rostedt@kernel.org> wrote:

> On Thu, 7 May 2026 13:14:16 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > > I'll test this some more, and make a proper patch.  
> > 
> > Ah, indeed. Thanks for fixing!
> > 
> > BTW, shouldn't we unify common logic of those functions?
> 
> Hmm, there's not much common between the two. One is a consuming read and
> the other is a non-consuming read that needs to test for a bunch of race
> conditions.
> 
> If you see something that can be shared, I'm all for it.

Maybe we can introduce a common inline function to calculate
max_loop, or at least replacing "3" with a common macro.

Thank you,


> 
> -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>