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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- 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 --]