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

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

On Wed, 14 May 2025 09:00:50 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

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

Ah, that sounds good :-D

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

OK, I think we can just restart validating unread part from
orig_head.

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

OK. And I found list_insert() is not in the kernel.
(tools/firmware/list.h has that)

> 
> 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.
> 	 */
> 
> ?

Yeah, anyway I will leave a comment.

Thank you,

> 
> -- 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,
> > 
> > 
> > 
> 


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