[Qemu-devel] [PULL v2 16/23] virtio-scsi: do not use vring in dataplane

[Michael S. Tsirkin]

From: Paolo Bonzini <address@hidden>

Signed-off-by: Paolo Bonzini <address@hidden>
Reviewed-by: Michael S. Tsirkin <address@hidden>
Signed-off-by: Michael S. Tsirkin <address@hidden>
Acked-by: Stefan Hajnoczi <address@hidden>
---
 include/hw/virtio/virtio-scsi.h |  21 +----
 hw/scsi/virtio-scsi-dataplane.c | 196 +++++-----------------------------------
 hw/scsi/virtio-scsi.c           |  52 ++---------
 3 files changed, 35 insertions(+), 234 deletions(-)

diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index a8029aa..209eaa4 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -22,7 +22,6 @@
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
 #include "sysemu/iothread.h"
-#include "hw/virtio/dataplane/vring.h"
 
 #define TYPE_VIRTIO_SCSI_COMMON "virtio-scsi-common"
 #define VIRTIO_SCSI_COMMON(obj) \
@@ -58,13 +57,6 @@ struct VirtIOSCSIConf {
 
 struct VirtIOSCSI;
 
-typedef struct {
-    struct VirtIOSCSI *parent;
-    Vring vring;
-    EventNotifier host_notifier;
-    EventNotifier guest_notifier;
-} VirtIOSCSIVring;
-
 typedef struct VirtIOSCSICommon {
     VirtIODevice parent_obj;
     VirtIOSCSIConf conf;
@@ -96,18 +88,12 @@ typedef struct VirtIOSCSI {
     QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) insert_notifiers;
     QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers;
 
-    /* Vring is used instead of vq in dataplane code, because of the underlying
-     * memory layer thread safety */
-    VirtIOSCSIVring *ctrl_vring;
-    VirtIOSCSIVring *event_vring;
-    VirtIOSCSIVring **cmd_vrings;
     bool dataplane_started;
     bool dataplane_starting;
     bool dataplane_stopping;
     bool dataplane_disabled;
     bool dataplane_fenced;
     Error *blocker;
-    Notifier migration_state_notifier;
     uint32_t host_features;
 } VirtIOSCSI;
 
@@ -123,9 +109,6 @@ typedef struct VirtIOSCSIReq {
     QEMUSGList qsgl;
     QEMUIOVector resp_iov;
 
-    /* Set by dataplane code. */
-    VirtIOSCSIVring *vring;
-
     union {
         /* Used for two-stage request submission */
         QTAILQ_ENTRY(VirtIOSCSIReq) next;
@@ -168,8 +151,6 @@ void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
 void virtio_scsi_set_iothread(VirtIOSCSI *s, IOThread *iothread);
 void virtio_scsi_dataplane_start(VirtIOSCSI *s);
 void virtio_scsi_dataplane_stop(VirtIOSCSI *s);
-void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req);
-VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
-                                         VirtIOSCSIVring *vring);
+void virtio_scsi_dataplane_notify(VirtIODevice *vdev, VirtIOSCSIReq *req);
 
 #endif /* _QEMU_VIRTIO_SCSI_H */
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 8340326..367e476 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -39,14 +39,10 @@ void virtio_scsi_set_iothread(VirtIOSCSI *s, IOThread 
*iothread)
     }
 }
 
-static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
-                                               VirtQueue *vq,
-                                               EventNotifierHandler *handler,
-                                               int n)
+static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n)
 {
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    VirtIOSCSIVring *r;
     int rc;
 
     /* Set up virtqueue notify */
@@ -55,105 +51,17 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI 
*s,
         fprintf(stderr, "virtio-scsi: Failed to set host notifier (%d)\n",
                 rc);
         s->dataplane_fenced = true;
-        return NULL;
+        return rc;
     }
 
-    r = g_new(VirtIOSCSIVring, 1);
-    r->host_notifier = *virtio_queue_get_host_notifier(vq);
-    r->guest_notifier = *virtio_queue_get_guest_notifier(vq);
-    aio_set_event_notifier(s->ctx, &r->host_notifier, true, handler);
-
-    r->parent = s;
-
-    if (!vring_setup(&r->vring, VIRTIO_DEVICE(s), n)) {
-        fprintf(stderr, "virtio-scsi: VRing setup failed\n");
-        goto fail_vring;
-    }
-    return r;
-
-fail_vring:
-    aio_set_event_notifier(s->ctx, &r->host_notifier, true, NULL);
-    k->set_host_notifier(qbus->parent, n, false);
-    g_free(r);
-    return NULL;
-}
-
-VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
-                                         VirtIOSCSIVring *vring)
-{
-    VirtIOSCSICommon *vs = (VirtIOSCSICommon *)s;
-    VirtIOSCSIReq *req;
-
-    req = vring_pop((VirtIODevice *)s, &vring->vring,
-                    sizeof(VirtIOSCSIReq) + vs->cdb_size);
-    if (!req) {
-        return NULL;
-    }
-    virtio_scsi_init_req(s, NULL, req);
-    req->vring = vring;
-    return req;
-}
-
-void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(req->vring->parent);
-
-    vring_push(vdev, &req->vring->vring, &req->elem,
-               req->qsgl.size + req->resp_iov.size);
-
-    if (vring_should_notify(vdev, &req->vring->vring)) {
-        event_notifier_set(&req->vring->guest_notifier);
-    }
+    virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, true, true);
+    return 0;
 }
 
-static void virtio_scsi_iothread_handle_ctrl(EventNotifier *notifier)
+void virtio_scsi_dataplane_notify(VirtIODevice *vdev, VirtIOSCSIReq *req)
 {
-    VirtIOSCSIVring *vring = container_of(notifier,
-                                          VirtIOSCSIVring, host_notifier);
-    VirtIOSCSI *s = VIRTIO_SCSI(vring->parent);
-    VirtIOSCSIReq *req;
-
-    event_notifier_test_and_clear(notifier);
-    while ((req = virtio_scsi_pop_req_vring(s, vring))) {
-        virtio_scsi_handle_ctrl_req(s, req);
-    }
-}
-
-static void virtio_scsi_iothread_handle_event(EventNotifier *notifier)
-{
-    VirtIOSCSIVring *vring = container_of(notifier,
-                                          VirtIOSCSIVring, host_notifier);
-    VirtIOSCSI *s = vring->parent;
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-
-    event_notifier_test_and_clear(notifier);
-
-    if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
-        return;
-    }
-
-    if (s->events_dropped) {
-        virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0);
-    }
-}
-
-static void virtio_scsi_iothread_handle_cmd(EventNotifier *notifier)
-{
-    VirtIOSCSIVring *vring = container_of(notifier,
-                                          VirtIOSCSIVring, host_notifier);
-    VirtIOSCSI *s = (VirtIOSCSI *)vring->parent;
-    VirtIOSCSIReq *req, *next;
-    QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
-
-    event_notifier_test_and_clear(notifier);
-    while ((req = virtio_scsi_pop_req_vring(s, vring))) {
-        if (virtio_scsi_handle_cmd_req_prepare(s, req)) {
-            QTAILQ_INSERT_TAIL(&reqs, req, next);
-        }
-    }
-
-    QTAILQ_FOREACH_SAFE(req, &reqs, next, next) {
-        virtio_scsi_handle_cmd_req_submit(s, req);
+    if (virtio_should_notify(vdev, req->vq)) {
+        event_notifier_set(virtio_queue_get_guest_notifier(req->vq));
     }
 }
 
@@ -163,46 +71,10 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
     int i;
 
-    if (s->ctrl_vring) {
-        aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier,
-                               true, NULL);
-    }
-    if (s->event_vring) {
-        aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier,
-                               true, NULL);
-    }
-    if (s->cmd_vrings) {
-        for (i = 0; i < vs->conf.num_queues && s->cmd_vrings[i]; i++) {
-            aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier,
-                                   true, NULL);
-        }
-    }
-}
-
-static void virtio_scsi_vring_teardown(VirtIOSCSI *s)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
-    int i;
-
-    if (s->ctrl_vring) {
-        vring_teardown(&s->ctrl_vring->vring, vdev, 0);
-        g_free(s->ctrl_vring);
-        s->ctrl_vring = NULL;
-    }
-    if (s->event_vring) {
-        vring_teardown(&s->event_vring->vring, vdev, 1);
-        g_free(s->event_vring);
-        s->event_vring = NULL;
-    }
-    if (s->cmd_vrings) {
-        for (i = 0; i < vs->conf.num_queues && s->cmd_vrings[i]; i++) {
-            vring_teardown(&s->cmd_vrings[i]->vring, vdev, 2 + i);
-            g_free(s->cmd_vrings[i]);
-            s->cmd_vrings[i] = NULL;
-        }
-        free(s->cmd_vrings);
-        s->cmd_vrings = NULL;
+    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);
+    for (i = 0; i < vs->conf.num_queues; i++) {
+        virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, 
false, false);
     }
 }
 
@@ -229,30 +101,21 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
     if (rc != 0) {
         fprintf(stderr, "virtio-scsi: Failed to set guest notifiers (%d), "
                 "ensure -enable-kvm is set\n", rc);
-        s->dataplane_fenced = true;
         goto fail_guest_notifiers;
     }
 
     aio_context_acquire(s->ctx);
-    s->ctrl_vring = virtio_scsi_vring_init(s, vs->ctrl_vq,
-                                           virtio_scsi_iothread_handle_ctrl,
-                                           0);
-    if (!s->ctrl_vring) {
+    rc = virtio_scsi_vring_init(s, vs->ctrl_vq, 0);
+    if (rc) {
         goto fail_vrings;
     }
-    s->event_vring = virtio_scsi_vring_init(s, vs->event_vq,
-                                            virtio_scsi_iothread_handle_event,
-                                            1);
-    if (!s->event_vring) {
+    rc = virtio_scsi_vring_init(s, vs->event_vq, 1);
+    if (rc) {
         goto fail_vrings;
     }
-    s->cmd_vrings = g_new(VirtIOSCSIVring *, vs->conf.num_queues);
     for (i = 0; i < vs->conf.num_queues; i++) {
-        s->cmd_vrings[i] =
-            virtio_scsi_vring_init(s, vs->cmd_vqs[i],
-                                   virtio_scsi_iothread_handle_cmd,
-                                   i + 2);
-        if (!s->cmd_vrings[i]) {
+        rc = virtio_scsi_vring_init(s, vs->cmd_vqs[i], i + 2);
+        if (rc) {
             goto fail_vrings;
         }
     }
@@ -265,13 +128,14 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
 fail_vrings:
     virtio_scsi_clear_aio(s);
     aio_context_release(s->ctx);
-    virtio_scsi_vring_teardown(s);
     for (i = 0; i < vs->conf.num_queues + 2; i++) {
         k->set_host_notifier(qbus->parent, i, false);
     }
     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;
 }
 
 /* Context: QEMU global mutex held */
@@ -282,12 +146,14 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
     int i;
 
+    if (!s->dataplane_started || s->dataplane_stopping) {
+        return;
+    }
+
     /* Better luck next time. */
     if (s->dataplane_fenced) {
         s->dataplane_fenced = false;
-        return;
-    }
-    if (!s->dataplane_started || s->dataplane_stopping) {
+        s->dataplane_started = false;
         return;
     }
     s->dataplane_stopping = true;
@@ -295,24 +161,12 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
 
     aio_context_acquire(s->ctx);
 
-    aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier,
-                           true, NULL);
-    aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier,
-                           true, NULL);
-    for (i = 0; i < vs->conf.num_queues; i++) {
-        aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier,
-                               true, NULL);
-    }
+    virtio_scsi_clear_aio(s);
 
     blk_drain_all(); /* ensure there are no in-flight requests */
 
     aio_context_release(s->ctx);
 
-    /* Sync vring state back to virtqueue so that non-dataplane request
-     * processing can continue when we disable the host notifier below.
-     */
-    virtio_scsi_vring_teardown(s);
-
     for (i = 0; i < vs->conf.num_queues + 2; i++) {
         k->set_host_notifier(qbus->parent, i, false);
     }
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 5b29bac..0c30d2e 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -23,7 +23,6 @@
 #include <block/scsi.h>
 #include <hw/virtio/virtio-bus.h>
 #include "hw/virtio/virtio-access.h"
-#include "migration/migration.h"
 
 static inline int virtio_scsi_get_lun(uint8_t *lun)
 {
@@ -43,7 +42,8 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI 
*s, uint8_t *lun)
 
 void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
 {
-    const size_t zero_skip = offsetof(VirtIOSCSIReq, vring);
+    const size_t zero_skip =
+        offsetof(VirtIOSCSIReq, resp_iov) + sizeof(req->resp_iov);
 
     req->vq = vq;
     req->dev = s;
@@ -66,11 +66,10 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
     qemu_iovec_from_buf(&req->resp_iov, 0, &req->resp, req->resp_size);
-    if (req->vring) {
-        assert(req->vq == NULL);
-        virtio_scsi_vring_push_notify(req);
+    virtqueue_push(vq, &req->elem, req->qsgl.size + req->resp_iov.size);
+    if (s->dataplane_started) {
+        virtio_scsi_dataplane_notify(vdev, req);
     } else {
-        virtqueue_push(vq, &req->elem, req->qsgl.size + req->resp_iov.size);
         virtio_notify(vdev, vq);
     }
 
@@ -417,7 +416,7 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, 
VirtQueue *vq)
     VirtIOSCSI *s = (VirtIOSCSI *)vdev;
     VirtIOSCSIReq *req;
 
-    if (s->ctx && !s->dataplane_disabled) {
+    if (s->ctx && !s->dataplane_started) {
         virtio_scsi_dataplane_start(s);
         return;
     }
@@ -567,7 +566,7 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, 
VirtQueue *vq)
     VirtIOSCSIReq *req, *next;
     QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
 
-    if (s->ctx && !s->dataplane_disabled) {
+    if (s->ctx && !s->dataplane_started) {
         virtio_scsi_dataplane_start(s);
         return;
     }
@@ -687,11 +686,7 @@ void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
         aio_context_acquire(s->ctx);
     }
 
-    if (s->dataplane_started) {
-        req = virtio_scsi_pop_req_vring(s, s->event_vring);
-    } else {
-        req = virtio_scsi_pop_req(s, vs->event_vq);
-    }
+    req = virtio_scsi_pop_req(s, vs->event_vq);
     if (!req) {
         s->events_dropped = true;
         goto out;
@@ -733,7 +728,7 @@ static void virtio_scsi_handle_event(VirtIODevice *vdev, 
VirtQueue *vq)
 {
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
-    if (s->ctx && !s->dataplane_disabled) {
+    if (s->ctx && !s->dataplane_started) {
         virtio_scsi_dataplane_start(s);
         return;
     }
@@ -901,31 +896,6 @@ void virtio_scsi_common_realize(DeviceState *dev, Error 
**errp,
     }
 }
 
-/* Disable dataplane thread during live migration since it does not
- * update the dirty memory bitmap yet.
- */
-static void virtio_scsi_migration_state_changed(Notifier *notifier, void *data)
-{
-    VirtIOSCSI *s = container_of(notifier, VirtIOSCSI,
-                                 migration_state_notifier);
-    MigrationState *mig = data;
-
-    if (migration_in_setup(mig)) {
-        if (!s->dataplane_started) {
-            return;
-        }
-        virtio_scsi_dataplane_stop(s);
-        s->dataplane_disabled = true;
-    } else if (migration_has_finished(mig) ||
-               migration_has_failed(mig)) {
-        if (s->dataplane_started) {
-            return;
-        }
-        blk_drain_all(); /* complete in-flight non-dataplane requests */
-        s->dataplane_disabled = false;
-    }
-}
-
 static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -956,8 +926,6 @@ static void virtio_scsi_device_realize(DeviceState *dev, 
Error **errp)
 
     register_savevm(dev, "virtio-scsi", virtio_scsi_id++, 1,
                     virtio_scsi_save, virtio_scsi_load, s);
-    s->migration_state_notifier.notify = virtio_scsi_migration_state_changed;
-    add_migration_state_change_notifier(&s->migration_state_notifier);
 
     error_setg(&s->blocker, "block device is in use by data plane");
 
@@ -991,8 +959,6 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, 
Error **errp)
     error_free(s->blocker);
 
     unregister_savevm(dev, "virtio-scsi", s);
-    remove_migration_state_change_notifier(&s->migration_state_notifier);
-
     virtio_scsi_common_unrealize(dev, errp);
 }
 
-- 
MST