Marcelo Tosatti wrote:
On Sun, May 31, 2009 at 03:14:36PM +0300, Avi Kivity wrote:
Marcelo Tosatti wrote:
Move coalesced_mmio locking to its own device, instead of relying on
kvm->lock.
Signed-off-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
Index: kvm-irqlock/virt/kvm/coalesced_mmio.c
===================================================================
--- kvm-irqlock.orig/virt/kvm/coalesced_mmio.c
+++ kvm-irqlock/virt/kvm/coalesced_mmio.c
@@ -26,9 +26,7 @@ static int coalesced_mmio_in_range(struc
if (!is_write)
return 0;
- /* kvm->lock is taken by the caller and must be not released before
- * dev.read/write
- */
+ spin_lock(&dev->lock);
This unbalanced locking is still very displeasing. At a minimum you
need a sparse annotation to indicate it.
But I think it really indicates a problem with the io_device API.
Potential solutions:
- fold in_range() into ->write and ->read. Make those functions
responsible for both determining whether they can handle the range and
performing the I/O.
- have a separate rwlock for the device list.
IMO the problem is the coalesced_mmio device. The unbalanced locking is
a result of the abuse of the in_range() and read/write() methods.
Okay, the penny has dropped. I understand now.
Normally you'd expect parallel accesses to in_range() to be allowed,
since its just checking whether (aha) the access is in range, returning
a pointer to the device if positive. Now read/write() are the ones who
need serialization, since they touch the device internal state.
coalesced_mmio abuses in_range() to do more things than it should.
Ideally we should fix coalesced_mmio, but i'm not going to do that now
(sorry, not confident in changing it without seeing go through intense
torture testing).
It's not trivial since it's userspace that clears the ring, and we can't
wait on userspace.
That said, is sparse annotation enough the convince you?
Let me have a look at fixing coalesced_mmio first. We might allow
->write to fail, causing a fallback to userspace. Or we could fail if
n_avail < MAX_VCPUS, so even the worst-case race leaves us one entry.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
