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

[Stefan Hajnoczi]

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 
> ---
>  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(>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(>dataplane_state);

  if (state == DATAPLANE_STARTING || state == DATAPLANE_STARTED || ...)


signature.asc
Description: PGP signature

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

[Emanuele Giuseppe Esposito]

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 
---
 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(>dataplane_state) <= DATAPLANE_STARTED ||
 s->dataplane_fenced) {
 return 0;
 }
 
-s->dataplane_starting = true;
+qatomic_set(>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(>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(>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(>dataplane_state) != DATAPLANE_STARTED) {
 return;
 }
 
 /* Better luck next time. */
 if (s->dataplane_fenced) {
 s->dataplane_fenced = false;
-s->dataplane_started = false;
+qatomic_set(>dataplane_state, DATAPLANE_STOPPED);
 return;
 }
-s->dataplane_stopping = true;
+qatomic_set(>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(>dataplane_state, DATAPLANE_STOPPED);
 }
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 414151..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(>resp_iov, 0, >resp, req->resp_size);
 virtqueue_push(vq, >elem, req->qsgl.size + req->resp_iov.size);
-if (s->dataplane_started && !s->dataplane_fenced) {
+if (qatomic_read(>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(>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(>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(>dataplane_state) != DATAPLANE_STARTED);
 s->resetting++;
 qbus_reset_all(BUS(>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