Re: [RFC PATCH] tracing: ring_buffer: Rewind persistent ring buffer when reboot

On Tue, 13 May 2025 20:32:37 -0400
Steven Rostedt [off-list ref] wrote:

On Tue, 13 May 2025 09:50:27 +0900
"Masami Hiramatsu (Google)" [off-list ref] wrote:

quoted

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`.

Is that a problem? I'm thinking that the data in the buffer should not be
used.

Yes, even if we read (dump) the previous boot data, the data is
in the buffer. Thus the kernel rebooted before reusing the buffer
the dumped pages are recovered again. Unless comparing with the
previous dump data, we can not know this data is older boot or not.
Anyway, user can avoid this issue by clearing the trace buffer
explicitly.

Anyway, I had to fix a bug with trace_pipe:

  https://lore.kernel.org/linux-trace-kernel/20250513115032.3e0b97f7@gandalf.local.home/ (local)

And with that applied, it doesn't rewind all the way, but still gives you
what's left on the reader page. I think that is the case without this patch
too.

OK.

quoted

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/ring_buffer.c |   55
++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 52
insertions(+), 3 deletions(-)

Anyway, I found that this is a lot more complex than what you have here. As
the commit size is set to zero when added back into the ring buffer, I
needed to remove that. But things get much more trickier than that!

First, we need to make the new head page found (the one that was read
already) the new "reader page". And we need to insert the old reader page
back into the ring buffer. And reset everything to handle all of this.

Yeah, I noticed the timestamps were messed up with my patch. That will
be sorted if we use external tools like perfetto, but still not good
for tracefs users.

I played a little with it, and came up with this. It's still a bit buggy,
but it does give you an idea of what needs to be done.

Thanks! I will work on this version.

Nice catch! ts should be better here.

+
+	/*
+	 * 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)
+		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;
+
+		if (ts < head_page->page->time_stamp)
+			break;
+
+		ts = head_page->page->time_stamp;
+		if (!ts)
+			break;
+
+		if (rb_page_commit(head_page) == 0)
+			break;

Ah, OK. the single 0 size page can be the end.

+
+		/* 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. */
+		local_set(&head_page->entries, ret);
+		if (ret)
+			local_inc(&cpu_buffer->pages_touched);
+		entries += ret;
+		entry_bytes += rb_page_commit(head_page);

If we validate the pages again later (because fixing head_page),
we can skip this part.

+	}
+
+	/* The last rewind page must be skipped. */
+	if (head_page != orig_head)
+		rb_inc_page(&head_page);
+
+	if (head_page != orig_head) {

Ah, I forgot this part (setup new reader_page)

+		struct buffer_page *bpage = orig_head;
+
+		rb_dec_page(&bpage);
+		/*
+		 * Move the reader page between the orig_head and the page
+		 * before it.
+		 */

-----

+		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;

-----
These seems the same as (because head_page->list.prev->next encodes
flags, but we don't read that pointer.);

		list_insert(&orig_head->list, &cpu_buffer->reader_page->list);

Thank you,



-- 
Masami Hiramatsu (Google) [off-list ref]