[PATCH v3 0/2] tracing: Rewind persistent ring buffer

[PATCH v3 0/2] tracing: Rewind persistent ring buffer

[Masami Hiramatsu (Google)]

Hi,

Here is the 3rd version of the persistent ring buffer rewinding
patches. I added a related patch for resetting ring buffer when
all buffers are read to this series.

Thank you,

---

Masami Hiramatsu (Google) (2):
      tracing: ring_buffer: Rewind persistent ring buffer when reboot
      tracing: Reset last-boot buffers when reading out all cpu buffers


 kernel/trace/ring_buffer.c |  104 ++++++++++++++++++++++++++++++++++++++++++--
 kernel/trace/trace.c       |   22 +++++++++
 2 files changed, 122 insertions(+), 4 deletions(-)

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

[PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Masami Hiramatsu (Google)]

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

Rewind persistent ring buffer pages which have been read in the
previous boot. Those pages are highly possible to be lost before
writing it to the disk if the previous kernel crashed. In this
case, the trace data is kept on the persistent ring buffer, but
it can not be read because its commit size has been reset after
read.
This skips clearing the commit size of each sub-buffer and
recover it after reboot.

Note: If you read the previous boot data via trace_pipe, that
is not accessible in that time. But reboot without clearing (or
reusing) the read data, the read data is recovered again in the
next boot.
Thus, when you read the previous boot data, clear it by
`echo > trace`.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 Changes in v3:
  - Fix a label name to better one. (skip_rewind)
  - Fix comments.
  - Fix rewound log message and show only if rewound pages.
  - Fix to adjust cpu_buffer->pages correctly.
  - Fix a buffer overflow.
 Changes in v2:
  - Stop rewind if timestamp is not older.
  - Rewind reader page and reset all indexes.
  - Make ring_buffer_read_page() not clear the commit size.
---
 kernel/trace/ring_buffer.c |  104 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 100 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 6859008ca34d..5034bae02f08 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -1358,6 +1358,13 @@ static inline void rb_inc_page(struct buffer_page **bpage)
 	*bpage = list_entry(p, struct buffer_page, list);
 }
 
+static inline void rb_dec_page(struct buffer_page **bpage)
+{
+	struct list_head *p = rb_list_head((*bpage)->list.prev);
+
+	*bpage = list_entry(p, struct buffer_page, list);
+}
+
 static struct buffer_page *
 rb_set_head_page(struct ring_buffer_per_cpu *cpu_buffer)
 {
@@ -1866,10 +1873,11 @@ static int rb_validate_buffer(struct buffer_data_page *dpage, int cpu)
 static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 {
 	struct ring_buffer_cpu_meta *meta = cpu_buffer->ring_meta;
-	struct buffer_page *head_page;
+	struct buffer_page *head_page, *orig_head;
 	unsigned long entry_bytes = 0;
 	unsigned long entries = 0;
 	int ret;
+	u64 ts;
 	int i;
 
 	if (!meta || !meta->head_buffer)
@@ -1885,8 +1893,98 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 	entry_bytes += local_read(&cpu_buffer->reader_page->page->commit);
 	local_set(&cpu_buffer->reader_page->entries, ret);
 
-	head_page = cpu_buffer->head_page;
+	orig_head = head_page = cpu_buffer->head_page;
+	ts = head_page->page->time_stamp;
+
+	/*
+	 * Try to rewind the head so that we can read the pages which already
+	 * read in the previous boot.
+	 */
+	if (head_page == cpu_buffer->tail_page)
+		goto skip_rewind;
+
+	rb_dec_page(&head_page);
+	for (i = 0; i < meta->nr_subbufs + 1; i++, rb_dec_page(&head_page)) {
+
+		/* Rewind until tail (writer) page. */
+		if (head_page == cpu_buffer->tail_page)
+			break;
+
+		/* Ensure the page has older data than head. */
+		if (ts < head_page->page->time_stamp)
+			break;
+
+		ts = head_page->page->time_stamp;
+		/* Ensure the page has correct timestamp and some data. */
+		if (!ts || rb_page_commit(head_page) == 0)
+			break;
 
+		/* Stop rewind if the page is invalid. */
+		ret = rb_validate_buffer(head_page->page, cpu_buffer->cpu);
+		if (ret < 0)
+			break;
+
+		/* Recover the number of entries and update stats. */
+		local_set(&head_page->entries, ret);
+		if (ret)
+			local_inc(&cpu_buffer->pages_touched);
+		entries += ret;
+		entry_bytes += rb_page_commit(head_page);
+	}
+	if (i)
+		pr_info("Ring buffer [%d] rewound %d pages\n", cpu_buffer->cpu, i);
+
+	/* The last rewound page must be skipped. */
+	if (head_page != orig_head)
+		rb_inc_page(&head_page);
+
+	/*
+	 * If the ring buffer was rewound, then inject the reader page
+	 * into the location just before the original head page.
+	 */
+	if (head_page != orig_head) {
+		struct buffer_page *bpage = orig_head;
+
+		rb_dec_page(&bpage);
+		/*
+		 * Insert the reader_page before the original head page.
+		 * Since the list encode RB_PAGE flags, general list
+		 * operations should be avoided.
+		 */
+		cpu_buffer->reader_page->list.next = &orig_head->list;
+		cpu_buffer->reader_page->list.prev = orig_head->list.prev;
+		orig_head->list.prev = &cpu_buffer->reader_page->list;
+		bpage->list.next = &cpu_buffer->reader_page->list;
+
+		/* Make the head_page the reader page */
+		cpu_buffer->reader_page = head_page;
+		bpage = head_page;
+		rb_inc_page(&head_page);
+		head_page->list.prev = bpage->list.prev;
+		rb_dec_page(&bpage);
+		bpage->list.next = &head_page->list;
+		rb_set_list_to_head(&bpage->list);
+		cpu_buffer->pages = &head_page->list;
+
+		cpu_buffer->head_page = head_page;
+		meta->head_buffer = (unsigned long)head_page->page;
+
+		/* Reset all the indexes */
+		bpage = cpu_buffer->reader_page;
+		meta->buffers[0] = rb_meta_subbuf_idx(meta, bpage->page);
+		bpage->id = 0;
+
+		for (i = 1, bpage = head_page; i < meta->nr_subbufs;
+		     i++, rb_inc_page(&bpage)) {
+			meta->buffers[i] = rb_meta_subbuf_idx(meta, bpage->page);
+			bpage->id = i;
+		}
+
+		/* We'll restart verifying from orig_head */
+		head_page = orig_head;
+	}
+
+ skip_rewind:
 	/* If the commit_buffer is the reader page, update the commit page */
 	if (meta->commit_buffer == (unsigned long)cpu_buffer->reader_page->page) {
 		cpu_buffer->commit_page = cpu_buffer->reader_page;
@@ -5348,7 +5446,6 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 	 */
 	local_set(&cpu_buffer->reader_page->write, 0);
 	local_set(&cpu_buffer->reader_page->entries, 0);
-	local_set(&cpu_buffer->reader_page->page->commit, 0);
 	cpu_buffer->reader_page->real_end = 0;
 
  spin:
@@ -6642,7 +6739,6 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
 		cpu_buffer->read_bytes += rb_page_size(reader);
 
 		/* swap the pages */
-		rb_init_page(bpage);
 		bpage = reader->page;
 		reader->page = data_page->data;
 		local_set(&reader->write, 0);

[PATCH v3 2/2] tracing: Reset last-boot buffers when reading out all cpu buffers

[Masami Hiramatsu (Google)]

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

Reset the last-boot ring buffers when read() reads out all cpu
buffers through trace_pipe/trace_pipe_raw. This prevents ftrace to
unwind ring buffer read pointer next boot.

Note that this resets only when all per-cpu buffers are empty, and
read via read(2) syscall. For example, if you read only one of the
per-cpu trace_pipe, it does not reset it. Also, reading buffer by
splice(2) syscall does not reset because some data in the reader
(the last) page.

Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 2c1764ed87b0..433671d3aa43 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6677,6 +6677,22 @@ static int tracing_wait_pipe(struct file *filp)
 	return 1;
 }
 
+static bool update_last_data_if_empty(struct trace_array *tr)
+{
+	if (!(tr->flags & TRACE_ARRAY_FL_LAST_BOOT))
+		return false;
+
+	if (!ring_buffer_empty(tr->array_buffer.buffer))
+		return false;
+
+	/*
+	 * If the buffer contains the last boot data and all per-cpu
+	 * buffers are empty, reset it from the kernel side.
+	 */
+	update_last_data(tr);
+	return true;
+}
+
 /*
  * Consumer reader.
  */
@@ -6708,6 +6724,9 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
 	}
 
 waitagain:
+	if (update_last_data_if_empty(iter->tr))
+		return 0;
+
 	sret = tracing_wait_pipe(filp);
 	if (sret <= 0)
 		return sret;
@@ -8286,6 +8305,9 @@ tracing_buffers_read(struct file *filp, char __user *ubuf,
 
 	if (ret < 0) {
 		if (trace_empty(iter) && !iter->closed) {
+			if (update_last_data_if_empty(iter->tr))
+				return 0;
+
 			if ((filp->f_flags & O_NONBLOCK))
 				return -EAGAIN;
 

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Steven Rostedt]

On Fri, 23 May 2025 00:54:44 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

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

Unfortunately, I don't think this is going to make the merge window.

This failed one of my tests and it took me a while to figure it out. And I
think there may be some more subtle issues that we need further testing on.

> 
> Rewind persistent ring buffer pages which have been read in the
> previous boot. Those pages are highly possible to be lost before
> writing it to the disk if the previous kernel crashed. In this
> case, the trace data is kept on the persistent ring buffer, but
> it can not be read because its commit size has been reset after
> read.
> This skips clearing the commit size of each sub-buffer and
> recover it after reboot.
> 
> Note: If you read the previous boot data via trace_pipe, that
> is not accessible in that time. But reboot without clearing (or
> reusing) the read data, the read data is recovered again in the
> next boot.
> Thus, when you read the previous boot data, clear it by
> `echo > trace`.
> 
> @@ -5348,7 +5446,6 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>  	 */
>  	local_set(&cpu_buffer->reader_page->write, 0);
>  	local_set(&cpu_buffer->reader_page->entries, 0);
> -	local_set(&cpu_buffer->reader_page->page->commit, 0);

We need to make sure the above doesn't cause any issues. It wasn't the
reason for my test failure, but it's something we need to look more closely
at.


>  	cpu_buffer->reader_page->real_end = 0;
>  
>   spin:
> @@ -6642,7 +6739,6 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
>  		cpu_buffer->read_bytes += rb_page_size(reader);
>  
>  		/* swap the pages */
> -		rb_init_page(bpage);

This isn't needed, because the persistent ring buffer is never swapped. And
this is what caused my test to fail. For some reason (and I'm still trying
to figure out exactly why), it causes my stress test that runs:

  perf record -o perf-test.dat -a -- trace-cmd record -e all -p function /work/c/hackbench 10 || exit -1

To never end after tracing is stopped, and it fills up all the disk space.

But again, this part isn't needed because the persistent ring buffer
doesn't do the swapping. This replaces what the user passed in with the
current page.

>  		bpage = reader->page;
>  		reader->page = data_page->data;
>  		local_set(&reader->write, 0);

I think we also need this:

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 7d837963dd1e..456efebc396a 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -3638,6 +3638,9 @@ rb_move_tail(struct ring_buffer_per_cpu *cpu_buffer,
 
 	rb_reset_tail(cpu_buffer, tail, info);
 
+	/* The new page should have zero committed */
+	rb_init_page(next_page->page);
+
 	/* Commit what we have for now. */
 	rb_end_commit(cpu_buffer);
 	/* rb_end_commit() decs committing */

-- Steve

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Steven Rostedt]

On Fri, 23 May 2025 16:54:25 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> I think we also need this:
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 7d837963dd1e..456efebc396a 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -3638,6 +3638,9 @@ rb_move_tail(struct ring_buffer_per_cpu *cpu_buffer,
>  
>  	rb_reset_tail(cpu_buffer, tail, info);
>  
> +	/* The new page should have zero committed */
> +	rb_init_page(next_page->page);
> +
>  	/* Commit what we have for now. */
>  	rb_end_commit(cpu_buffer);
>  	/* rb_end_commit() decs committing */

No we don't need it ;-)

I'm looking deeper into the code and we have this:

		/*
		 * No need to worry about races with clearing out the commit.
		 * it only can increment when a commit takes place. But that
		 * only happens in the outer most nested commit.
		 */
		local_set(&next_page->page->commit, 0);

When the tail page gets moved.

-- Steve

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Steven Rostedt]

On Fri, 23 May 2025 16:54:25 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> >   spin:
> > @@ -6642,7 +6739,6 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
> >  		cpu_buffer->read_bytes += rb_page_size(reader);
> >  
> >  		/* swap the pages */
> > -		rb_init_page(bpage);  
> 
> This isn't needed, because the persistent ring buffer is never swapped. And
> this is what caused my test to fail. For some reason (and I'm still trying
> to figure out exactly why), it causes my stress test that runs:
> 
>   perf record -o perf-test.dat -a -- trace-cmd record -e all -p function /work/c/hackbench 10 || exit -1
> 
> To never end after tracing is stopped, and it fills up all the disk space.
> 
> But again, this part isn't needed because the persistent ring buffer
> doesn't do the swapping. This replaces what the user passed in with the
> current page.
> 
> >  		bpage = reader->page;
> >  		reader->page = data_page->data;
> >  		local_set(&reader->write, 0);  

So I analyzed why this fails and we need to reset the commit here.

Adding a bunch of trace_printk() (and using the new trace_printk_dest
option where I can have trace_printk go into an instance) I was able to see
why this was an issue.

This part of the code swaps the reader page with what was passed in by the
caller. The page doesn't go back into the write part of the ring buffer.
The "commit" field is used to know if there's more data or not.

By not resetting the "commit" field, we have:

	reader = rb_get_reader_page(cpu_buffer)
		if (cpu_buffer->reader_page->read < rb_page_size(reader))
			return reader;
	// swap passed in page with reader

Without resetting "commit", the caller consumes the page and then uses that
same page to pass back to this function. Since the "commit" field is never
zero'd, it the above if statement always returns true! And this function
just keeps swapping the reader each time and goes into an infinite loop
(this loop requires user space to do a read or splice on this page so it's
not a kernel infinite loop).

Now the question is, can this affect the persistent ring buffer too? I'll
memory map the buffer and see if it causes the same issue.

-- Steve

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Masami Hiramatsu]

On Fri, 23 May 2025 17:28:57 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 23 May 2025 16:54:25 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > I think we also need this:
> > 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 7d837963dd1e..456efebc396a 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -3638,6 +3638,9 @@ rb_move_tail(struct ring_buffer_per_cpu *cpu_buffer,
> >  
> >  	rb_reset_tail(cpu_buffer, tail, info);
> >  
> > +	/* The new page should have zero committed */
> > +	rb_init_page(next_page->page);
> > +
> >  	/* Commit what we have for now. */
> >  	rb_end_commit(cpu_buffer);
> >  	/* rb_end_commit() decs committing */
> 
> No we don't need it ;-)
> 
> I'm looking deeper into the code and we have this:
> 
> 		/*
> 		 * No need to worry about races with clearing out the commit.
> 		 * it only can increment when a commit takes place. But that
> 		 * only happens in the outer most nested commit.
> 		 */
> 		local_set(&next_page->page->commit, 0);
> 
> When the tail page gets moved.

Yeah, when the page gets written, it is cleared.

Thanks,

> 
> -- Steve


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

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Masami Hiramatsu]

On Fri, 23 May 2025 19:20:53 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 23 May 2025 16:54:25 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > >   spin:
> > > @@ -6642,7 +6739,6 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
> > >  		cpu_buffer->read_bytes += rb_page_size(reader);
> > >  
> > >  		/* swap the pages */
> > > -		rb_init_page(bpage);  
> > 
> > This isn't needed, because the persistent ring buffer is never swapped. And
> > this is what caused my test to fail. For some reason (and I'm still trying
> > to figure out exactly why), it causes my stress test that runs:
> > 
> >   perf record -o perf-test.dat -a -- trace-cmd record -e all -p function /work/c/hackbench 10 || exit -1
> > 
> > To never end after tracing is stopped, and it fills up all the disk space.

Thanks for sharing the test code. So it means perf reads the ring buffer
endlessly?

[reader commit=X1, ts=1]

[head commit=X2, ts=2]...[tail commit=Xn, ts=n]
  |                           |
  |---------------------------|

I thought that the read will end when it hits tail or ts < ts_current.


> > 
> > But again, this part isn't needed because the persistent ring buffer
> > doesn't do the swapping. This replaces what the user passed in with the
> > current page.
> > 
> > >  		bpage = reader->page;
> > >  		reader->page = data_page->data;
> > >  		local_set(&reader->write, 0);  
> 
> So I analyzed why this fails and we need to reset the commit here.
> 
> Adding a bunch of trace_printk() (and using the new trace_printk_dest
> option where I can have trace_printk go into an instance) I was able to see
> why this was an issue.
> 
> This part of the code swaps the reader page with what was passed in by the
> caller. The page doesn't go back into the write part of the ring buffer.
> The "commit" field is used to know if there's more data or not.
> 
> By not resetting the "commit" field, we have:
> 
> 	reader = rb_get_reader_page(cpu_buffer)
> 		if (cpu_buffer->reader_page->read < rb_page_size(reader))
> 			return reader;
> 	// swap passed in page with reader
> 
> Without resetting "commit", the caller consumes the page and then uses that
> same page to pass back to this function. Since the "commit" field is never
> zero'd, it the above if statement always returns true! And this function
> just keeps swapping the reader each time and goes into an infinite loop
> (this loop requires user space to do a read or splice on this page so it's
> not a kernel infinite loop).


Ah, in rb_get_reader_page(cpu_buffer),

	/* If there's more to read, return this page */
	if (cpu_buffer->reader_page->read < rb_page_size(reader))
		goto out;

	/* Never should we have an index greater than the size */
	if (RB_WARN_ON(cpu_buffer,
		       cpu_buffer->reader_page->read > rb_page_size(reader)))
		goto out;

	/* check if we caught up to the tail */
	reader = NULL;
	if (cpu_buffer->commit_page == cpu_buffer->reader_page)
		goto out;

It checks remaining (unread) data first, and move to the next.

> 
> Now the question is, can this affect the persistent ring buffer too? I'll
> memory map the buffer and see if it causes the same issue.

Yeah, it can happen, but I didn't hit that.
Let me test it too.

Hmm, BTW, is there any possible solution? records the consumed
bytes in meta data?


> 
> -- Steve


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

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Masami Hiramatsu]

On Mon, 26 May 2025 12:03:42 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Fri, 23 May 2025 17:28:57 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 23 May 2025 16:54:25 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > I think we also need this:
> > > 
> > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > > index 7d837963dd1e..456efebc396a 100644
> > > --- a/kernel/trace/ring_buffer.c
> > > +++ b/kernel/trace/ring_buffer.c
> > > @@ -3638,6 +3638,9 @@ rb_move_tail(struct ring_buffer_per_cpu *cpu_buffer,
> > >  
> > >  	rb_reset_tail(cpu_buffer, tail, info);
> > >  
> > > +	/* The new page should have zero committed */
> > > +	rb_init_page(next_page->page);
> > > +
> > >  	/* Commit what we have for now. */
> > >  	rb_end_commit(cpu_buffer);
> > >  	/* rb_end_commit() decs committing */
> > 
> > No we don't need it ;-)
> > 
> > I'm looking deeper into the code and we have this:
> > 
> > 		/*
> > 		 * No need to worry about races with clearing out the commit.
> > 		 * it only can increment when a commit takes place. But that
> > 		 * only happens in the outer most nested commit.
> > 		 */
> > 		local_set(&next_page->page->commit, 0);
> > 
> > When the tail page gets moved.
> 
> Yeah, when the page gets written, it is cleared.

What about this? It seems if we keep the reader->page->commit, when we
catch up the tail page, rb_get_reader_page() returns new reader (== tail)
because reader->read(==0) < reader->page->commit. But we should not return
new reader.


diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 5034bae02f08..179142db9586 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -5522,6 +5522,12 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
                cpu_buffer->last_overrun = overwrite;
        }
 
+       /* But if we catch up to the tail, do not return reader page. */
+       if (cpu_buffer->commit_page == cpu_buffer->reader_page) {
+               reader = NULL;
+               goto out;
+       }
+
        goto again;
 
  out:


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

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Masami Hiramatsu]

On Tue, 27 May 2025 09:17:43 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Mon, 26 May 2025 12:03:42 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > On Fri, 23 May 2025 17:28:57 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > On Fri, 23 May 2025 16:54:25 -0400
> > > Steven Rostedt <rostedt@goodmis.org> wrote:
> > > 
> > > > I think we also need this:
> > > > 
> > > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > > > index 7d837963dd1e..456efebc396a 100644
> > > > --- a/kernel/trace/ring_buffer.c
> > > > +++ b/kernel/trace/ring_buffer.c
> > > > @@ -3638,6 +3638,9 @@ rb_move_tail(struct ring_buffer_per_cpu *cpu_buffer,
> > > >  
> > > >  	rb_reset_tail(cpu_buffer, tail, info);
> > > >  
> > > > +	/* The new page should have zero committed */
> > > > +	rb_init_page(next_page->page);
> > > > +
> > > >  	/* Commit what we have for now. */
> > > >  	rb_end_commit(cpu_buffer);
> > > >  	/* rb_end_commit() decs committing */
> > > 
> > > No we don't need it ;-)
> > > 
> > > I'm looking deeper into the code and we have this:
> > > 
> > > 		/*
> > > 		 * No need to worry about races with clearing out the commit.
> > > 		 * it only can increment when a commit takes place. But that
> > > 		 * only happens in the outer most nested commit.
> > > 		 */
> > > 		local_set(&next_page->page->commit, 0);
> > > 
> > > When the tail page gets moved.
> > 
> > Yeah, when the page gets written, it is cleared.
> 
> What about this? It seems if we keep the reader->page->commit, when we
> catch up the tail page, rb_get_reader_page() returns new reader (== tail)
> because reader->read(==0) < reader->page->commit. But we should not return
> new reader.
> 
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 5034bae02f08..179142db9586 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -5522,6 +5522,12 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>                 cpu_buffer->last_overrun = overwrite;
>         }
>  
> +       /* But if we catch up to the tail, do not return reader page. */
> +       if (cpu_buffer->commit_page == cpu_buffer->reader_page) {


Hmm, I found this might not work if it is called twice.
Maybe rb_get_reader_page() can check the reader_page->ts > head_page->ts
and return NULL (or reader_page if read < commit) at first.

Thank you,

> +               reader = NULL;
> +               goto out;
> +       }
> +
>         goto again;
>  
>   out:
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>


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

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Masami Hiramatsu]

On Tue, 27 May 2025 12:15:49 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:


> > What about this? It seems if we keep the reader->page->commit, when we
> > catch up the tail page, rb_get_reader_page() returns new reader (== tail)
> > because reader->read(==0) < reader->page->commit. But we should not return
> > new reader.
> > 
> > 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 5034bae02f08..179142db9586 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -5522,6 +5522,12 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
> >                 cpu_buffer->last_overrun = overwrite;
> >         }
> >  
> > +       /* But if we catch up to the tail, do not return reader page. */
> > +       if (cpu_buffer->commit_page == cpu_buffer->reader_page) {
> 
> 
> Hmm, I found this might not work if it is called twice.
> Maybe rb_get_reader_page() can check the reader_page->ts > head_page->ts
> and return NULL (or reader_page if read < commit) at first.

Here is what I meant. As far as I ran my test, it looks good (it prevents
over-read by `cat per_cpu/cpu0/trace_pipe_raw`)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 5034bae02f08..de1831eb3446 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -5405,6 +5405,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
        unsigned long flags;
        int nr_loops = 0;
        bool ret;
+       u64 ts;
 
        local_irq_save(flags);
        arch_spin_lock(&cpu_buffer->lock);
@@ -5423,6 +5424,18 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 
        reader = cpu_buffer->reader_page;
 
+       /*
+        * Now the page->commit is not cleared when it read.
+        * Check whether timestamp is newer instead. We also don't
+        * care the head_page is overwritten. In that case, timestamp
+        * should be newer than reader timestamp too.
+        */
+       ts = cpu_buffer->head_page->page->time_stamp;
+       if (ts < reader->page->time_stamp) {
+               reader = NULL;
+               goto out;
+       }
+
        /* If there's more to read, return this page */
        if (cpu_buffer->reader_page->read < rb_page_size(reader))
                goto out;



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

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Steven Rostedt]

On Tue, 27 May 2025 12:54:44 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> Here is what I meant. As far as I ran my test, it looks good (it prevents
> over-read by `cat per_cpu/cpu0/trace_pipe_raw`)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 5034bae02f08..de1831eb3446 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -5405,6 +5405,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>         unsigned long flags;
>         int nr_loops = 0;
>         bool ret;
> +       u64 ts;
>  
>         local_irq_save(flags);
>         arch_spin_lock(&cpu_buffer->lock);
> @@ -5423,6 +5424,18 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>  
>         reader = cpu_buffer->reader_page;
>  
> +       /*
> +        * Now the page->commit is not cleared when it read.
> +        * Check whether timestamp is newer instead. We also don't
> +        * care the head_page is overwritten. In that case, timestamp
> +        * should be newer than reader timestamp too.
> +        */
> +       ts = cpu_buffer->head_page->page->time_stamp;
> +       if (ts < reader->page->time_stamp) {

Hmm, I think this test may be too fragile. The head_page can be moved
by the writer, and this would need to handle races.

I found an issue with commit overflow and have a couple of bugs to fix that
touches some of this code. Let's revisit after I get those fixed.

Thanks,

-- Steve



> +               reader = NULL;
> +               goto out;
> +       }
> +
>         /* If there's more to read, return this page */
>         if (cpu_buffer->reader_page->read < rb_page_size(reader))
>                 goto out;
> 

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Steven Rostedt]

On Mon, 26 May 2025 12:44:03 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > 
> > Now the question is, can this affect the persistent ring buffer too? I'll
> > memory map the buffer and see if it causes the same issue.  
> 
> Yeah, it can happen, but I didn't hit that.
> Let me test it too.
> 
> Hmm, BTW, is there any possible solution? records the consumed
> bytes in meta data?

After memory mapping the main buffer, I found a couple of bugs. I'll have
the fixes out shortly.

-- Steve

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Masami Hiramatsu]

On Tue, 27 May 2025 09:47:57 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 27 May 2025 12:54:44 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > Here is what I meant. As far as I ran my test, it looks good (it prevents
> > over-read by `cat per_cpu/cpu0/trace_pipe_raw`)
> > 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 5034bae02f08..de1831eb3446 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -5405,6 +5405,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
> >         unsigned long flags;
> >         int nr_loops = 0;
> >         bool ret;
> > +       u64 ts;
> >  
> >         local_irq_save(flags);
> >         arch_spin_lock(&cpu_buffer->lock);
> > @@ -5423,6 +5424,18 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
> >  
> >         reader = cpu_buffer->reader_page;
> >  
> > +       /*
> > +        * Now the page->commit is not cleared when it read.
> > +        * Check whether timestamp is newer instead. We also don't
> > +        * care the head_page is overwritten. In that case, timestamp
> > +        * should be newer than reader timestamp too.
> > +        */
> > +       ts = cpu_buffer->head_page->page->time_stamp;
> > +       if (ts < reader->page->time_stamp) {
> 
> Hmm, I think this test may be too fragile. The head_page can be moved
> by the writer, and this would need to handle races.

Good point! Can we pick the page out from ring buffer as same
as reader_page? If its timestamp is newer, we push the reader
page (swap reader and head), or push back the header page.

> 
> I found an issue with commit overflow and have a couple of bugs to fix that
> touches some of this code. Let's revisit after I get those fixed.

OK, let's review it.

BTW, we need a ring buffer test tool not depending on perf tool.

Thank you,

> 
> Thanks,
> 
> -- Steve
> 
> 
> 
> > +               reader = NULL;
> > +               goto out;
> > +       }
> > +
> >         /* If there's more to read, return this page */
> >         if (cpu_buffer->reader_page->read < rb_page_size(reader))
> >                 goto out;
> > 
> 


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

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Steven Rostedt]

On Fri, 23 May 2025 00:54:44 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> @@ -6642,7 +6739,6 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
>  		cpu_buffer->read_bytes += rb_page_size(reader);
>  
>  		/* swap the pages */
> -		rb_init_page(bpage);
>  		bpage = reader->page;
>  		reader->page = data_page->data;
>  		local_set(&reader->write, 0);

Here's the bug we were looking for. We definitely need to keep the
rb_init_page() here!

That's because this is the } else { part of the if condition that on true
will do a copy and not a swap of the reader page.

ring_buffer_read_page() has:

	if (read || (len < (commit - read)) ||
	    cpu_buffer->reader_page == cpu_buffer->commit_page ||
	    cpu_buffer->mapped) {

		// Copy the buffer to the passed in data_page

	} else {

		// swap the data_page with the reader page

	}

The else part is here, and it's swapping the passed in data_page with the
current reader_page. We have to initialize the data_page here.

What we see happening is:

	info->spare = ring_buffer_alloc_read_page(); <-- this is the data_page

	[..]

	ret = ring_buffer_read_page(..., info->spare, ...);

Since this is a normal buffer, we swap the reader_page with info->spare,
where info->spare now has the reader_page.

It consumes the data via:

	trace_data = ring_buffer_read_data(info->spare);

Then reads the buffer again:

	ret = ring_buffer_read_page(..., info->spare, ...);

Now it hits the if statement again:

	if (read || (len < (commit - read)) ||
	    cpu_buffer->reader_page == cpu_buffer->commit_page ||
	    cpu_buffer->mapped) {

		// Copy the buffer to the passed in data_page

	} else {

		// swap the data_page with the reader page

---->> here it just swaps the last data_page with the current one.

	}


As we never clear the "commit" part of the page, it still thinks it has
content on the page as the "read" was set to zero, and it reads the old
content again.

There's no reason not to clear the "commit" of the data_page passed in. It
is not old data that is about to be lost. We most definitely need to call
rb_init_page() on it.

After adding that back, trace-cmd record works properly again.

Care to send a v4 without removing this rb_init_page(bpage); ?

-- Steve

Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Masami Hiramatsu]

On Tue, 3 Jun 2025 12:00:49 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 23 May 2025 00:54:44 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > @@ -6642,7 +6739,6 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
> >  		cpu_buffer->read_bytes += rb_page_size(reader);
> >  
> >  		/* swap the pages */
> > -		rb_init_page(bpage);
> >  		bpage = reader->page;
> >  		reader->page = data_page->data;
> >  		local_set(&reader->write, 0);
> 
> Here's the bug we were looking for. We definitely need to keep the
> rb_init_page() here!

Ah, thanks for finding the bug!

> 
> That's because this is the } else { part of the if condition that on true
> will do a copy and not a swap of the reader page.
> 
> ring_buffer_read_page() has:
> 
> 	if (read || (len < (commit - read)) ||
> 	    cpu_buffer->reader_page == cpu_buffer->commit_page ||
> 	    cpu_buffer->mapped) {
> 
> 		// Copy the buffer to the passed in data_page
> 
> 	} else {
> 
> 		// swap the data_page with the reader page
> 
> 	}
> 
> The else part is here, and it's swapping the passed in data_page with the
> current reader_page. We have to initialize the data_page here.

Oops, I confused the "swap" meant reader <-> head, but that should be
done in rb_get_reader_page(), not here.

> 
> What we see happening is:
> 
> 	info->spare = ring_buffer_alloc_read_page(); <-- this is the data_page
> 
> 	[..]
> 
> 	ret = ring_buffer_read_page(..., info->spare, ...);
> 
> Since this is a normal buffer, we swap the reader_page with info->spare,
> where info->spare now has the reader_page.
> 
> It consumes the data via:
> 
> 	trace_data = ring_buffer_read_data(info->spare);
> 
> Then reads the buffer again:
> 
> 	ret = ring_buffer_read_page(..., info->spare, ...);
> 
> Now it hits the if statement again:
> 
> 	if (read || (len < (commit - read)) ||
> 	    cpu_buffer->reader_page == cpu_buffer->commit_page ||
> 	    cpu_buffer->mapped) {
> 
> 		// Copy the buffer to the passed in data_page

So all mapped buffers (including persistent ring buffer) pass
this block.

> 
> 	} else {
> 
> 		// swap the data_page with the reader page
> 
> ---->> here it just swaps the last data_page with the current one.

So the data_page should not have any data. its commit should be 0.

> 
> 	}
> 
> 
> As we never clear the "commit" part of the page, it still thinks it has
> content on the page as the "read" was set to zero, and it reads the old
> content again.

Got it.

> 
> There's no reason not to clear the "commit" of the data_page passed in. It
> is not old data that is about to be lost. We most definitely need to call
> rb_init_page() on it.

Yeah, that data should be considered as cleared.

> 
> After adding that back, trace-cmd record works properly again.
> 
> Care to send a v4 without removing this rb_init_page(bpage); ?

OK, let me remove that (also remove timestamp check).

Thank you!

> 
> -- Steve


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