| Message ID | 1452586854-21058-1-git-send-email-victork@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/12/2016 09:26 AM, Victor Kaplansky wrote: > This RFC PATCH tries to solve the problem of lost interrupts > from a slow back-end. Didier could you test it? > > Thanks, Victor > > When interrupts are unmasked, it could take some undefined time > to the back-end to start routing events to guest_notifier. Till > that the events will continue flow to masked_notifier, and some > interrupts could be lost. > > This patch tries to handle the above situation by testing and > cleaning both masked_notifier and guest_notifier in > guest_notifier read handler. > > Signed-off-by: Victor Kaplansky <victork@redhat.com> > --- > include/hw/virtio/virtio.h | 1 + > hw/virtio/vhost.c | 3 +++ > hw/virtio/virtio.c | 14 ++++++++++++++ > 3 files changed, 18 insertions(+) > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 205fadf2..f52b0b6a 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -240,6 +240,7 @@ VirtQueue *virtio_get_queue(VirtIODevice *vdev, int n); > uint16_t virtio_get_queue_index(VirtQueue *vq); > int virtio_queue_get_id(VirtQueue *vq); > EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq); > +void virtio_queue_set_masked_guest_notifier(VirtQueue *vq, EventNotifier *n); > void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign, > bool with_irqfd); > EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq); > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index de29968a..51ce1532 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -854,6 +854,9 @@ static int vhost_virtqueue_start(struct vhost_dev *dev, > /* Clear and discard previous events if any. */ > event_notifier_test_and_clear(&vq->masked_notifier); > > + /* Set masked guest_notifier. */ > + virtio_queue_set_masked_guest_notifier(vvq, &vq->masked_notifier); > + > return 0; > > fail_kick: > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index bd6b4df9..d9095c51 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -89,6 +89,7 @@ struct VirtQueue > VirtIODevice *vdev; > EventNotifier guest_notifier; > EventNotifier host_notifier; > + EventNotifier *masked_guest_notifier; > QLIST_ENTRY(VirtQueue) node; > }; > > @@ -1622,6 +1623,14 @@ static void virtio_queue_guest_notifier_read(EventNotifier *n) > if (event_notifier_test_and_clear(n)) { > virtio_irq(vq); > } > + /* It could take some time to the backend to switch to > + * sending to unmasked evenfd, so we have to test masked > + * notifier too. */ > + if (vq->masked_guest_notifier) { > + if (event_notifier_test_and_clear(vq->masked_guest_notifier)) { > + virtio_irq(vq); > + } > + } > } > > void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign, > @@ -1645,6 +1654,11 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq) > return &vq->guest_notifier; > } > > +void virtio_queue_set_masked_guest_notifier(VirtQueue *vq, EventNotifier *n) > +{ > + vq->masked_guest_notifier = n; > +} > + > static void virtio_queue_host_notifier_read(EventNotifier *n) > { > VirtQueue *vq = container_of(n, VirtQueue, host_notifier); Hi viktor, i'm wondering how this patch works. virtio_queue_guest_notifier_read is only used in virtio_queue_set_guest_notifier_fd_handler. and it is only used if with_irq is not set: if (assign && !with_irqfd) { event_notifier_set_handler(&vq->guest_notifier, virtio_queue_guest_notifier_read); } else { event_notifier_set_handler(&vq->guest_notifier, NULL); } else null handler is set in guest_notifier. And from my understanding, virtio-pci in kvm mode uses irqfd, so when are we entering the virtio_queue_guest_notifier_read? did you also change the qemu configuration? thanks didier
On Tue, Jan 12, 2016 at 01:05:52PM +0100, Didier Pallard wrote: > On 01/12/2016 09:26 AM, Victor Kaplansky wrote: > >This RFC PATCH tries to solve the problem of lost interrupts > >from a slow back-end. Didier could you test it? > > > >Thanks, Victor > > > >When interrupts are unmasked, it could take some undefined time > >to the back-end to start routing events to guest_notifier. Till > >that the events will continue flow to masked_notifier, and some > >interrupts could be lost. > > > >This patch tries to handle the above situation by testing and > >cleaning both masked_notifier and guest_notifier in > >guest_notifier read handler. > > > >Signed-off-by: Victor Kaplansky <victork@redhat.com> > >--- > > include/hw/virtio/virtio.h | 1 + > > hw/virtio/vhost.c | 3 +++ > > hw/virtio/virtio.c | 14 ++++++++++++++ > > 3 files changed, 18 insertions(+) > > > >diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > >index 205fadf2..f52b0b6a 100644 > >--- a/include/hw/virtio/virtio.h > >+++ b/include/hw/virtio/virtio.h > >@@ -240,6 +240,7 @@ VirtQueue *virtio_get_queue(VirtIODevice *vdev, int n); > > uint16_t virtio_get_queue_index(VirtQueue *vq); > > int virtio_queue_get_id(VirtQueue *vq); > > EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq); > >+void virtio_queue_set_masked_guest_notifier(VirtQueue *vq, EventNotifier *n); > > void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign, > > bool with_irqfd); > > EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq); > >diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > >index de29968a..51ce1532 100644 > >--- a/hw/virtio/vhost.c > >+++ b/hw/virtio/vhost.c > >@@ -854,6 +854,9 @@ static int vhost_virtqueue_start(struct vhost_dev *dev, > > /* Clear and discard previous events if any. */ > > event_notifier_test_and_clear(&vq->masked_notifier); > >+ /* Set masked guest_notifier. */ > >+ virtio_queue_set_masked_guest_notifier(vvq, &vq->masked_notifier); > >+ > > return 0; > > fail_kick: > >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >index bd6b4df9..d9095c51 100644 > >--- a/hw/virtio/virtio.c > >+++ b/hw/virtio/virtio.c > >@@ -89,6 +89,7 @@ struct VirtQueue > > VirtIODevice *vdev; > > EventNotifier guest_notifier; > > EventNotifier host_notifier; > >+ EventNotifier *masked_guest_notifier; > > QLIST_ENTRY(VirtQueue) node; > > }; > >@@ -1622,6 +1623,14 @@ static void virtio_queue_guest_notifier_read(EventNotifier *n) > > if (event_notifier_test_and_clear(n)) { > > virtio_irq(vq); > > } > >+ /* It could take some time to the backend to switch to > >+ * sending to unmasked evenfd, so we have to test masked > >+ * notifier too. */ > >+ if (vq->masked_guest_notifier) { > >+ if (event_notifier_test_and_clear(vq->masked_guest_notifier)) { > >+ virtio_irq(vq); > >+ } > >+ } > > } > > void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign, > >@@ -1645,6 +1654,11 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq) > > return &vq->guest_notifier; > > } > >+void virtio_queue_set_masked_guest_notifier(VirtQueue *vq, EventNotifier *n) > >+{ > >+ vq->masked_guest_notifier = n; > >+} > >+ > > static void virtio_queue_host_notifier_read(EventNotifier *n) > > { > > VirtQueue *vq = container_of(n, VirtQueue, host_notifier); > > Hi viktor, > > i'm wondering how this patch works. > virtio_queue_guest_notifier_read is only used in > virtio_queue_set_guest_notifier_fd_handler. > and it is only used if with_irq is not set: > if (assign && !with_irqfd) { > event_notifier_set_handler(&vq->guest_notifier, > virtio_queue_guest_notifier_read); > } else { > event_notifier_set_handler(&vq->guest_notifier, NULL); > } > else null handler is set in guest_notifier. > And from my understanding, virtio-pci in kvm mode uses irqfd, so when are > we entering the virtio_queue_guest_notifier_read? > did you also change the qemu configuration? > Hmm, right. So, probably it would be better to take your version of the fix as a temporarily solution which just disables the ability to mask interrupts when virtio-net-pci is backed by vhost-user. > thanks > didier > > -- > Didier PALLARD > 6WIND > Software Engineer > > Tel: +33 1 39 30 92 46 > Mob: +33 6 49 11 40 14 > Fax: +33 1 39 30 92 11 > didier.pallard@6wind.com > www.6wind.com > >
On 01/13/2016 04:32 PM, Victor Kaplansky wrote: > On Tue, Jan 12, 2016 at 01:05:52PM +0100, Didier Pallard wrote: >> On 01/12/2016 09:26 AM, Victor Kaplansky wrote: >>> This RFC PATCH tries to solve the problem of lost interrupts >> >from a slow back-end. Didier could you test it? >>> Thanks, Victor >>> >>> When interrupts are unmasked, it could take some undefined time >>> to the back-end to start routing events to guest_notifier. Till >>> that the events will continue flow to masked_notifier, and some >>> interrupts could be lost. >>> >>> This patch tries to handle the above situation by testing and >>> cleaning both masked_notifier and guest_notifier in >>> guest_notifier read handler. >>> >>> Signed-off-by: Victor Kaplansky <victork@redhat.com> >>> --- >>> include/hw/virtio/virtio.h | 1 + >>> hw/virtio/vhost.c | 3 +++ >>> hw/virtio/virtio.c | 14 ++++++++++++++ >>> 3 files changed, 18 insertions(+) >>> >>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >>> index 205fadf2..f52b0b6a 100644 >>> --- a/include/hw/virtio/virtio.h >>> +++ b/include/hw/virtio/virtio.h >>> @@ -240,6 +240,7 @@ VirtQueue *virtio_get_queue(VirtIODevice *vdev, int n); >>> uint16_t virtio_get_queue_index(VirtQueue *vq); >>> int virtio_queue_get_id(VirtQueue *vq); >>> EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq); >>> +void virtio_queue_set_masked_guest_notifier(VirtQueue *vq, EventNotifier *n); >>> void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign, >>> bool with_irqfd); >>> EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq); >>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>> index de29968a..51ce1532 100644 >>> --- a/hw/virtio/vhost.c >>> +++ b/hw/virtio/vhost.c >>> @@ -854,6 +854,9 @@ static int vhost_virtqueue_start(struct vhost_dev *dev, >>> /* Clear and discard previous events if any. */ >>> event_notifier_test_and_clear(&vq->masked_notifier); >>> + /* Set masked guest_notifier. */ >>> + virtio_queue_set_masked_guest_notifier(vvq, &vq->masked_notifier); >>> + >>> return 0; >>> fail_kick: >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>> index bd6b4df9..d9095c51 100644 >>> --- a/hw/virtio/virtio.c >>> +++ b/hw/virtio/virtio.c >>> @@ -89,6 +89,7 @@ struct VirtQueue >>> VirtIODevice *vdev; >>> EventNotifier guest_notifier; >>> EventNotifier host_notifier; >>> + EventNotifier *masked_guest_notifier; >>> QLIST_ENTRY(VirtQueue) node; >>> }; >>> @@ -1622,6 +1623,14 @@ static void virtio_queue_guest_notifier_read(EventNotifier *n) >>> if (event_notifier_test_and_clear(n)) { >>> virtio_irq(vq); >>> } >>> + /* It could take some time to the backend to switch to >>> + * sending to unmasked evenfd, so we have to test masked >>> + * notifier too. */ >>> + if (vq->masked_guest_notifier) { >>> + if (event_notifier_test_and_clear(vq->masked_guest_notifier)) { >>> + virtio_irq(vq); >>> + } >>> + } >>> } >>> void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign, >>> @@ -1645,6 +1654,11 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq) >>> return &vq->guest_notifier; >>> } >>> +void virtio_queue_set_masked_guest_notifier(VirtQueue *vq, EventNotifier *n) >>> +{ >>> + vq->masked_guest_notifier = n; >>> +} >>> + >>> static void virtio_queue_host_notifier_read(EventNotifier *n) >>> { >>> VirtQueue *vq = container_of(n, VirtQueue, host_notifier); >> Hi viktor, >> >> i'm wondering how this patch works. >> virtio_queue_guest_notifier_read is only used in >> virtio_queue_set_guest_notifier_fd_handler. >> and it is only used if with_irq is not set: >> if (assign && !with_irqfd) { >> event_notifier_set_handler(&vq->guest_notifier, >> virtio_queue_guest_notifier_read); >> } else { >> event_notifier_set_handler(&vq->guest_notifier, NULL); >> } >> else null handler is set in guest_notifier. >> And from my understanding, virtio-pci in kvm mode uses irqfd, so when are >> we entering the virtio_queue_guest_notifier_read? >> did you also change the qemu configuration? >> > Hmm, right. So, probably it would be better to take your version > of the fix as a temporarily solution which just disables the > ability to mask interrupts when virtio-net-pci is backed by > vhost-user. > > Well, it does not completely disable the ability to mask interrupts: interrupt masking is directly done by qemu (that set/unset eventfd in kvm to unmask/mask interrupts) rather than by vhost-user backend. This allows to be sure that interrupts are correctly masked[unmasked] on return of virtio_pci_vq_vector_mask[unmask] function, (which is not the case when a message is sent through the vhost-user linux socket) but my patch was only tested with a single platform configuration. I don't know if it behaves well with non pci buses, for example.
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 205fadf2..f52b0b6a 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -240,6 +240,7 @@ VirtQueue *virtio_get_queue(VirtIODevice *vdev, int n); uint16_t virtio_get_queue_index(VirtQueue *vq); int virtio_queue_get_id(VirtQueue *vq); EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq); +void virtio_queue_set_masked_guest_notifier(VirtQueue *vq, EventNotifier *n); void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign, bool with_irqfd); EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq); diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index de29968a..51ce1532 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -854,6 +854,9 @@ static int vhost_virtqueue_start(struct vhost_dev *dev, /* Clear and discard previous events if any. */ event_notifier_test_and_clear(&vq->masked_notifier); + /* Set masked guest_notifier. */ + virtio_queue_set_masked_guest_notifier(vvq, &vq->masked_notifier); + return 0; fail_kick: diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index bd6b4df9..d9095c51 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -89,6 +89,7 @@ struct VirtQueue VirtIODevice *vdev; EventNotifier guest_notifier; EventNotifier host_notifier; + EventNotifier *masked_guest_notifier; QLIST_ENTRY(VirtQueue) node; }; @@ -1622,6 +1623,14 @@ static void virtio_queue_guest_notifier_read(EventNotifier *n) if (event_notifier_test_and_clear(n)) { virtio_irq(vq); } + /* It could take some time to the backend to switch to + * sending to unmasked evenfd, so we have to test masked + * notifier too. */ + if (vq->masked_guest_notifier) { + if (event_notifier_test_and_clear(vq->masked_guest_notifier)) { + virtio_irq(vq); + } + } } void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign, @@ -1645,6 +1654,11 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq) return &vq->guest_notifier; } +void virtio_queue_set_masked_guest_notifier(VirtQueue *vq, EventNotifier *n) +{ + vq->masked_guest_notifier = n; +} + static void virtio_queue_host_notifier_read(EventNotifier *n) { VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
This RFC PATCH tries to solve the problem of lost interrupts from a slow back-end. Didier could you test it? Thanks, Victor When interrupts are unmasked, it could take some undefined time to the back-end to start routing events to guest_notifier. Till that the events will continue flow to masked_notifier, and some interrupts could be lost. This patch tries to handle the above situation by testing and cleaning both masked_notifier and guest_notifier in guest_notifier read handler. Signed-off-by: Victor Kaplansky <victork@redhat.com> --- include/hw/virtio/virtio.h | 1 + hw/virtio/vhost.c | 3 +++ hw/virtio/virtio.c | 14 ++++++++++++++ 3 files changed, 18 insertions(+)