* [PATCH] virtio: notify virtqueue via host notifier when available @ 2019-10-21 11:40 Stefan Hajnoczi 2019-10-24 3:30 ` Yongji Xie 0 siblings, 1 reply; 3+ messages in thread From: Stefan Hajnoczi @ 2019-10-21 11:40 UTC (permalink / raw) To: qemu-devel; +Cc: Yongji Xie, Michael S. Tsirkin, Stefan Hajnoczi, felipe 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] virtio: notify virtqueue via host notifier when available 2019-10-21 11:40 [PATCH] virtio: notify virtqueue via host notifier when available Stefan Hajnoczi @ 2019-10-24 3:30 ` Yongji Xie 2019-11-05 12:35 ` Stefan Hajnoczi 0 siblings, 1 reply; 3+ messages in thread From: Yongji Xie @ 2019-10-24 3:30 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: Michael S. Tsirkin, qemu-devel, Felipe Franciosi 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] virtio: notify virtqueue via host notifier when available 2019-10-24 3:30 ` Yongji Xie @ 2019-11-05 12:35 ` Stefan Hajnoczi 0 siblings, 0 replies; 3+ messages in thread From: Stefan Hajnoczi @ 2019-11-05 12:35 UTC (permalink / raw) To: Yongji Xie; +Cc: Michael S. Tsirkin, qemu-devel, Felipe Franciosi [-- 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 --] ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-05 12:36 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-21 11:40 [PATCH] virtio: notify virtqueue via host notifier when available Stefan Hajnoczi 2019-10-24 3:30 ` Yongji Xie 2019-11-05 12:35 ` Stefan Hajnoczi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.