Re: [PATCH v21 8/9] ring-buffer: Show persistent buffer dropped events in trace file

[Google]

On Tue, 26 May 2026 13:41:16 -0400
Steven Rostedt <rostedt@xxxxxxxxxx> wrote:

> On Tue, 26 May 2026 14:06:09 +0900
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> wrote:
> 
> > > @@ -7204,10 +7209,12 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
> > >  	 * Set a flag in the commit field if we lost events
> > >  	 */
> > >  	if (missed_events) {
> > > -		/* If there is room at the end of the page to save the
> > > +		/*
> > > +		 * If there is room at the end of the page to save the
> > >  		 * missed events, then record it there.
> > >  		 */
> > > -		if (buffer->subbuf_size - commit >= sizeof(missed_events)) {
> > > +		if (missed_events > 0 &&
> > > +		    buffer->subbuf_size - commit >= sizeof(missed_events)) {
> > >  			memcpy(&dpage->data[commit], &missed_events,
> > >  			       sizeof(missed_events));
> > >  			local_add(RB_MISSED_STORED, &dpage->commit);  
> > 
> > After this line, we "add" RB_MISSED_EVENTS instead of set.
> > In this case, does it clear the RB_MISSED_EVENTS bit because
> > it already sets RB_MISSED_EVENTS.
> > 
> > 			commit += sizeof(missed_events);
> > 		}
> > 		local_add(RB_MISSED_EVENTS, &bpage->commit);
> >                       ^^^ here.
> 
> Perhaps this needs to be commented better.
> 
> The answer to your question is "No". The reason is that this is a *copy* of
> the page we are reading. As persistent pages are always assigned to
> specific memory, it can never leave the buffer even for the splice system
> call. It is always copied to a new page.
> 
> The new page doesn't have these bits set and needs to set them depending on
> what was found when reading the page from the buffer.
> 
> Now if this was a normal ring buffer where it did a zero copy from the
> buffer itself by swapping pages with the passed in page, if the bit was set
> before, then adding would cause a problem. But normal ring buffer pages
> never set these bits while in the buffer. They are only set by this function.

Yeah, for the persistent ring buffer, it does not happen.
But there seems RB_MISSED_EVENTS bit can be cleared in
"else" path (after applying 1-8 patches)?

----------
	if (read || (len < (commit - read)) ||
	    cpu_buffer->reader_page == cpu_buffer->commit_page ||
	    force_memcpy) { // <-- persistent ring buffer sets force_memcpy = true.
[...]
	} else {
		/* update the entry counter */
[...]
		if (!missed_events && rb_data_page_commit(dpage) & RB_MISSED_EVENTS)
			missed_events = -1;	
			//^-- we check RB_MISSED_EVENTS bit on @dpage->commit and set missed_events = -1.

		/*
		 * Use the real_end for the data size,
		 * This gives us a chance to store the lost events
		 * on the page.
		 */
		if (reader->real_end)
			local_set(&dpage->commit, reader->real_end);
                        // ^- only if @reader->real_end, RB_MISSED_EVENTS bit is dropped.
	}

	cpu_buffer->lost_events = 0;

	commit = rb_data_page_commit(dpage);
	/*
	 * Set a flag in the commit field if we lost events
	 */
	if (missed_events) {
		/*
		 * If there is room at the end of the page to save the
		 * missed events, then record it there.
		 */
		if (missed_events > 0 &&
		    buffer->subbuf_size - commit >= sizeof(missed_events)) {
			memcpy(&dpage->data[commit], &missed_events,
			       sizeof(missed_events));
			local_add(RB_MISSED_STORED, &dpage->commit);
			commit += sizeof(missed_events);
		}
		local_add(RB_MISSED_EVENTS, &dpage->commit); // <-- @dpage->commit is updated.
	}
----------

Thanks,


> 
> -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>