From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56202) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akvx9-0008BG-Nj for qemu-devel@nongnu.org; Tue, 29 Mar 2016 11:53:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1akvx6-0003iB-Fw for qemu-devel@nongnu.org; Tue, 29 Mar 2016 11:53:11 -0400 Received: from e06smtp13.uk.ibm.com ([195.75.94.109]:47070) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akvx6-0003hx-3N for qemu-devel@nongnu.org; Tue, 29 Mar 2016 11:53:08 -0400 Received: from localhost by e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 29 Mar 2016 16:53:06 +0100 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id C54E72190046 for ; Tue, 29 Mar 2016 16:52:43 +0100 (BST) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u2TFr3Nv66191522 for ; Tue, 29 Mar 2016 15:53:03 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u2TFr2Ie015441 for ; Tue, 29 Mar 2016 11:53:03 -0400 References: <1459261027-4398-1-git-send-email-cornelia.huck@de.ibm.com> <1459261027-4398-2-git-send-email-cornelia.huck@de.ibm.com> From: Christian Borntraeger Message-ID: <56FAA4DD.5060600@de.ibm.com> Date: Tue, 29 Mar 2016 17:53:01 +0200 MIME-Version: 1.0 In-Reply-To: <1459261027-4398-2-git-send-email-cornelia.huck@de.ibm.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck , qemu-devel@nongnu.org Cc: tubo@linux.vnet.ibm.com, pbonzini@redhat.com, famz@redhat.com, stefanha@redhat.com, mst@redhat.com On 03/29/2016 04:17 PM, Cornelia Huck wrote: > The ->set_host_notifier() callback is invoked whenever we want to > switch from or to the generic ioeventfd handler. Currently, all > transports deregister the ioeventfd backing and then re-register > it. This opens a race window where we are without ioeventfd > backing for a time period: In the virtio-blk dataplane case, we > observed notifications coming in from both the vcpu thread and > the iothread. > > Let's change pci, mmio and ccw to keep the ioeventfd during > ->set_host_notifier() and only switch the ioeventfd handler. > > Signed-off-by: Cornelia Huck Seems to work fine. Next I will test Michaels patches. > --- > hw/s390x/virtio-ccw.c | 22 +++++++++++++++++----- > hw/virtio/virtio-mmio.c | 27 +++++++++++++++++---------- > hw/virtio/virtio-pci.c | 28 ++++++++++++++++++---------- > include/hw/virtio/virtio-bus.h | 4 ++++ > 4 files changed, 56 insertions(+), 25 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index cb887ba..7b1088e 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -1196,14 +1196,26 @@ static bool virtio_ccw_query_guest_notifiers(DeviceState *d) > static int virtio_ccw_set_host_notifier(DeviceState *d, int n, bool assign) > { > VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); > + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); > + VirtQueue *vq = virtio_get_queue(vdev, n); > > - /* Stop using the generic ioeventfd, we are doing eventfd handling > - * ourselves below */ > - dev->ioeventfd_disabled = assign; > if (assign) { > - virtio_ccw_stop_ioeventfd(dev); > + /* Stop using the generic ioeventfd, we are doing eventfd handling > + * ourselves below */ > + dev->ioeventfd_disabled = true; > + }; > + /* > + * Just switch the handler, don't deassign the ioeventfd. > + * Otherwise, there's a window where we don't have an > + * ioeventfd and we may end up with a notification where > + * we don't expect one. > + */ > + virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); > + if (!assign) { > + /* Use generic ioeventfd handler again. */ > + dev->ioeventfd_disabled = false; > } > - return virtio_ccw_set_guest2host_notifier(dev, n, assign, false); > + return 0; > } > > static int virtio_ccw_get_mappings(VirtioCcwDevice *dev) > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c > index d4cd91f..aafebdf 100644 > --- a/hw/virtio/virtio-mmio.c > +++ b/hw/virtio/virtio-mmio.c > @@ -502,19 +502,26 @@ static int virtio_mmio_set_host_notifier(DeviceState *opaque, int n, > bool assign) > { > VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque); > + VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > + VirtQueue *vq = virtio_get_queue(vdev, n); > > - /* Stop using ioeventfd for virtqueue kick if the device starts using host > - * notifiers. This makes it easy to avoid stepping on each others' toes. > - */ > - proxy->ioeventfd_disabled = assign; > if (assign) { > - virtio_mmio_stop_ioeventfd(proxy); > + /* Stop using the generic ioeventfd, we are doing eventfd handling > + * ourselves below */ > + proxy->ioeventfd_disabled = true; > + }; > + /* > + * Just switch the handler, don't deassign the ioeventfd. > + * Otherwise, there's a window where we don't have an > + * ioeventfd and we may end up with a notification where > + * we don't expect one. > + */ > + virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); > + if (!assign) { > + /* Use generic ioeventfd handler again. */ > + proxy->ioeventfd_disabled = false; > } > - /* We don't need to start here: it's not needed because backend > - * currently only stops on status change away from ok, > - * reset, vmstop and such. If we do add code to start here, > - * need to check vmstate, device state etc. */ > - return virtio_mmio_set_host_notifier_internal(proxy, n, assign, false); > + return 0; > } > > /* virtio-mmio device */ > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 0dadb66..a91c1e8 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1115,18 +1115,26 @@ static int virtio_pci_set_host_notifier(DeviceState *d, int n, bool assign) > { > VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d); > > - /* Stop using ioeventfd for virtqueue kick if the device starts using host > - * notifiers. This makes it easy to avoid stepping on each others' toes. > - */ > - proxy->ioeventfd_disabled = assign; > + VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > + VirtQueue *vq = virtio_get_queue(vdev, n); > + > if (assign) { > - virtio_pci_stop_ioeventfd(proxy); > + /* Stop using the generic ioeventfd, we are doing eventfd handling > + * ourselves below */ > + proxy->ioeventfd_disabled = true; > + }; > + /* > + * Just switch the handler, don't deassign the ioeventfd. > + * Otherwise, there's a window where we don't have an > + * ioeventfd and we may end up with a notification where > + * we don't expect one. > + */ > + virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); > + if (!assign) { > + /* Use generic ioeventfd handler again. */ > + proxy->ioeventfd_disabled = false; > } > - /* We don't need to start here: it's not needed because backend > - * currently only stops on status change away from ok, > - * reset, vmstop and such. If we do add code to start here, > - * need to check vmstate, device state etc. */ > - return virtio_pci_set_host_notifier_internal(proxy, n, assign, false); > + return 0; > } > > static void virtio_pci_vmstate_change(DeviceState *d, bool running) > diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h > index 3f2c136..98c660a 100644 > --- a/include/hw/virtio/virtio-bus.h > +++ b/include/hw/virtio/virtio-bus.h > @@ -52,6 +52,10 @@ typedef struct VirtioBusClass { > bool (*has_extra_state)(DeviceState *d); > bool (*query_guest_notifiers)(DeviceState *d); > int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign); > + /* > + * Switch from/to the generic ioeventfd handler. > + * assigned==false means 'use generic ioeventfd handler'. > + */ > int (*set_host_notifier)(DeviceState *d, int n, bool assigned); > void (*vmstate_change)(DeviceState *d, bool running); > /* >