On Wed, 13 Mar 2024 00:22:10 +0900 Masami Hiramatsu (Google) mhiramat@kernel.org wrote:
On Tue, 12 Mar 2024 09:19:20 -0400 Steven Rostedt rostedt@goodmis.org wrote:
From: "Steven Rostedt (Google)" rostedt@goodmis.org
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@kernel.org
Thanks!
I'm running it through my tests and when they finish, I'll be posting the for-linus patches.
-- Steve