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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1734 bytes --]

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 || ...)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]