From 4c1dd505a6a68172c019bf20ae797f4fbf6b05c2 Mon Sep 17 00:00:00 2001
From: Nadav Har'El <nyh@cloudius-systems.com>
Date: Wed, 10 Jul 2013 22:04:44 +0300
Subject: [PATCH] rewrite virtio_driver::wait_for_queue
In my memcached tests (with mc_benchmark as the driver), I saw
virtio_driver::wait_for_queue appears to have some bug or race condition -
in some cases it hangs on waiting for the rx queue - and simply never
returns.
I can't say I understand what the bug in this code is, however.
Instead, I just wrote it from scratch in a different way, which I think
is much clearer - and this code no longer exhibits this bug.
I can't put my finger on why my new version is more correct than
the old one - or even just difference... Dor, maybe you can find a
difference? But it definitely behaves differently.
---
drivers/virtio.cc | 33 ++++++++++++++++-----------------
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/drivers/virtio.cc b/drivers/virtio.cc
index ad6178f6c..5741fb20e 100644
--- a/drivers/virtio.cc
+++ b/drivers/virtio.cc
@@ -192,24 +192,23 @@ vring* virtio_driver::get_virt_queue(unsigned idx)
void virtio_driver::wait_for_queue(vring* queue, bool (vring::*pred)() const)
{
- sched::thread::wait_until([queue,pred] {
- bool have_elements = (queue->*pred)();
- if (!have_elements) {
- queue->enable_interrupts();
-
- // we must check that the ring is not empty *after*
- // we enable interrupts to avoid a race where a packet
- // may have been delivered between queue->used_ring_not_empty()
- // and queue->enable_interrupts() above
- have_elements = (queue->*pred)();
- if (have_elements) {
- queue->disable_interrupts();
- }
- }
-
- trace_virtio_wait_for_queue(queue, have_elements);
- return have_elements;
+ if ((queue->*pred)()) {
+ return;
+ }
+ queue->enable_interrupts();
+ // It is possible that after checking pred() and before enabling
+ // interrupts, the delivered us a packet, for which we will not get an
+ // interrupt. So we much check again.
+ if ((queue->*pred)()) {
+ queue->disable_interrupts();
+ return;
+ }
+ sched::thread::wait_until([&] {
+ // Need to re-enable interrupts as the interrupt disabled them.
+ queue->enable_interrupts();
+ return (queue->*pred)();
});
+ queue->disable_interrupts();
}
u32 virtio_driver::get_device_features(void)
--
GitLab