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

[Yongji Xie]

On Mon, 21 Oct 2019 at 19:40, Stefan Hajnoczi <address@hidden> 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 <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
> 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