[PATCH] virtio: notify virtqueue via host notifier when available

[PATCH] virtio: notify virtqueue via host notifier when available

[Stefan Hajnoczi]

Host notifiers are used in several cases:
1. Traditional ioeventfd where virtqueue notifications are handled in
   the main loop thread.
2. IOThreads (aio_handle_output) where virtqueue notifications are
   handled in an IOThread AioContext.
3. vhost where virtqueue notifications are handled by kernel vhost or
   a vhost-user device backend.

Most virtqueue notifications from the guest use the ioeventfd mechanism,
but there are corner cases where QEMU code calls virtio_queue_notify().
This currently honors the host notifier for the IOThreads
aio_handle_output case, but not for the vhost case.  The result is that
vhost does not receive virtqueue notifications from QEMU when
virtio_queue_notify() is called.

This patch extends virtio_queue_notify() to set the host notifier
whenever it is enabled instead of calling the vq->(aio_)handle_output()
function directly.

This fixes the vhost case although it does add a trip through the
eventfd for the traditional ioeventfd case.  I don't think it's worth
adding a fast path for the traditional ioeventfd case because calling
virtio_queue_notify() is rare when ioeventfd is enabled.

Reported-by: Felipe Franciosi <felipe@nutanix.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
Felipe and Yongji: Only tested with "make check".  Please try
vhost-user-scsi/blk and let us know if it fixes the issue.

 include/hw/virtio/virtio-bus.h | 7 +++++++
 hw/virtio/virtio.c             | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 38c9399cd4..28ca51cb4c 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -139,6 +139,13 @@ static inline VirtIODevice *virtio_bus_get_device(VirtioBusState *bus)
 
 /* Return whether the proxy allows ioeventfd.  */
 bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus);
+
+/* Return whether ioeventfd has been started. */
+static inline bool virtio_bus_ioeventfd_started(VirtioBusState *bus)
+{
+    return bus->ioeventfd_started;
+}
+
 /* Start the ioeventfd. */
 int virtio_bus_start_ioeventfd(VirtioBusState *bus);
 /* Stop the ioeventfd. */
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 527df03bfd..abdcec00cd 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1567,6 +1567,8 @@ static void virtio_queue_notify_vq(VirtQueue *vq)
 
 void virtio_queue_notify(VirtIODevice *vdev, int n)
 {
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
+    VirtioBusState *vbus = VIRTIO_BUS(qbus);
     VirtQueue *vq = &vdev->vq[n];
 
     if (unlikely(!vq->vring.desc || vdev->broken)) {
@@ -1574,7 +1576,7 @@ void virtio_queue_notify(VirtIODevice *vdev, int n)
     }
 
     trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
-    if (vq->handle_aio_output) {
+    if (virtio_bus_ioeventfd_started(vbus)) {
         event_notifier_set(&vq->host_notifier);
     } else if (vq->handle_output) {
         vq->handle_output(vdev, vq);
-- 
2.21.0

Re: [PATCH] virtio: notify virtqueue via host notifier when available

[Yongji Xie]

On Mon, 21 Oct 2019 at 19:40, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> Host notifiers are used in several cases:
> 1. Traditional ioeventfd where virtqueue notifications are handled in
>    the main loop thread.
> 2. IOThreads (aio_handle_output) where virtqueue notifications are
>    handled in an IOThread AioContext.
> 3. vhost where virtqueue notifications are handled by kernel vhost or
>    a vhost-user device backend.
>
> Most virtqueue notifications from the guest use the ioeventfd mechanism,
> but there are corner cases where QEMU code calls virtio_queue_notify().
> This currently honors the host notifier for the IOThreads
> aio_handle_output case, but not for the vhost case.  The result is that
> vhost does not receive virtqueue notifications from QEMU when
> virtio_queue_notify() is called.
>
> This patch extends virtio_queue_notify() to set the host notifier
> whenever it is enabled instead of calling the vq->(aio_)handle_output()
> function directly.
>
> This fixes the vhost case although it does add a trip through the
> eventfd for the traditional ioeventfd case.  I don't think it's worth
> adding a fast path for the traditional ioeventfd case because calling
> virtio_queue_notify() is rare when ioeventfd is enabled.
>
> Reported-by: Felipe Franciosi <felipe@nutanix.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> Felipe and Yongji: Only tested with "make check".  Please try
> vhost-user-scsi/blk and let us know if it fixes the issue.
>

I can see the vhost-user-blk issue is fixed by this patch after the
below patch applied:

diff --git a/hw/virtio/vhost-user-blk-pci.c b/hw/virtio/vhost-user-blk-pci.c
index 1dc834a..a32a439 100644
--- a/hw/virtio/vhost-user-blk-pci.c
+++ b/hw/virtio/vhost-user-blk-pci.c
@@ -46,6 +46,8 @@ static Property vhost_user_blk_pci_properties[] = {
     DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
                        DEV_NVECTORS_UNSPECIFIED),
+    DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
+                    VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
 };

>  include/hw/virtio/virtio-bus.h | 7 +++++++
>  hw/virtio/virtio.c             | 4 +++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> index 38c9399cd4..28ca51cb4c 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -139,6 +139,13 @@ static inline VirtIODevice *virtio_bus_get_device(VirtioBusState *bus)
>
>  /* Return whether the proxy allows ioeventfd.  */
>  bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus);
> +
> +/* Return whether ioeventfd has been started. */
> +static inline bool virtio_bus_ioeventfd_started(VirtioBusState *bus)
> +{
> +    return bus->ioeventfd_started;
> +}
> +
>  /* Start the ioeventfd. */
>  int virtio_bus_start_ioeventfd(VirtioBusState *bus);
>  /* Stop the ioeventfd. */
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 527df03bfd..abdcec00cd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1567,6 +1567,8 @@ static void virtio_queue_notify_vq(VirtQueue *vq)
>
>  void virtio_queue_notify(VirtIODevice *vdev, int n)
>  {
> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> +    VirtioBusState *vbus = VIRTIO_BUS(qbus);
>      VirtQueue *vq = &vdev->vq[n];
>
>      if (unlikely(!vq->vring.desc || vdev->broken)) {
> @@ -1574,7 +1576,7 @@ void virtio_queue_notify(VirtIODevice *vdev, int n)
>      }
>
>      trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
> -    if (vq->handle_aio_output) {
> +    if (virtio_bus_ioeventfd_started(vbus)) {

Need to check whether vq->host_notifier is valid or not here.
Otherwise, it could break the ctrl_vq in vhost_net device.

Thanks,
Yongji

Re: [PATCH] virtio: notify virtqueue via host notifier when available

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4316 bytes --]

On Thu, Oct 24, 2019 at 11:30:31AM +0800, Yongji Xie wrote:
> On Mon, 21 Oct 2019 at 19:40, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > Host notifiers are used in several cases:
> > 1. Traditional ioeventfd where virtqueue notifications are handled in
> >    the main loop thread.
> > 2. IOThreads (aio_handle_output) where virtqueue notifications are
> >    handled in an IOThread AioContext.
> > 3. vhost where virtqueue notifications are handled by kernel vhost or
> >    a vhost-user device backend.
> >
> > Most virtqueue notifications from the guest use the ioeventfd mechanism,
> > but there are corner cases where QEMU code calls virtio_queue_notify().
> > This currently honors the host notifier for the IOThreads
> > aio_handle_output case, but not for the vhost case.  The result is that
> > vhost does not receive virtqueue notifications from QEMU when
> > virtio_queue_notify() is called.
> >
> > This patch extends virtio_queue_notify() to set the host notifier
> > whenever it is enabled instead of calling the vq->(aio_)handle_output()
> > function directly.
> >
> > This fixes the vhost case although it does add a trip through the
> > eventfd for the traditional ioeventfd case.  I don't think it's worth
> > adding a fast path for the traditional ioeventfd case because calling
> > virtio_queue_notify() is rare when ioeventfd is enabled.
> >
> > Reported-by: Felipe Franciosi <felipe@nutanix.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > Felipe and Yongji: Only tested with "make check".  Please try
> > vhost-user-scsi/blk and let us know if it fixes the issue.
> >
> 
> I can see the vhost-user-blk issue is fixed by this patch after the
> below patch applied:
> 
> diff --git a/hw/virtio/vhost-user-blk-pci.c b/hw/virtio/vhost-user-blk-pci.c
> index 1dc834a..a32a439 100644
> --- a/hw/virtio/vhost-user-blk-pci.c
> +++ b/hw/virtio/vhost-user-blk-pci.c
> @@ -46,6 +46,8 @@ static Property vhost_user_blk_pci_properties[] = {
>      DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0),
>      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
>                         DEV_NVECTORS_UNSPECIFIED),
> +    DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
> +                    VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> 
> >  include/hw/virtio/virtio-bus.h | 7 +++++++
> >  hw/virtio/virtio.c             | 4 +++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> > index 38c9399cd4..28ca51cb4c 100644
> > --- a/include/hw/virtio/virtio-bus.h
> > +++ b/include/hw/virtio/virtio-bus.h
> > @@ -139,6 +139,13 @@ static inline VirtIODevice *virtio_bus_get_device(VirtioBusState *bus)
> >
> >  /* Return whether the proxy allows ioeventfd.  */
> >  bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus);
> > +
> > +/* Return whether ioeventfd has been started. */
> > +static inline bool virtio_bus_ioeventfd_started(VirtioBusState *bus)
> > +{
> > +    return bus->ioeventfd_started;
> > +}
> > +
> >  /* Start the ioeventfd. */
> >  int virtio_bus_start_ioeventfd(VirtioBusState *bus);
> >  /* Stop the ioeventfd. */
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 527df03bfd..abdcec00cd 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1567,6 +1567,8 @@ static void virtio_queue_notify_vq(VirtQueue *vq)
> >
> >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> >  {
> > +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> > +    VirtioBusState *vbus = VIRTIO_BUS(qbus);
> >      VirtQueue *vq = &vdev->vq[n];
> >
> >      if (unlikely(!vq->vring.desc || vdev->broken)) {
> > @@ -1574,7 +1576,7 @@ void virtio_queue_notify(VirtIODevice *vdev, int n)
> >      }
> >
> >      trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
> > -    if (vq->handle_aio_output) {
> > +    if (virtio_bus_ioeventfd_started(vbus)) {
> 
> Need to check whether vq->host_notifier is valid or not here.
> Otherwise, it could break the ctrl_vq in vhost_net device.

Good points, thanks!  I will send v2 that works for all vhost-user
devices (including blk, scsi, and net).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]