Re: [PATCH] ring-buffer: Do not trigger WARN_ON() due to a commit_overrun

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 28 May 2025 10:42:03 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > The way to differentiate this case from the normal case of there
> > only being one page written to where the swap of the reader page
> > received that one page (which is the commit page), check if the
> > tail page is on the reader page. The difference between the commit
> > page and the tail page is that the tail page is where new writes go
> > to, and the commit page holds the first write that hasn't been
> > committed yet. In the case of an interrupt preempting the write of
> > an event and filling the buffer, it would move the tail page but
> > not the commit page.  
> 
> (BTW, what happen if the interrupted process commits the event? That
>  event will be lost, or commit and just move commit_page?)

No, the first event to be created is the "commit" event. If it gets
interrupted and the interrupt adds a bunch of events that wraps the
ring buffer, it can't touch the commit event, it will just start
dropping events. Then when the commit event finishes, it can either be
read by the reader, or overwritten by the next events coming in.

> 
> 
> Thus the reader_page == commit_page && reader_page == tail_page,
> missed_events should be 0?
> 
> Possible cases if missed_events != 0:
> 
>  - reader_page != commit_page
> 	(writer's commit overtook the reader)

The reader is never in the write buffer. Just the head page will move.
When a new reader page is taken it will swap out the old reader page
with the head page. If the head page is the commit page, then the
commit page becomes the reader page too.

> 
>  - reader_page == commit_page but reader_page != tail_page 
> 	(writer overtook the reader, but commit is not completed yet.)

No, "writer overtook the reader" doesn't make sense as the reader is
not on the write buffer, so the writer can not catch up to it. What the
write buffer has is the "head" page which is where the next reader will
come to.

The only way reader_page == commit_page and reader_page != tail_page is
if the commit was interrupted and the interrupt added events and moved
forward off the commit_page. The only way there would be missed events
in that case is if the interrupt added so many events it wrapped the
buffer and then started dropping events.

> 
> if 
>  - reader_page == commit_page == tail_page
> in this case, missed_events should be 0.
> 
> Since the reader_page is out of the ring buffer, writer should not
> use reader_page while reading the same reader_page, is that right?

Correct. But the writer could end up on the reader page after the swap,
if the head page happened to be the commit page.

> 


> > cpu_buffer->tail_page,
> > +				       "Reader on commit with %ld
> > missed events",
> > +				       missed_events)) {
> > +				/*
> > +				 * If the tail page is not on the
> > reader page but
> > +				 * the commit_page is, that would
> > mean that there's
> > +				 * a commit_overrun (an interrupt
> > preempted an
> > +				 * addition of an event and then
> > filled the buffer
> > +				 * with new events). In this case
> > it's not an
> > +				 * error, but it should still be
> > reported.
> > +				 */
> > +				pr_info("Ring buffer commit
> > overrun lost %ld events at timestamp:%lld\n",
> > +					missed_events,
> > cpu_buffer->reader_page->page->time_stamp);  
> 
> Do we need this pr_info() for each commit overrun?

Yes. When doing this stress test, it printed at most 4 times. It
happens once per time the interrupt fills the buffer while interrupting
the buffer.

I seldom ever get commit overruns. It's one of the fields in the status
file located in: /sys/kernel/tracing/per_cpu/cpu*/stats

> 
> > +			}
> > +		}
> >  	}  
> 
> Just for cleanup the code idea, with above change, this code is
> something like;
> 
> ----------------
> 
> missed_events = cpu_buffer->lost_events;
> 
> if (cpu_buffer->reader_page != cpu_buffer->commit_page) {
> 	if (missed_event) {
> 
> 	}
> } else {
> 	if (missed_event) {
> 		if (!WARN_ONCE(cpu_buffer->reader_page ==
> cpu_buffer->tail_page,"...")) { pr_info("...")
> 		}
> 	}
> }
> 
> ----------------
> 
> Can we make it as below?
> 
> ----------------
> missed_events = cpu_buffer->lost_events;
> 
> if (missed_event) {
> 	if (cpu_buffer->reader_page != cpu_buffer->commit_page) {
> 
> 	} else if (!WARN_ONCE(cpu_buffer->reader_page ==
> cpu_buffer->tail_page, "...") { /**/
> 		pr_info("..."); 
> 	}
> }

Hmm, OK, I'll look at that.

Thanks,

-- Steve