diff mbox series

[1/2] virtio-scsi: replace VirtIOBlock dataplane_{start/starting/stopped} with enum

Message ID 20220808094147.612472-2-eesposit@redhat.com (mailing list archive)
State New, archived
Headers show
Series virtio-blk and scsi: replace dataplane_{start/stopping/started} | expand

Commit Message

Emanuele Giuseppe Esposito Aug. 8, 2022, 9:41 a.m. UTC 
Simplify the various dataplane stages in dataplane_start/stop by using
a single enum instead of having multiple flags.

Read/write the enum atomically, as it can be read also by iothread
callbacks.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 21 +++++++++------------
 hw/scsi/virtio-scsi.c           | 10 ++++++----
 include/hw/virtio/virtio-scsi.h |  5 ++---
 include/hw/virtio/virtio.h      |  7 +++++++
 4 files changed, 24 insertions(+), 19 deletions(-)

Comments

Stefan Hajnoczi Aug. 8, 2022, 3:43 p.m. UTC | #1 
On Mon, Aug 08, 2022 at 05:41:46AM -0400, Emanuele Giuseppe Esposito wrote:
> Simplify the various dataplane stages in dataplane_start/stop by using
> a single enum instead of having multiple flags.
> 
> Read/write the enum atomically, as it can be read also by iothread
> callbacks.

What guarantees that these relaxed loads/stores always produce
DATAPLANE_STARTING/STARTED in virtio_scsi_defer_to_dataplane() and not
an older value? Are there implicit memory barriers?

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  hw/scsi/virtio-scsi-dataplane.c | 21 +++++++++------------
>  hw/scsi/virtio-scsi.c           | 10 ++++++----
>  include/hw/virtio/virtio-scsi.h |  5 ++---
>  include/hw/virtio/virtio.h      |  7 +++++++
>  4 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index a575c3f0cd..9ad73e3e19 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -106,13 +106,12 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
>      VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>  
> -    if (s->dataplane_started ||
> -        s->dataplane_starting ||
> +    if (qatomic_read(&s->dataplane_state) <= DATAPLANE_STARTED ||

It's not obvious that the STOPPING and STOPPED constants have a value
greater than STARTING and STARTED. It could be other way around too. It
would be safer to write the code so there are no assumptions about the
constants:

  VirtIODataplaneStates state = qatomic_read(&s->dataplane_state);

  if (state == DATAPLANE_STARTING || state == DATAPLANE_STARTED || ...)
diff mbox series

Patch

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index a575c3f0cd..9ad73e3e19 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -106,13 +106,12 @@  int virtio_scsi_dataplane_start(VirtIODevice *vdev)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
-    if (s->dataplane_started ||
-        s->dataplane_starting ||
+    if (qatomic_read(&s->dataplane_state) <= DATAPLANE_STARTED ||
         s->dataplane_fenced) {
         return 0;
     }
 
-    s->dataplane_starting = true;
+    qatomic_set(&s->dataplane_state, DATAPLANE_STARTING);
 
     /* Set up guest notifier (irq) */
     rc = k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, true);
@@ -151,8 +150,7 @@  int virtio_scsi_dataplane_start(VirtIODevice *vdev)
 
     memory_region_transaction_commit();
 
-    s->dataplane_starting = false;
-    s->dataplane_started = true;
+    qatomic_set(&s->dataplane_state, DATAPLANE_STARTED);
 
     /*
      * Attach notifiers from within the IOThread. It's possible to attach
@@ -183,8 +181,8 @@  fail_host_notifiers:
     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;
+    qatomic_set(&s->dataplane_state, DATAPLANE_STARTED);
+
     return -ENOSYS;
 }
 
@@ -197,17 +195,17 @@  void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
     int i;
 
-    if (!s->dataplane_started || s->dataplane_stopping) {
+    if (qatomic_read(&s->dataplane_state) != DATAPLANE_STARTED) {
         return;
     }
 
     /* Better luck next time. */
     if (s->dataplane_fenced) {
         s->dataplane_fenced = false;
-        s->dataplane_started = false;
+        qatomic_set(&s->dataplane_state, DATAPLANE_STOPPED);
         return;
     }
-    s->dataplane_stopping = true;
+    qatomic_set(&s->dataplane_state, DATAPLANE_STOPPING);
 
     aio_context_acquire(s->ctx);
     aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_stop_bh, s);
@@ -237,6 +235,5 @@  void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
 
     /* 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;
+    qatomic_set(&s->dataplane_state, DATAPLANE_STOPPED);
 }
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 4141dddd51..e6ff667e86 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -110,7 +110,8 @@  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 && !s->dataplane_fenced) {
+    if (qatomic_read(&s->dataplane_state) == DATAPLANE_STARTED &&
+        !s->dataplane_fenced) {
         virtio_notify_irqfd(vdev, vq);
     } else {
         virtio_notify(vdev, vq);
@@ -288,7 +289,8 @@  static void virtio_scsi_cancel_notify(Notifier *notifier, void *data)
 
 static inline void virtio_scsi_ctx_check(VirtIOSCSI *s, SCSIDevice *d)
 {
-    if (s->dataplane_started && d && blk_is_available(d->conf.blk)) {
+    if (qatomic_read(&s->dataplane_state) == DATAPLANE_STARTED && d &&
+        blk_is_available(d->conf.blk)) {
         assert(blk_get_aio_context(d->conf.blk) == s->ctx);
     }
 }
@@ -516,7 +518,7 @@  static void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq)
  */
 static bool virtio_scsi_defer_to_dataplane(VirtIOSCSI *s)
 {
-    if (!s->ctx || s->dataplane_started) {
+    if (!s->ctx || qatomic_read(&s->dataplane_state) <= DATAPLANE_STARTED) {
         return false;
     }
 
@@ -828,7 +830,7 @@  static void virtio_scsi_reset(VirtIODevice *vdev)
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
 
-    assert(!s->dataplane_started);
+    assert(qatomic_read(&s->dataplane_state) != DATAPLANE_STARTED);
     s->resetting++;
     qbus_reset_all(BUS(&s->bus));
     s->resetting--;
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index a36aad9c86..e9e922dff4 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -85,9 +85,8 @@  struct VirtIOSCSI {
     /* Fields for dataplane below */
     AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
 
-    bool dataplane_started;
-    bool dataplane_starting;
-    bool dataplane_stopping;
+    enum VirtIODataplaneStates dataplane_state;
+
     bool dataplane_fenced;
     uint32_t host_features;
 };
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index db1c0ddf6b..25d1515570 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -71,6 +71,13 @@  typedef struct VirtQueueElement
 #define TYPE_VIRTIO_DEVICE "virtio-device"
 OBJECT_DECLARE_TYPE(VirtIODevice, VirtioDeviceClass, VIRTIO_DEVICE)
 
+enum VirtIODataplaneStates {
+    DATAPLANE_STARTING, /* dataplane is being initialized */
+    DATAPLANE_STARTED,  /* dataplane has been intialized */
+    DATAPLANE_STOPPING, /* dataplane is being stopped */
+    DATAPLANE_STOPPED,  /* dataplane is stopped */
+};
+
 enum virtio_device_endian {
     VIRTIO_DEVICE_ENDIAN_UNKNOWN,
     VIRTIO_DEVICE_ENDIAN_LITTLE,