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

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 14 May 2025 15:00:59 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > 
> > 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.

What we could do, and I don't think this would be too hard, is once the
buffer is empty and it's still LAST_BOOT buffer, we simply clear it in
the kernel.

That way after a reboot, a read of trace_pipe that reads the entire
buffer will end up resetting the buffer, and I think that will solve
this problem.



> > +
> > +		/* 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 validator takes a bit of time. I would rather not do another loop
if we don't have to. If this is duplicate code, lets just make a static
inline helper function that does it and use that in both places.

> 
> > +	}
> > +
> > +	/* 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);

I thought about this, but because the pointers are used to encode
flags, I try to avoid using the list_*() functions all together on
these. Just to remind everyone that these are "special" lists.

I prefer it open coded because that way I can see exactly what it is
doing. Note, this is not just assigning pointers, it is also clearing
flags in the process.

We could add a comment that states something like:

	/*
	 * This is the same as:
	 *   list_insert(&orig_head->list, &cpu_buffer->read_page->list);
	 * but as it is also clearing flags, its open coded so that
	 * there's no chance that list_insert() gets optimized where
	 * it doesn't do the extra work that this is doing.
	 */

?

-- Steve


> 
> > +
> > +		/* Make the head_page the new 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->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;
> > +		}
> > +		head_page = orig_head;
> > +	}
> > +
> >  	/* Iterate until finding the commit page */
> >  	for (i = 0; i < meta->nr_subbufs + 1; i++, rb_inc_page(&head_page)) {
> >  
> > @@ -5348,7 +5439,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 +6732,7 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
> >  		cpu_buffer->read_bytes += rb_page_size(reader);
> >  
> >  		/* swap the pages */
> > -		rb_init_page(bpage);
> > +//		rb_init_page(bpage);
> >  		bpage = reader->page;
> >  		reader->page = data_page->data;
> >  		local_set(&reader->write, 0);  
> 
> Thank you,
> 
> 
>