| Message ID | 1304058985-13833-3-git-send-email-asias.hejun@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Asias He <asias.hejun@gmail.com> wrote: > Signed-off-by: Asias He <asias.hejun@gmail.com> > --- > tools/kvm/virtio-blk.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/tools/kvm/virtio-blk.c b/tools/kvm/virtio-blk.c > index 3feabd0..ade6335 100644 > --- a/tools/kvm/virtio-blk.c > +++ b/tools/kvm/virtio-blk.c > @@ -159,7 +159,9 @@ static void virtio_blk_do_io(struct kvm *kvm, void *param) > while (virt_queue__available(vq)) > virtio_blk_do_io_request(kvm, vq); > > + mutex_lock(&blk_device.mutex); > kvm__irq_line(kvm, VIRTIO_BLK_IRQ, 1); > + mutex_unlock(&blk_device.mutex); Hm, this looks a bit strange (the mutex here protects only a kernel call - that cannot be right) and there's no explanation why it's needed. Why do VIRTIO_BLK_IRQ (== KVM_IRQ_LINE ioctl()) calls have to be covered by the mutex? A short blurb about expected behavior on SMP and locking rules at the top of virtio-blk.c would be nice. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 29, 2011 at 9:44 AM, Ingo Molnar <mingo@elte.hu> wrote: > Hm, this looks a bit strange (the mutex here protects only a kernel call - that > cannot be right) and there's no explanation why it's needed. Why do > VIRTIO_BLK_IRQ (== KVM_IRQ_LINE ioctl()) calls have to be covered by the mutex? > > A short blurb about expected behavior on SMP and locking rules at the top of > virtio-blk.c would be nice. Yes, looks strange. Asias, did you see some bad behavior that this fixes? The per-device mutexes are there to protect device state. The assumption here is that KVM handles KVM_IRQ_LINE ioctl() serialization by titself. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/29/2011 02:55 PM, Pekka Enberg wrote: > On Fri, Apr 29, 2011 at 9:44 AM, Ingo Molnar <mingo@elte.hu> wrote: >> Hm, this looks a bit strange (the mutex here protects only a kernel call - that >> cannot be right) and there's no explanation why it's needed. Why do >> VIRTIO_BLK_IRQ (== KVM_IRQ_LINE ioctl()) calls have to be covered by the mutex? >> >> A short blurb about expected behavior on SMP and locking rules at the top of >> virtio-blk.c would be nice. > > Yes, looks strange. Asias, did you see some bad behavior that this > fixes? Yes. I see random virtio net hangs with scp test. This commit 847838c573f4c04be7547f508a99be2dfdefb4fd triggers this problem as we are using separte irq lines for each virtio devices instead of sharing one irq line for all virtio devices. > The per-device mutexes are there to protect device state. The > assumption here is that KVM handles KVM_IRQ_LINE ioctl() serialization > by titself.
On Fri, Apr 29, 2011 at 10:26 AM, Asias He <asias.hejun@gmail.com> wrote: >> Yes, looks strange. Asias, did you see some bad behavior that this >> fixes? > > Yes. I see random virtio net hangs with scp test. This commit > 847838c573f4c04be7547f508a99be2dfdefb4fd triggers this problem as we are > using separte irq lines for each virtio devices instead of sharing one > irq line for all virtio devices. That's interesting. So reverting that commit makes the hangs go away? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2011-04-29 at 10:55 +0300, Pekka Enberg wrote: > On Fri, Apr 29, 2011 at 10:26 AM, Asias He <asias.hejun@gmail.com> wrote: > >> Yes, looks strange. Asias, did you see some bad behavior that this > >> fixes? > > > > Yes. I see random virtio net hangs with scp test. This commit > > 847838c573f4c04be7547f508a99be2dfdefb4fd triggers this problem as we are > > using separte irq lines for each virtio devices instead of sharing one > > irq line for all virtio devices. > > That's interesting. So reverting that commit makes the hangs go away? I was able to reproduce the hang by running 'ping -f' from the guest to the host and from the host to the guest. From debugging I found that virtio-net's VIRTIO_PCI_QUEUE_NOTIFY doesn't get called once it's hung. The hang does seems to go away once I revert 847838c573f4c04be7547f508a99be2dfdefb4fd .
diff --git a/tools/kvm/virtio-blk.c b/tools/kvm/virtio-blk.c index 3feabd0..ade6335 100644 --- a/tools/kvm/virtio-blk.c +++ b/tools/kvm/virtio-blk.c @@ -159,7 +159,9 @@ static void virtio_blk_do_io(struct kvm *kvm, void *param) while (virt_queue__available(vq)) virtio_blk_do_io_request(kvm, vq); + mutex_lock(&blk_device.mutex); kvm__irq_line(kvm, VIRTIO_BLK_IRQ, 1); + mutex_unlock(&blk_device.mutex); } static bool virtio_blk_pci_io_out(struct kvm *self, uint16_t port, void *data, int size, uint32_t count)
Signed-off-by: Asias He <asias.hejun@gmail.com> --- tools/kvm/virtio-blk.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)