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

On Thu, 15 May 2025 20:15:56 +0900
"Masami Hiramatsu (Google)" [off-list ref] wrote:

Hmm, jumping to a label called "rewound" when you didn't do a rewind seems
confusing.

Perhaps call the label "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);
+	}
+	pr_info("Rewound %d pages on cpu%d\n", i, cpu_buffer->cpu);

Should state this is coming from the ring buffer and use "[%d]" for cpu
number as the other pr_info()'s do. Also only print if it did a rewind:

	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 there are rewound pages, rewind the reader page too. */

I would change the comment to:

	/*
	 * 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 tthe new read page */

Typo "tthe" and call it "new reader page", not "read 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->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 = 0, bpage = head_page; i < meta->nr_subbufs;
+		     i++, rb_inc_page(&bpage)) {
+			meta->buffers[i + 1] = rb_meta_subbuf_idx(meta, bpage->page);
+			bpage->id = i + 1;
+		}
+
+		/* We'll restart verifying from orig_head */
+		head_page = orig_head;
+	}
+
+ rewound:

 skip_rewind:

Also, I know other's don't like to do this, but I do add a space before
labels. It makes patch diffs easier to see which functions they are,
otherwise the patch shows the label and not the function.

-- Steve