From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52270) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alSaB-0007mY-L4 for qemu-devel@nongnu.org; Wed, 30 Mar 2016 22:43:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alSa8-0001le-D2 for qemu-devel@nongnu.org; Wed, 30 Mar 2016 22:43:39 -0400 Received: from e17.ny.us.ibm.com ([129.33.205.207]:42760) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alSa8-0001lY-74 for qemu-devel@nongnu.org; Wed, 30 Mar 2016 22:43:36 -0400 Received: from localhost by e17.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 30 Mar 2016 22:43:35 -0400 Received: from b01cxnp23034.gho.pok.ibm.com (b01cxnp23034.gho.pok.ibm.com [9.57.198.29]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 5A0AD38C803B for ; Wed, 30 Mar 2016 22:43:32 -0400 (EDT) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by b01cxnp23034.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u2V2hVNG36372494 for ; Thu, 31 Mar 2016 02:43:32 GMT Received: from d01av04.pok.ibm.com (localhost [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u2V2hUCd021062 for ; Wed, 30 Mar 2016 22:43:30 -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: tu bo Message-ID: <56FC8ECF.4020102@linux.vnet.ibm.com> Date: Thu, 31 Mar 2016 10:43:27 +0800 MIME-Version: 1.0 In-Reply-To: <1459261027-4398-2-git-send-email-cornelia.huck@de.ibm.com> Content-Type: text/plain; charset=windows-1252; format=flowed 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: pbonzini@redhat.com, famz@redhat.com, borntraeger@de.ibm.com, stefanha@redhat.com, mst@redhat.com Hi Cornelia: I saw qemu crash for this patch on qemu master yesterday. However, scsi disks of my lpar is not available because of s38 firmware update. I'll double check the test when it's ready. thx On 03/29/2016 10: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 > --- > 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); > /* >