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
