From patchwork Wed Mar 30 12:47:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 8695951 Return-Path: X-Original-To: patchwork-qemu-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 15B8AC0553 for ; Wed, 30 Mar 2016 12:48:26 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D0D922035B for ; Wed, 30 Mar 2016 12:48:23 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 33A8A20279 for ; Wed, 30 Mar 2016 12:48:21 +0000 (UTC) Received: from localhost ([::1]:53943 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alFXo-0008Aq-MT for patchwork-qemu-devel@patchwork.kernel.org; Wed, 30 Mar 2016 08:48:20 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42849) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alFXa-000841-7O for qemu-devel@nongnu.org; Wed, 30 Mar 2016 08:48:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alFXT-0008CF-7x for qemu-devel@nongnu.org; Wed, 30 Mar 2016 08:48:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53379) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alFXS-0008C9-KQ for qemu-devel@nongnu.org; Wed, 30 Mar 2016 08:47:58 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id B1F7F85367; Wed, 30 Mar 2016 12:47:56 +0000 (UTC) Received: from donizetti.redhat.com (ovpn-112-37.ams2.redhat.com [10.36.112.37]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u2UClrkX006079; Wed, 30 Mar 2016 08:47:53 -0400 From: Paolo Bonzini To: qemu-devel@nongnu.org Date: Wed, 30 Mar 2016 14:47:49 +0200 Message-Id: <1459342069-24212-1-git-send-email-pbonzini@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: cornelia.huck@de.ibm.com, borntraeger@de.ibm.com, famz@redhat.com, mst@redhat.com Subject: [Qemu-devel] [PATCH 0/8] virtio: aio handler API X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Since Tu Bo tested Michael's patches successfully, here is my take on them. The only change in Michael's patches is that I handled a failure to start dataplane; however, I am also adding other cleanups (patches 1-2 and 8), two bugfixes (patches 3-4), converting virtio-scsi (patch 7), and finally changing the API to be simpler (patch 8). Paolo Michael S. Tsirkin (2): virtio: add aio handler virtio-blk: use aio handler for data plane Paolo Bonzini (7): virtio-dataplane: pass assign=true to virtio_queue_aio_set_host_notifier_handler virtio: make virtio_queue_notify_vq static virtio-blk: fix disabled mode virtio-scsi: fix disabled mode virtio-scsi: use aio handler for data plane virtio: merge virtio_queue_aio_set_host_notifier_handler with virtio_queue_set_aio virtio: remove starting/stopping checks hw/block/dataplane/virtio-blk.c | 35 +++++++++++---------- hw/block/virtio-blk.c | 29 ++++++++++------- hw/scsi/virtio-scsi-dataplane.c | 56 +++++++++++++++++++++++---------- hw/scsi/virtio-scsi.c | 69 +++++++++++++++++++++++++++-------------- hw/virtio/virtio.c | 37 ++++++++++++++++------ include/hw/virtio/virtio-blk.h | 3 ++ include/hw/virtio/virtio-scsi.h | 9 ++---- include/hw/virtio/virtio.h | 4 +-- 8 files changed, 158 insertions(+), 84 deletions(-) diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index 36f3d2b..0d76110 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -261,7 +261,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) aio_context_acquire(s->ctx); /* Stop notifications for new requests from guest */ - virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, false, false); + virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, false); /* Drain and switch bs back to the QEMU main loop */ blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context()); diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c index 367e476..c57480e 100644 --- a/hw/scsi/virtio-scsi-dataplane.c +++ b/hw/scsi/virtio-scsi-dataplane.c @@ -71,10 +71,10 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s) VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); int i; - virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, false, false); - virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, false, false); + virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, true, false); + virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, true, false); for (i = 0; i < vs->conf.num_queues; i++) { - virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, false, false); + virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, true, false); } } -- 1.8.3.1 From bb6da098bc15ba56d81fb11fc6c00e84fa9069ab Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 29 Mar 2016 16:25:03 +0200 Subject: [PATCH 2/9] virtio: make virtio_queue_notify_vq static Signed-off-by: Paolo Bonzini --- hw/virtio/virtio.c | 2 +- include/hw/virtio/virtio.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 08275a9..07c45b6 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1086,7 +1086,7 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align) virtio_queue_update_rings(vdev, n); } -void virtio_queue_notify_vq(VirtQueue *vq) +static void virtio_queue_notify_vq(VirtQueue *vq) { if (vq->vring.desc && vq->handle_output) { VirtIODevice *vdev = vq->vdev; diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 2b5b248..5afb51c 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -252,7 +252,6 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign, bool set_handler); void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx, bool assign, bool set_handler); -void virtio_queue_notify_vq(VirtQueue *vq); void virtio_irq(VirtQueue *vq); VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector); VirtQueue *virtio_vector_next_queue(VirtQueue *vq); -- 1.8.3.1 From f189e8a8b4ba795f5183d2393613049eeb09c10a Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 30 Mar 2016 12:55:41 +0200 Subject: [PATCH 3/9] virtio-blk: fix disabled mode The missing check on dataplane_disabled caused a segmentation fault in notify_guest_bh, because s->guest_notifier was NULL. Signed-off-by: Paolo Bonzini --- hw/block/dataplane/virtio-blk.c | 7 +++---- hw/block/virtio-blk.c | 2 +- include/hw/virtio/virtio-blk.h | 1 + 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index 0d76110..378feb3 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -28,7 +28,6 @@ struct VirtIOBlockDataPlane { bool starting; bool stopping; - bool disabled; VirtIOBlkConf *conf; @@ -233,7 +232,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) fail_host_notifier: k->set_guest_notifiers(qbus->parent, 1, false); fail_guest_notifiers: - s->disabled = true; + vblk->dataplane_disabled = true; s->starting = false; vblk->dataplane_started = true; } @@ -250,8 +249,8 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) } /* Better luck next time. */ - if (s->disabled) { - s->disabled = false; + if (vblk->dataplane_disabled) { + vblk->dataplane_disabled = false; vblk->dataplane_started = false; return; } diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index d0b8248..77221c1 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -53,7 +53,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status) stb_p(&req->in->status, status); virtqueue_push(s->vq, &req->elem, req->in_len); - if (s->dataplane) { + if (s->dataplane_started && !s->dataplane_disabled) { virtio_blk_data_plane_notify(s->dataplane); } else { virtio_notify(vdev, s->vq); diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index 5cb66cd..073c632 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -53,6 +53,7 @@ typedef struct VirtIOBlock { unsigned short sector_mask; bool original_wce; VMChangeStateEntry *change; + bool dataplane_disabled; bool dataplane_started; int reentrancy_test; struct VirtIOBlockDataPlane *dataplane; -- 1.8.3.1 From 619366c3c83a142a1c5f779d5390e19310616822 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 29 Mar 2016 17:11:19 +0200 Subject: [PATCH 4/9] virtio-scsi: fix disabled mode Add two missing checks for s->dataplane_fenced. In one case, QEMU would skip injecting an IRQ due to a write to an uninitialized EventNotifier's file descriptor. In the second case, the dataplane_disabled field was used by mistake; in fact after fixing this occurrence it is completely unused. Signed-off-by: Paolo Bonzini --- hw/scsi/virtio-scsi.c | 4 ++-- include/hw/virtio/virtio-scsi.h | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 0c30d2e..ac531e2 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -67,7 +67,7 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req) qemu_iovec_from_buf(&req->resp_iov, 0, &req->resp, req->resp_size); virtqueue_push(vq, &req->elem, req->qsgl.size + req->resp_iov.size); - if (s->dataplane_started) { + if (s->dataplane_started && !s->dataplane_fenced) { virtio_scsi_dataplane_notify(vdev, req); } else { virtio_notify(vdev, vq); @@ -772,7 +772,7 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, VirtIOSCSI *s = VIRTIO_SCSI(vdev); SCSIDevice *sd = SCSI_DEVICE(dev); - if (s->ctx && !s->dataplane_disabled) { + if (s->ctx && !s->dataplane_fenced) { VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier; if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) { diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index 209eaa4..eef4e95 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -91,7 +91,6 @@ typedef struct VirtIOSCSI { bool dataplane_started; bool dataplane_starting; bool dataplane_stopping; - bool dataplane_disabled; bool dataplane_fenced; Error *blocker; uint32_t host_features; -- 1.8.3.1 From 46e3cec19b347700fd8683b5177f5d11acd4b875 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 29 Mar 2016 16:42:11 +0300 Subject: [PATCH 5/9] virtio: add aio handler In addition to handling IO in vcpu thread and in io thread, blk dataplane introduces yet another mode: handling it by aio. This reuses the same handler as previous modes, which triggers races as these were not designed to be reentrant. As a temporary fix, add a separate handler just for aio, this will make it possible to disable regular handlers when dataplane is active. Signed-off-by: Michael S. Tsirkin Signed-off-by: Paolo Bonzini --- hw/virtio/virtio.c | 36 ++++++++++++++++++++++++++++++++---- include/hw/virtio/virtio.h | 3 +++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 07c45b6..39a9f47 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -94,6 +94,7 @@ struct VirtQueue uint16_t vector; void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq); + void (*handle_aio_output)(VirtIODevice *vdev, VirtQueue *vq); VirtIODevice *vdev; EventNotifier guest_notifier; EventNotifier host_notifier; @@ -1086,6 +1087,16 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align) virtio_queue_update_rings(vdev, n); } +static void virtio_queue_notify_aio_vq(VirtQueue *vq) +{ + if (vq->vring.desc && vq->handle_aio_output) { + VirtIODevice *vdev = vq->vdev; + + trace_virtio_queue_notify(vdev, vq - vdev->vq, vq); + vq->handle_aio_output(vdev, vq); + } +} + static void virtio_queue_notify_vq(VirtQueue *vq) { if (vq->vring.desc && vq->handle_output) { @@ -1141,10 +1152,19 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, vdev->vq[i].vring.num_default = queue_size; vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN; vdev->vq[i].handle_output = handle_output; + vdev->vq[i].handle_aio_output = NULL; return &vdev->vq[i]; } +void virtio_set_queue_aio(VirtQueue *vq, + void (*handle_output)(VirtIODevice *, VirtQueue *)) +{ + assert(vq->handle_output); + + vq->handle_aio_output = handle_output; +} + void virtio_del_queue(VirtIODevice *vdev, int n) { if (n < 0 || n >= VIRTIO_QUEUE_MAX) { @@ -1778,11 +1798,11 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq) return &vq->guest_notifier; } -static void virtio_queue_host_notifier_read(EventNotifier *n) +static void virtio_queue_host_notifier_aio_read(EventNotifier *n) { VirtQueue *vq = container_of(n, VirtQueue, host_notifier); if (event_notifier_test_and_clear(n)) { - virtio_queue_notify_vq(vq); + virtio_queue_notify_aio_vq(vq); } } @@ -1791,14 +1811,22 @@ void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx, { if (assign && set_handler) { aio_set_event_notifier(ctx, &vq->host_notifier, true, - virtio_queue_host_notifier_read); + virtio_queue_host_notifier_aio_read); } else { aio_set_event_notifier(ctx, &vq->host_notifier, true, NULL); } if (!assign) { /* Test and clear notifier before after disabling event, * in case poll callback didn't have time to run. */ - virtio_queue_host_notifier_read(&vq->host_notifier); + virtio_queue_host_notifier_aio_read(&vq->host_notifier); + } +} + +static void virtio_queue_host_notifier_read(EventNotifier *n) +{ + VirtQueue *vq = container_of(n, VirtQueue, host_notifier); + if (event_notifier_test_and_clear(n)) { + virtio_queue_notify_vq(vq); } } diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 5afb51c..fa3f93b 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -142,6 +142,9 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, void (*handle_output)(VirtIODevice *, VirtQueue *)); +void virtio_set_queue_aio(VirtQueue *vq, + void (*handle_output)(VirtIODevice *, VirtQueue *)); + void virtio_del_queue(VirtIODevice *vdev, int n); void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num); -- 1.8.3.1 From e3a5122c33e3e92d356450db6c89820649ea013d Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 29 Mar 2016 16:42:14 +0300 Subject: [PATCH 6/9] virtio-blk: use aio handler for data plane In addition to handling IO in vcpu thread and in io thread, dataplane introduces yet another mode: handling it by aio. This reuses the same handler as previous modes, which triggers races as these were not designed to be reentrant. Use a separate handler just for aio, and disable regular handlers when dataplane is active. Signed-off-by: Michael S. Tsirkin Signed-off-by: Paolo Bonzini --- hw/block/dataplane/virtio-blk.c | 13 +++++++++++++ hw/block/virtio-blk.c | 27 +++++++++++++++++---------- include/hw/virtio/virtio-blk.h | 2 ++ 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index 378feb3..4a137df 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -183,6 +183,17 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s) g_free(s); } +static void virtio_blk_data_plane_handle_output(VirtIODevice *vdev, + VirtQueue *vq) +{ + VirtIOBlock *s = (VirtIOBlock *)vdev; + + assert(s->dataplane); + assert(s->dataplane_started); + + virtio_blk_handle_vq(s, vq); +} + /* Context: QEMU global mutex held */ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) { @@ -225,6 +236,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) /* Get this show started by hooking up our callbacks */ aio_context_acquire(s->ctx); + virtio_set_queue_aio(s->vq, virtio_blk_data_plane_handle_output); virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true); aio_context_release(s->ctx); return; @@ -261,6 +273,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) /* Stop notifications for new requests from guest */ virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, false); + virtio_set_queue_aio(s->vq, NULL); /* Drain and switch bs back to the QEMU main loop */ blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context()); diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 77221c1..47ad9ed 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -577,20 +577,11 @@ void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) } } -static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) +void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq) { - VirtIOBlock *s = VIRTIO_BLK(vdev); VirtIOBlockReq *req; MultiReqBuffer mrb = {}; - /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start - * dataplane here instead of waiting for .set_status(). - */ - if (s->dataplane && !s->dataplane_started) { - virtio_blk_data_plane_start(s->dataplane); - return; - } - assert(atomic_fetch_inc(&s->reentrancy_test) == 0); blk_io_plug(s->blk); @@ -606,6 +597,22 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) atomic_dec(&s->reentrancy_test); } +static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) +{ + VirtIOBlock *s = (VirtIOBlock *)vdev; + + if (s->dataplane) { + /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start + * dataplane here instead of waiting for .set_status(). + */ + virtio_blk_data_plane_start(s->dataplane); + if (!s->dataplane_disabled) { + return; + } + } + virtio_blk_handle_vq(s, vq); +} + static void virtio_blk_dma_restart_bh(void *opaque) { VirtIOBlock *s = opaque; diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index 073c632..b5117a8 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -87,4 +87,6 @@ void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb); void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb); +void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq); + #endif -- 1.8.3.1 From 711ded78b91de2de793c907a20037304de406a5e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 30 Mar 2016 12:14:23 +0200 Subject: [PATCH 7/9] virtio-scsi: use aio handler for data plane In addition to handling IO in vcpu thread and in io thread, dataplane introduces yet another mode: handling it by aio. This reuses the same handler as previous modes, which triggers races as these were not designed to be reentrant. Use a separate handler just for aio, and disable regular handlers when dataplane is active. Signed-off-by: Paolo Bonzini --- hw/scsi/virtio-scsi-dataplane.c | 43 ++++++++++++++++++++++++--- hw/scsi/virtio-scsi.c | 65 ++++++++++++++++++++++++++++------------- include/hw/virtio/virtio-scsi.h | 6 ++-- 3 files changed, 86 insertions(+), 28 deletions(-) diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c index c57480e..0fa5bb5 100644 --- a/hw/scsi/virtio-scsi-dataplane.c +++ b/hw/scsi/virtio-scsi-dataplane.c @@ -39,7 +39,35 @@ void virtio_scsi_set_iothread(VirtIOSCSI *s, IOThread *iothread) } } -static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n) +static void virtio_scsi_data_plane_handle_cmd(VirtIODevice *vdev, + VirtQueue *vq) +{ + VirtIOSCSI *s = (VirtIOSCSI *)vdev; + + assert(s->ctx && s->dataplane_started); + virtio_scsi_handle_cmd_vq(s, vq); +} + +static void virtio_scsi_data_plane_handle_ctrl(VirtIODevice *vdev, + VirtQueue *vq) +{ + VirtIOSCSI *s = VIRTIO_SCSI(vdev); + + assert(s->ctx && s->dataplane_started); + virtio_scsi_handle_ctrl_vq(s, vq); +} + +static void virtio_scsi_data_plane_handle_event(VirtIODevice *vdev, + VirtQueue *vq) +{ + VirtIOSCSI *s = VIRTIO_SCSI(vdev); + + assert(s->ctx && s->dataplane_started); + virtio_scsi_handle_event_vq(s, vq); +} + +static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n, + void (*fn)(VirtIODevice *vdev, VirtQueue *vq)) { BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s))); VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); @@ -55,6 +83,7 @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n) } virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, true, true); + virtio_set_queue_aio(vq, fn); return 0; } @@ -72,9 +101,12 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s) int i; virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, true, false); + virtio_set_queue_aio(vs->ctrl_vq, NULL); virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, true, false); + virtio_set_queue_aio(vs->event_vq, NULL); for (i = 0; i < vs->conf.num_queues; i++) { virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, true, false); + virtio_set_queue_aio(vs->cmd_vqs[i], NULL); } } @@ -105,16 +137,19 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s) } aio_context_acquire(s->ctx); - rc = virtio_scsi_vring_init(s, vs->ctrl_vq, 0); + rc = virtio_scsi_vring_init(s, vs->ctrl_vq, 0, + virtio_scsi_data_plane_handle_ctrl); if (rc) { goto fail_vrings; } - rc = virtio_scsi_vring_init(s, vs->event_vq, 1); + rc = virtio_scsi_vring_init(s, vs->event_vq, 1, + virtio_scsi_data_plane_handle_event); if (rc) { goto fail_vrings; } for (i = 0; i < vs->conf.num_queues; i++) { - rc = virtio_scsi_vring_init(s, vs->cmd_vqs[i], i + 2); + rc = virtio_scsi_vring_init(s, vs->cmd_vqs[i], i + 2, + virtio_scsi_data_plane_handle_cmd); if (rc) { goto fail_vrings; } diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index ac531e2..7342d26 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -373,7 +373,7 @@ fail: return ret; } -void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req) +static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req) { VirtIODevice *vdev = (VirtIODevice *)s; uint32_t type; @@ -411,20 +411,28 @@ void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req) } } -static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq) +void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq) { - VirtIOSCSI *s = (VirtIOSCSI *)vdev; VirtIOSCSIReq *req; - if (s->ctx && !s->dataplane_started) { - virtio_scsi_dataplane_start(s); - return; - } while ((req = virtio_scsi_pop_req(s, vq))) { virtio_scsi_handle_ctrl_req(s, req); } } +static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq) +{ + VirtIOSCSI *s = (VirtIOSCSI *)vdev; + + if (s->ctx) { + virtio_scsi_dataplane_start(s); + if (!s->dataplane_fenced) { + return; + } + } + virtio_scsi_handle_ctrl_vq(s, vq); +} + static void virtio_scsi_complete_cmd_req(VirtIOSCSIReq *req) { /* Sense data is not in req->resp and is copied separately @@ -507,7 +515,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req) virtio_scsi_complete_cmd_req(req); } -bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req) +static bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req) { VirtIOSCSICommon *vs = &s->parent_obj; SCSIDevice *d; @@ -549,7 +557,7 @@ bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req) return true; } -void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req) +static void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req) { SCSIRequest *sreq = req->sreq; if (scsi_req_enqueue(sreq)) { @@ -559,17 +567,11 @@ void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req) scsi_req_unref(sreq); } -static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq) +void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq) { - /* use non-QOM casts in the data path */ - VirtIOSCSI *s = (VirtIOSCSI *)vdev; VirtIOSCSIReq *req, *next; QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs); - if (s->ctx && !s->dataplane_started) { - virtio_scsi_dataplane_start(s); - return; - } while ((req = virtio_scsi_pop_req(s, vq))) { if (virtio_scsi_handle_cmd_req_prepare(s, req)) { QTAILQ_INSERT_TAIL(&reqs, req, next); @@ -581,6 +583,20 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq) } } +static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq) +{ + /* use non-QOM casts in the data path */ + VirtIOSCSI *s = (VirtIOSCSI *)vdev; + + if (s->ctx) { + virtio_scsi_dataplane_start(s); + if (!s->dataplane_fenced) { + return; + } + } + virtio_scsi_handle_cmd_vq(s, vq); +} + static void virtio_scsi_get_config(VirtIODevice *vdev, uint8_t *config) { @@ -724,17 +740,24 @@ out: } } +void virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq) +{ + if (s->events_dropped) { + virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0); + } +} + static void virtio_scsi_handle_event(VirtIODevice *vdev, VirtQueue *vq) { VirtIOSCSI *s = VIRTIO_SCSI(vdev); - if (s->ctx && !s->dataplane_started) { + if (s->ctx) { virtio_scsi_dataplane_start(s); - return; - } - if (s->events_dropped) { - virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0); + if (!s->dataplane_fenced) { + return; + } } + virtio_scsi_handle_event_vq(s, vq); } static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense) diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index eef4e95..ba2f5ce 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -139,9 +139,9 @@ void virtio_scsi_common_realize(DeviceState *dev, Error **errp, HandleOutput cmd); void virtio_scsi_common_unrealize(DeviceState *dev, Error **errp); -void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req); -bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req); -void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req); +void virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq); +void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq); +void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq); void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req); void virtio_scsi_free_req(VirtIOSCSIReq *req); void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev, -- 1.8.3.1 From 3a065c32bee48ac0e4252d549db6eec7deaa8170 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 29 Mar 2016 16:24:06 +0200 Subject: [PATCH 8/9] virtio: merge virtio_queue_aio_set_host_notifier_handler with virtio_queue_set_aio Eliminating the reentrancy is actually a nice thing that we can do with the API that Michael proposed, so let's make it first class. This also hides the complex assign/set_handler conventions from callers of virtio_queue_aio_set_host_notifier_handler, and fixes a possible race that could happen when passing assign=false to virtio_queue_aio_set_host_notifier_handler. Signed-off-by: Paolo Bonzini --- hw/block/dataplane/virtio-blk.c | 7 +++---- hw/scsi/virtio-scsi-dataplane.c | 12 ++++-------- hw/virtio/virtio.c | 19 ++++--------------- include/hw/virtio/virtio.h | 6 ++---- 4 files changed, 13 insertions(+), 31 deletions(-) diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index 4a137df..7e50d01 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -236,8 +236,8 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) /* Get this show started by hooking up our callbacks */ aio_context_acquire(s->ctx); - virtio_set_queue_aio(s->vq, virtio_blk_data_plane_handle_output); - virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true); + virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, + virtio_blk_data_plane_handle_output); aio_context_release(s->ctx); return; @@ -272,8 +272,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) aio_context_acquire(s->ctx); /* Stop notifications for new requests from guest */ - virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, false); - virtio_set_queue_aio(s->vq, NULL); + virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, NULL); /* Drain and switch bs back to the QEMU main loop */ blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context()); diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c index 0fa5bb5..8694577 100644 --- a/hw/scsi/virtio-scsi-dataplane.c +++ b/hw/scsi/virtio-scsi-dataplane.c @@ -82,8 +82,7 @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n, return rc; } - virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, true, true); - virtio_set_queue_aio(vq, fn); + virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, fn); return 0; } @@ -100,13 +99,10 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s) VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); int i; - virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, true, false); - virtio_set_queue_aio(vs->ctrl_vq, NULL); - virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, true, false); - virtio_set_queue_aio(vs->event_vq, NULL); + virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, NULL); + virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, NULL); for (i = 0; i < vs->conf.num_queues; i++) { - virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, true, false); - virtio_set_queue_aio(vs->cmd_vqs[i], NULL); + virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, NULL); } } diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 39a9f47..b16d2d8 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1157,14 +1157,6 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, return &vdev->vq[i]; } -void virtio_set_queue_aio(VirtQueue *vq, - void (*handle_output)(VirtIODevice *, VirtQueue *)) -{ - assert(vq->handle_output); - - vq->handle_aio_output = handle_output; -} - void virtio_del_queue(VirtIODevice *vdev, int n) { if (n < 0 || n >= VIRTIO_QUEUE_MAX) { @@ -1807,19 +1799,16 @@ static void virtio_queue_host_notifier_aio_read(EventNotifier *n) } void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx, - bool assign, bool set_handler) + void (*handle_output)(VirtIODevice *, + VirtQueue *)) { - if (assign && set_handler) { + vq->handle_aio_output = handle_output; + if (handle_output) { aio_set_event_notifier(ctx, &vq->host_notifier, true, virtio_queue_host_notifier_aio_read); } else { aio_set_event_notifier(ctx, &vq->host_notifier, true, NULL); } - if (!assign) { - /* Test and clear notifier before after disabling event, - * in case poll callback didn't have time to run. */ - virtio_queue_host_notifier_aio_read(&vq->host_notifier); - } } static void virtio_queue_host_notifier_read(EventNotifier *n) diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index fa3f93b..6a37065 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -142,9 +142,6 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, void (*handle_output)(VirtIODevice *, VirtQueue *)); -void virtio_set_queue_aio(VirtQueue *vq, - void (*handle_output)(VirtIODevice *, VirtQueue *)); - void virtio_del_queue(VirtIODevice *vdev, int n); void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num); @@ -254,7 +251,8 @@ EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq); void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign, bool set_handler); void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx, - bool assign, bool set_handler); + void (*fn)(VirtIODevice *, + VirtQueue *)); void virtio_irq(VirtQueue *vq); VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector); VirtQueue *virtio_vector_next_queue(VirtQueue *vq); -- 1.8.3.1 From 8181f63a72e088bc3ad5d1fd5323217662705296 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 30 Mar 2016 12:51:15 +0200 Subject: [PATCH 9/9] virtio: remove starting/stopping checks Reentrancy cannot happen while the BQL is being held. Signed-off-by: Paolo Bonzini --- hw/block/dataplane/virtio-blk.c | 12 ++---------- hw/scsi/virtio-scsi-dataplane.c | 9 +-------- include/hw/virtio/virtio-scsi.h | 2 -- 3 files changed, 3 insertions(+), 20 deletions(-) diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index 7e50d01..0cff427 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -26,9 +26,6 @@ #include "qom/object_interfaces.h" struct VirtIOBlockDataPlane { - bool starting; - bool stopping; - VirtIOBlkConf *conf; VirtIODevice *vdev; @@ -202,11 +199,10 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) VirtIOBlock *vblk = VIRTIO_BLK(s->vdev); int r; - if (vblk->dataplane_started || s->starting) { + if (vblk->dataplane_started) { return; } - s->starting = true; s->vq = virtio_get_queue(s->vdev, 0); /* Set up guest notifier (irq) */ @@ -225,7 +221,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) goto fail_host_notifier; } - s->starting = false; vblk->dataplane_started = true; trace_virtio_blk_data_plane_start(s); @@ -245,7 +240,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) k->set_guest_notifiers(qbus->parent, 1, false); fail_guest_notifiers: vblk->dataplane_disabled = true; - s->starting = false; vblk->dataplane_started = true; } @@ -256,7 +250,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); VirtIOBlock *vblk = VIRTIO_BLK(s->vdev); - if (!vblk->dataplane_started || s->stopping) { + if (!vblk->dataplane_started) { return; } @@ -266,7 +260,6 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) vblk->dataplane_started = false; return; } - s->stopping = true; trace_virtio_blk_data_plane_stop(s); aio_context_acquire(s->ctx); @@ -285,5 +278,4 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) k->set_guest_notifiers(qbus->parent, 1, false); vblk->dataplane_started = false; - s->stopping = false; } diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c index 8694577..ac41787 100644 --- a/hw/scsi/virtio-scsi-dataplane.c +++ b/hw/scsi/virtio-scsi-dataplane.c @@ -116,14 +116,11 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s) VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); if (s->dataplane_started || - s->dataplane_starting || s->dataplane_fenced || s->ctx != iothread_get_aio_context(vs->conf.iothread)) { return; } - s->dataplane_starting = true; - /* Set up guest notifier (irq) */ rc = k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, true); if (rc != 0) { @@ -151,7 +148,6 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s) } } - s->dataplane_starting = false; s->dataplane_started = true; aio_context_release(s->ctx); return; @@ -165,7 +161,6 @@ fail_vrings: k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false); fail_guest_notifiers: s->dataplane_fenced = true; - s->dataplane_starting = false; s->dataplane_started = true; } @@ -177,7 +172,7 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s) VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); int i; - if (!s->dataplane_started || s->dataplane_stopping) { + if (!s->dataplane_started) { return; } @@ -187,7 +182,6 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s) s->dataplane_started = false; return; } - s->dataplane_stopping = true; assert(s->ctx == iothread_get_aio_context(vs->conf.iothread)); aio_context_acquire(s->ctx); @@ -204,6 +198,5 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s) /* Clean up guest notifier (irq) */ k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false); - s->dataplane_stopping = false; s->dataplane_started = false; } diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index ba2f5ce..d5352d8 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -89,8 +89,6 @@ typedef struct VirtIOSCSI { QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers; bool dataplane_started; - bool dataplane_starting; - bool dataplane_stopping; bool dataplane_fenced; Error *blocker; uint32_t host_features;