Re: [PATCH v2 1/2] ring-buffer: Fix full_waiters_pending in poll

[Steven Rostedt]

On Wed, 13 Mar 2024 00:22:10 +0900
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> wrote:

> On Tue, 12 Mar 2024 09:19:20 -0400
> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> 
> > From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
> > 
> > If a reader of the ring buffer is doing a poll, and waiting for the ring
> > buffer to hit a specific watermark, there could be a case where it gets
> > into an infinite ping-pong loop.
> > 
> > The poll code has:
> > 
> >   rbwork->full_waiters_pending = true;
> >   if (!cpu_buffer->shortest_full ||
> >       cpu_buffer->shortest_full > full)
> >          cpu_buffer->shortest_full = full;
> > 
> > The writer will see full_waiters_pending and check if the ring buffer is
> > filled over the percentage of the shortest_full value. If it is, it calls
> > an irq_work to wake up all the waiters.
> > 
> > But the code could get into a circular loop:
> > 
> > 	CPU 0					CPU 1
> > 	-----					-----
> >  [ Poll ]
> >    [ shortest_full = 0 ]
> >    rbwork->full_waiters_pending = true;
> > 					  if (rbwork->full_waiters_pending &&
> > 					      [ buffer percent ] > shortest_full) {
> > 					         rbwork->wakeup_full = true;
> > 					         [ queue_irqwork ]  
> 
> Oh, so `[ buffer percent ] > shortest_full` does not work because
> if this happens in this order, shortest_full may be 0.

Exactly!

> 
> > 
> >    cpu_buffer->shortest_full = full;
> > 
> > 					  [ IRQ work ]
> > 					  if (rbwork->wakeup_full) {
> > 					        cpu_buffer->shortest_full = 0;

And here shortest_full gets set back to zero! (But that's not the bug).

> > 					        wakeup poll waiters;
> >   [woken]
> >    if ([ buffer percent ] > full)
> >       break;
> >    rbwork->full_waiters_pending = true;

The bug is setting full_waiters_pending before updating the shortest_full.

> > 					  if (rbwork->full_waiters_pending &&
> > 					      [ buffer percent ] > shortest_full) {
> > 					         rbwork->wakeup_full = true;
> > 					         [ queue_irqwork ]
> > 
> >    cpu_buffer->shortest_full = full;
> > 
> > 					  [ IRQ work ]
> > 					  if (rbwork->wakeup_full) {
> > 					        cpu_buffer->shortest_full = 0;
> > 					        wakeup poll waiters;
> >   [woken]
> > 
> >  [ Wash, rinse, repeat! ]
> > 
> > In the poll, the shortest_full needs to be set before the
> > full_pending_waiters, as once that is set, the writer will compare the
> > current shortest_full (which is incorrect) to decide to call the irq_work,
> > which will reset the shortest_full (expecting the readers to update it).
> > 
> > Also move the setting of full_waiters_pending after the check if the ring
> > buffer has the required percentage filled. There's no reason to tell the
> > writer to wake up waiters if there are no waiters.
> >   
> 
> Looks good to me.
> 
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>

Thanks!

I'm running it through my tests and when they finish, I'll be posting the
for-linus patches.

-- Steve