From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47134) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alXEl-0006n1-Jd for qemu-devel@nongnu.org; Thu, 31 Mar 2016 03:41:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alXEg-0001Pu-HO for qemu-devel@nongnu.org; Thu, 31 Mar 2016 03:41:51 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:53950) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alXEg-0001Pp-2i for qemu-devel@nongnu.org; Thu, 31 Mar 2016 03:41:46 -0400 Received: from localhost by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 31 Mar 2016 01:41:45 -0600 Received: from b01cxnp23032.gho.pok.ibm.com (b01cxnp23032.gho.pok.ibm.com [9.57.198.27]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id 823431FF0023 for ; Thu, 31 Mar 2016 01:29:51 -0600 (MDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by b01cxnp23032.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u2V7fg8N31785048 for ; Thu, 31 Mar 2016 07:41:42 GMT Received: from d01av02.pok.ibm.com (localhost [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u2V7ffKw021971 for ; Thu, 31 Mar 2016 03:41:42 -0400 References: <1459261027-4398-1-git-send-email-cornelia.huck@de.ibm.com> <1459261027-4398-2-git-send-email-cornelia.huck@de.ibm.com> <56FC8ECF.4020102@linux.vnet.ibm.com> From: tu bo Message-ID: <56FCD4B2.1030800@linux.vnet.ibm.com> Date: Thu, 31 Mar 2016 15:41:38 +0800 MIME-Version: 1.0 In-Reply-To: <56FC8ECF.4020102@linux.vnet.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: On 03/31/2016 10:43 AM, tu bo wrote: > 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 I still can see crash like below, (gdb) bt #0 blk_aio_read_entry (opaque=0x0) at block/block-backend.c:916 #1 0x000002aa0d6e873e in coroutine_trampoline (i0=, i1=1275094848) at util/coroutine-ucontext.c:78 #2 0x000003ffab4d150a in __makecontext_ret () from /lib64/libc.so.6 (gdb) list 911 static void blk_aio_read_entry(void *opaque) 912 { 913 BlkAioEmAIOCB *acb = opaque; 914 BlkRwCo *rwco = &acb->rwco; 915 916 rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, rwco->qiov->size, 917 rwco->qiov, rwco->flags); 918 blk_aio_complete(acb); 919 } 920 > > 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); >> /* >>