[PATCH 0/6] virtio-scsi: fix 100% CPU consumption in IOThread

[PATCH 0/6] virtio-scsi: fix 100% CPU consumption in IOThread

[Stefan Hajnoczi]

Nir Soffer reported that virtio-scsi,iothread=... consumes 100% CPU in QEMU
7.0. This patch series addresses two bugs in hw/scsi/virtio-scsi.c (see patches
1 & 2) and follows up with code cleanups.

Stefan Hajnoczi (6):
  virtio-scsi: fix ctrl and event handler functions in dataplane mode
  virtio-scsi: don't waste CPU polling the event virtqueue
  virtio-scsi: clean up virtio_scsi_handle_event_vq()
  virtio-scsi: clean up virtio_scsi_handle_ctrl_vq()
  virtio-scsi: clean up virtio_scsi_handle_cmd_vq()
  virtio-scsi: move request-related items from .h to .c

 include/hw/virtio/virtio-scsi.h |  43 --------------
 include/hw/virtio/virtio.h      |   1 +
 hw/scsi/virtio-scsi-dataplane.c |   2 +-
 hw/scsi/virtio-scsi.c           | 101 ++++++++++++++++++++++----------
 hw/virtio/virtio.c              |  13 ++++
 5 files changed, 86 insertions(+), 74 deletions(-)

-- 
2.35.1

[PATCH 1/6] virtio-scsi: fix ctrl and event handler functions in dataplane mode

[Stefan Hajnoczi]

Commit f34e8d8b8d48d73f36a67b6d5e492ef9784b5012 ("virtio-scsi: prepare
virtio_scsi_handle_cmd for dataplane") prepared the virtio-scsi cmd
virtqueue handler function to by used in both the dataplane and
non-datpalane code paths.

It failed to convert the ctrl and event virtqueue handler functions,
which are not designed to be called from the dataplane code path but
will be since the ioeventfd is set up for those virtqueues when
dataplane starts.

Convert the ctrl and event virtqueue handler functions now so they
operate correctly when called from the dataplane code path. Avoid code
duplication by extracting this code into a helper function.

Fixes: f34e8d8b8d48d73f36a67b6d5e492ef9784b5012 ("virtio-scsi: prepare virtio_scsi_handle_cmd for dataplane")
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/scsi/virtio-scsi.c | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 34a968ecfb..417fbc71d6 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -472,16 +472,32 @@ bool virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq)
     return progress;
 }
 
+/*
+ * If dataplane is configured but not yet started, do so now and return true on
+ * success.
+ *
+ * Dataplane is started by the core virtio code but virtqueue handler functions
+ * can also be invoked when a guest kicks before DRIVER_OK, so this helper
+ * function helps us deal with manually starting ioeventfd in that case.
+ */
+static bool virtio_scsi_defer_to_dataplane(VirtIOSCSI *s)
+{
+    if (!s->ctx || s->dataplane_started) {
+        return false;
+    }
+
+    virtio_device_start_ioeventfd(&s->parent_obj.parent_obj);
+    return !s->dataplane_fenced;
+}
+
 static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOSCSI *s = (VirtIOSCSI *)vdev;
 
-    if (s->ctx) {
-        virtio_device_start_ioeventfd(vdev);
-        if (!s->dataplane_fenced) {
-            return;
-        }
+    if (virtio_scsi_defer_to_dataplane(s)) {
+        return;
     }
+
     virtio_scsi_acquire(s);
     virtio_scsi_handle_ctrl_vq(s, vq);
     virtio_scsi_release(s);
@@ -720,12 +736,10 @@ 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 && !s->dataplane_started) {
-        virtio_device_start_ioeventfd(vdev);
-        if (!s->dataplane_fenced) {
-            return;
-        }
+    if (virtio_scsi_defer_to_dataplane(s)) {
+        return;
     }
+
     virtio_scsi_acquire(s);
     virtio_scsi_handle_cmd_vq(s, vq);
     virtio_scsi_release(s);
@@ -855,12 +869,10 @@ static void virtio_scsi_handle_event(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
-    if (s->ctx) {
-        virtio_device_start_ioeventfd(vdev);
-        if (!s->dataplane_fenced) {
-            return;
-        }
+    if (virtio_scsi_defer_to_dataplane(s)) {
+        return;
     }
+
     virtio_scsi_acquire(s);
     virtio_scsi_handle_event_vq(s, vq);
     virtio_scsi_release(s);
-- 
2.35.1

[PATCH 2/6] virtio-scsi: don't waste CPU polling the event virtqueue

[Stefan Hajnoczi]

The virtio-scsi event virtqueue is not emptied by its handler function.
This is typical for rx virtqueues where the device uses buffers when
some event occurs (e.g. a packet is received, an error condition
happens, etc).

Polling non-empty virtqueues wastes CPU cycles. We are not waiting for
new buffers to become available, we are waiting for an event to occur,
so it's a misuse of CPU resources to poll for buffers.

Introduce the new virtio_queue_aio_attach_host_notifier_no_poll() API,
which is identical to virtio_queue_aio_attach_host_notifier() except
that it does not poll the virtqueue.

Before this patch the following command-line consumed 100% CPU in the
IOThread polling and calling virtio_scsi_handle_event():

  $ qemu-system-x86_64 -M accel=kvm -m 1G -cpu host \
      --object iothread,id=iothread0 \
      --device virtio-scsi-pci,iothread=iothread0 \
      --blockdev file,filename=test.img,aio=native,cache.direct=on,node-name=drive0 \
      --device scsi-hd,drive=drive0

After this patch CPU is no longer wasted.

Reported-by: Nir Soffer <nsoffer@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio.h      |  1 +
 hw/scsi/virtio-scsi-dataplane.c |  2 +-
 hw/virtio/virtio.c              | 13 +++++++++++++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b31c4507f5..b62a35fdca 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -317,6 +317,7 @@ EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
 void virtio_queue_set_host_notifier_enabled(VirtQueue *vq, bool enabled);
 void virtio_queue_host_notifier_read(EventNotifier *n);
 void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx);
+void virtio_queue_aio_attach_host_notifier_no_poll(VirtQueue *vq, AioContext *ctx);
 void virtio_queue_aio_detach_host_notifier(VirtQueue *vq, AioContext *ctx);
 VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
 VirtQueue *virtio_vector_next_queue(VirtQueue *vq);
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 29575cbaf6..8bb6e6acfc 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -138,7 +138,7 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
 
     aio_context_acquire(s->ctx);
     virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, s->ctx);
-    virtio_queue_aio_attach_host_notifier(vs->event_vq, s->ctx);
+    virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, s->ctx);
 
     for (i = 0; i < vs->conf.num_queues; i++) {
         virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], s->ctx);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9d637e043e..67a873f54a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3534,6 +3534,19 @@ void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx)
                                 virtio_queue_host_notifier_aio_poll_end);
 }
 
+/*
+ * Same as virtio_queue_aio_attach_host_notifier() but without polling. Use
+ * this for rx virtqueues and similar cases where the virtqueue handler
+ * function does not pop all elements. When the virtqueue is left non-empty
+ * polling consumes CPU cycles and should not be used.
+ */
+void virtio_queue_aio_attach_host_notifier_no_poll(VirtQueue *vq, AioContext *ctx)
+{
+    aio_set_event_notifier(ctx, &vq->host_notifier, true,
+                           virtio_queue_host_notifier_read,
+                           NULL, NULL);
+}
+
 void virtio_queue_aio_detach_host_notifier(VirtQueue *vq, AioContext *ctx)
 {
     aio_set_event_notifier(ctx, &vq->host_notifier, true, NULL, NULL, NULL);
-- 
2.35.1

[PATCH 3/6] virtio-scsi: clean up virtio_scsi_handle_event_vq()

[Stefan Hajnoczi]

virtio_scsi_handle_event_vq() is only called from hw/scsi/virtio-scsi.c
now and its return value is no longer used. Remove the function
prototype from virtio-scsi.h and drop the return value.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio-scsi.h | 1 -
 hw/scsi/virtio-scsi.c           | 4 +---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 543681bc18..5957597825 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -151,7 +151,6 @@ void virtio_scsi_common_realize(DeviceState *dev,
                                 Error **errp);
 
 void virtio_scsi_common_unrealize(DeviceState *dev);
-bool virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq);
 bool virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq);
 bool virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq);
 void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req);
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 417fbc71d6..aa03a713d8 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -856,13 +856,11 @@ void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
     virtio_scsi_complete_req(req);
 }
 
-bool virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq)
+static 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);
-        return true;
     }
-    return false;
 }
 
 static void virtio_scsi_handle_event(VirtIODevice *vdev, VirtQueue *vq)
-- 
2.35.1

[PATCH 4/6] virtio-scsi: clean up virtio_scsi_handle_ctrl_vq()

[Stefan Hajnoczi]

virtio_scsi_handle_ctrl_vq() is only called from hw/scsi/virtio-scsi.c
now and its return value is no longer used. Remove the function
prototype from virtio-scsi.h and drop the return value.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio-scsi.h | 1 -
 hw/scsi/virtio-scsi.c           | 5 +----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 5957597825..44dc3b81ec 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -152,7 +152,6 @@ void virtio_scsi_common_realize(DeviceState *dev,
 
 void virtio_scsi_common_unrealize(DeviceState *dev);
 bool virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq);
-bool 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,
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index aa03a713d8..eefda16e4b 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -460,16 +460,13 @@ static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
     }
 }
 
-bool virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq)
+static void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq)
 {
     VirtIOSCSIReq *req;
-    bool progress = false;
 
     while ((req = virtio_scsi_pop_req(s, vq))) {
-        progress = true;
         virtio_scsi_handle_ctrl_req(s, req);
     }
-    return progress;
 }
 
 /*
-- 
2.35.1

[PATCH 5/6] virtio-scsi: clean up virtio_scsi_handle_cmd_vq()

[Stefan Hajnoczi]

virtio_scsi_handle_cmd_vq() is only called from hw/scsi/virtio-scsi.c
now and its return value is no longer used. Remove the function
prototype from virtio-scsi.h and drop the return value.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio-scsi.h | 1 -
 hw/scsi/virtio-scsi.c           | 5 +----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 44dc3b81ec..2497530064 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -151,7 +151,6 @@ void virtio_scsi_common_realize(DeviceState *dev,
                                 Error **errp);
 
 void virtio_scsi_common_unrealize(DeviceState *dev);
-bool virtio_scsi_handle_cmd_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,
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index eefda16e4b..12c6a21202 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -685,12 +685,11 @@ static void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req)
     scsi_req_unref(sreq);
 }
 
-bool virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
+static void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
 {
     VirtIOSCSIReq *req, *next;
     int ret = 0;
     bool suppress_notifications = virtio_queue_get_notification(vq);
-    bool progress = false;
 
     QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
 
@@ -700,7 +699,6 @@ bool virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
         }
 
         while ((req = virtio_scsi_pop_req(s, vq))) {
-            progress = true;
             ret = virtio_scsi_handle_cmd_req_prepare(s, req);
             if (!ret) {
                 QTAILQ_INSERT_TAIL(&reqs, req, next);
@@ -725,7 +723,6 @@ bool virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
     QTAILQ_FOREACH_SAFE(req, &reqs, next, next) {
         virtio_scsi_handle_cmd_req_submit(s, req);
     }
-    return progress;
 }
 
 static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
-- 
2.35.1

[PATCH 6/6] virtio-scsi: move request-related items from .h to .c

[Stefan Hajnoczi]

There is no longer a need to expose the request and related APIs in
virtio-scsi.h since there are no callers outside virtio-scsi.c.

Note the block comment in VirtIOSCSIReq has been adjusted to meet the
coding style.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio-scsi.h | 40 -----------------------------
 hw/scsi/virtio-scsi.c           | 45 ++++++++++++++++++++++++++++++---
 2 files changed, 41 insertions(+), 44 deletions(-)

diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 2497530064..abdda2cbd0 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -94,42 +94,6 @@ struct VirtIOSCSI {
     uint32_t host_features;
 };
 
-typedef struct VirtIOSCSIReq {
-    /* Note:
-     * - fields up to resp_iov are initialized by virtio_scsi_init_req;
-     * - fields starting at vring are zeroed by virtio_scsi_init_req.
-     * */
-    VirtQueueElement elem;
-
-    VirtIOSCSI *dev;
-    VirtQueue *vq;
-    QEMUSGList qsgl;
-    QEMUIOVector resp_iov;
-
-    union {
-        /* Used for two-stage request submission */
-        QTAILQ_ENTRY(VirtIOSCSIReq) next;
-
-        /* Used for cancellation of request during TMFs */
-        int remaining;
-    };
-
-    SCSIRequest *sreq;
-    size_t resp_size;
-    enum SCSIXferMode mode;
-    union {
-        VirtIOSCSICmdResp     cmd;
-        VirtIOSCSICtrlTMFResp tmf;
-        VirtIOSCSICtrlANResp  an;
-        VirtIOSCSIEvent       event;
-    } resp;
-    union {
-        VirtIOSCSICmdReq      cmd;
-        VirtIOSCSICtrlTMFReq  tmf;
-        VirtIOSCSICtrlANReq   an;
-    } req;
-} VirtIOSCSIReq;
-
 static inline void virtio_scsi_acquire(VirtIOSCSI *s)
 {
     if (s->ctx) {
@@ -151,10 +115,6 @@ void virtio_scsi_common_realize(DeviceState *dev,
                                 Error **errp);
 
 void virtio_scsi_common_unrealize(DeviceState *dev);
-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,
-                            uint32_t event, uint32_t reason);
 
 void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp);
 int virtio_scsi_dataplane_start(VirtIODevice *s);
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 12c6a21202..db54d104be 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -29,6 +29,43 @@
 #include "hw/virtio/virtio-access.h"
 #include "trace.h"
 
+typedef struct VirtIOSCSIReq {
+    /*
+     * Note:
+     * - fields up to resp_iov are initialized by virtio_scsi_init_req;
+     * - fields starting at vring are zeroed by virtio_scsi_init_req.
+     */
+    VirtQueueElement elem;
+
+    VirtIOSCSI *dev;
+    VirtQueue *vq;
+    QEMUSGList qsgl;
+    QEMUIOVector resp_iov;
+
+    union {
+        /* Used for two-stage request submission */
+        QTAILQ_ENTRY(VirtIOSCSIReq) next;
+
+        /* Used for cancellation of request during TMFs */
+        int remaining;
+    };
+
+    SCSIRequest *sreq;
+    size_t resp_size;
+    enum SCSIXferMode mode;
+    union {
+        VirtIOSCSICmdResp     cmd;
+        VirtIOSCSICtrlTMFResp tmf;
+        VirtIOSCSICtrlANResp  an;
+        VirtIOSCSIEvent       event;
+    } resp;
+    union {
+        VirtIOSCSICmdReq      cmd;
+        VirtIOSCSICtrlTMFReq  tmf;
+        VirtIOSCSICtrlANReq   an;
+    } req;
+} VirtIOSCSIReq;
+
 static inline int virtio_scsi_get_lun(uint8_t *lun)
 {
     return ((lun[2] << 8) | lun[3]) & 0x3FFF;
@@ -45,7 +82,7 @@ static inline SCSIDevice *virtio_scsi_device_get(VirtIOSCSI *s, uint8_t *lun)
     return scsi_device_get(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun));
 }
 
-void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
+static void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
     const size_t zero_skip =
@@ -58,7 +95,7 @@ void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
     memset((uint8_t *)req + zero_skip, 0, sizeof(*req) - zero_skip);
 }
 
-void virtio_scsi_free_req(VirtIOSCSIReq *req)
+static void virtio_scsi_free_req(VirtIOSCSIReq *req)
 {
     qemu_iovec_destroy(&req->resp_iov);
     qemu_sglist_destroy(&req->qsgl);
@@ -801,8 +838,8 @@ static void virtio_scsi_reset(VirtIODevice *vdev)
     s->events_dropped = false;
 }
 
-void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
-                            uint32_t event, uint32_t reason)
+static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
+                                   uint32_t event, uint32_t reason)
 {
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
     VirtIOSCSIReq *req;
-- 
2.35.1

Re: [PATCH 1/6] virtio-scsi: fix ctrl and event handler functions in dataplane mode

[Michael Tokarev]

27.04.2022 17:35, Stefan Hajnoczi wrote:
> Commit f34e8d8b8d48d73f36a67b6d5e492ef9784b5012 ("virtio-scsi: prepare
> virtio_scsi_handle_cmd for dataplane") prepared the virtio-scsi cmd
> virtqueue handler function to by used in both the dataplane and

Nitpick: "to BE used".

/mjt

Re: [PATCH 2/6] virtio-scsi: don't waste CPU polling the event virtqueue

[Nir Soffer]

On Wed, Apr 27, 2022 at 5:35 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> The virtio-scsi event virtqueue is not emptied by its handler function.
> This is typical for rx virtqueues where the device uses buffers when
> some event occurs (e.g. a packet is received, an error condition
> happens, etc).
>
> Polling non-empty virtqueues wastes CPU cycles. We are not waiting for
> new buffers to become available, we are waiting for an event to occur,
> so it's a misuse of CPU resources to poll for buffers.
>
> Introduce the new virtio_queue_aio_attach_host_notifier_no_poll() API,
> which is identical to virtio_queue_aio_attach_host_notifier() except
> that it does not poll the virtqueue.
>
> Before this patch the following command-line consumed 100% CPU in the
> IOThread polling and calling virtio_scsi_handle_event():
>
>   $ qemu-system-x86_64 -M accel=kvm -m 1G -cpu host \
>       --object iothread,id=iothread0 \
>       --device virtio-scsi-pci,iothread=iothread0 \
>       --blockdev file,filename=test.img,aio=native,cache.direct=on,node-name=drive0 \
>       --device scsi-hd,drive=drive0
>
> After this patch CPU is no longer wasted.
>
> Reported-by: Nir Soffer <nsoffer@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/hw/virtio/virtio.h      |  1 +
>  hw/scsi/virtio-scsi-dataplane.c |  2 +-
>  hw/virtio/virtio.c              | 13 +++++++++++++
>  3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index b31c4507f5..b62a35fdca 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -317,6 +317,7 @@ EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
>  void virtio_queue_set_host_notifier_enabled(VirtQueue *vq, bool enabled);
>  void virtio_queue_host_notifier_read(EventNotifier *n);
>  void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx);
> +void virtio_queue_aio_attach_host_notifier_no_poll(VirtQueue *vq, AioContext *ctx);
>  void virtio_queue_aio_detach_host_notifier(VirtQueue *vq, AioContext *ctx);
>  VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
>  VirtQueue *virtio_vector_next_queue(VirtQueue *vq);
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index 29575cbaf6..8bb6e6acfc 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -138,7 +138,7 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
>
>      aio_context_acquire(s->ctx);
>      virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, s->ctx);
> -    virtio_queue_aio_attach_host_notifier(vs->event_vq, s->ctx);
> +    virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, s->ctx);
>
>      for (i = 0; i < vs->conf.num_queues; i++) {
>          virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], s->ctx);
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 9d637e043e..67a873f54a 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3534,6 +3534,19 @@ void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx)
>                                  virtio_queue_host_notifier_aio_poll_end);
>  }
>
> +/*
> + * Same as virtio_queue_aio_attach_host_notifier() but without polling. Use
> + * this for rx virtqueues and similar cases where the virtqueue handler
> + * function does not pop all elements. When the virtqueue is left non-empty
> + * polling consumes CPU cycles and should not be used.
> + */
> +void virtio_queue_aio_attach_host_notifier_no_poll(VirtQueue *vq, AioContext *ctx)
> +{
> +    aio_set_event_notifier(ctx, &vq->host_notifier, true,
> +                           virtio_queue_host_notifier_read,
> +                           NULL, NULL);
> +}
> +
>  void virtio_queue_aio_detach_host_notifier(VirtQueue *vq, AioContext *ctx)
>  {
>      aio_set_event_notifier(ctx, &vq->host_notifier, true, NULL, NULL, NULL);
> --
> 2.35.1
>

I tested patches 1 and 2 on top of 34723f59371f3fd02ea59b94674314b875504426
and it solved the issue.

Tested-by: Nir Soffer <nsoffer@redhat.com>

Nir

Re: [PATCH 1/6] virtio-scsi: fix ctrl and event handler functions in dataplane mode

[Stefan Hajnoczi]

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

On Wed, Apr 27, 2022 at 10:47:58PM +0300, Michael Tokarev wrote:
> 27.04.2022 17:35, Stefan Hajnoczi wrote:
> > Commit f34e8d8b8d48d73f36a67b6d5e492ef9784b5012 ("virtio-scsi: prepare
> > virtio_scsi_handle_cmd for dataplane") prepared the virtio-scsi cmd
> > virtqueue handler function to by used in both the dataplane and
> 
> Nitpick: "to BE used".

Thank you, this can be fixed when merging the patch.

Stefan

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

Re: [PATCH 2/6] virtio-scsi: don't waste CPU polling the event virtqueue

[Stefan Hajnoczi]

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

On Wed, Apr 27, 2022 at 11:12:34PM +0300, Nir Soffer wrote:
> I tested patches 1 and 2 on top of 34723f59371f3fd02ea59b94674314b875504426
> and it solved the issue.
> 
> Tested-by: Nir Soffer <nsoffer@redhat.com>

Thank you!

Stefan

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

Re: [PATCH 2/6] virtio-scsi: don't waste CPU polling the event virtqueue

[Paolo Bonzini]

On 4/27/22 16:35, Stefan Hajnoczi wrote:
> This is typical for rx virtqueues where the device uses buffers when
> some event occurs (e.g. a packet is received, an error condition
> happens, etc).
> 
> Polling non-empty virtqueues wastes CPU cycles. We are not waiting for
> new buffers to become available, we are waiting for an event to occur,
> so it's a misuse of CPU resources to poll for buffers.

Shouldn't polling wait for _used_ buffers, rather than available ones?

I agree that it's generally useless to poll the event queue, but not 
because it doesn't empty the virtqueue.

Paolo

Re: [PATCH 3/6] virtio-scsi: clean up virtio_scsi_handle_event_vq()

[Paolo Bonzini]

On 4/27/22 16:35, Stefan Hajnoczi wrote:
> virtio_scsi_handle_event_vq() is only called from hw/scsi/virtio-scsi.c
> now and its return value is no longer used. Remove the function
> prototype from virtio-scsi.h and drop the return value.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 4/6] virtio-scsi: clean up virtio_scsi_handle_ctrl_vq()

[Paolo Bonzini]

On 4/27/22 16:35, Stefan Hajnoczi wrote:
> virtio_scsi_handle_ctrl_vq() is only called from hw/scsi/virtio-scsi.c
> now and its return value is no longer used. Remove the function
> prototype from virtio-scsi.h and drop the return value.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   include/hw/virtio/virtio-scsi.h | 1 -
>   hw/scsi/virtio-scsi.c           | 5 +----
>   2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index 5957597825..44dc3b81ec 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -152,7 +152,6 @@ void virtio_scsi_common_realize(DeviceState *dev,
>   
>   void virtio_scsi_common_unrealize(DeviceState *dev);
>   bool virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq);
> -bool 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,
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index aa03a713d8..eefda16e4b 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -460,16 +460,13 @@ static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
>       }
>   }
>   
> -bool virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq)
> +static void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq)
>   {
>       VirtIOSCSIReq *req;
> -    bool progress = false;
>   
>       while ((req = virtio_scsi_pop_req(s, vq))) {
> -        progress = true;
>           virtio_scsi_handle_ctrl_req(s, req);
>       }
> -    return progress;
>   }
>   
>   /*

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 5/6] virtio-scsi: clean up virtio_scsi_handle_cmd_vq()

[Paolo Bonzini]

On 4/27/22 16:35, Stefan Hajnoczi wrote:
> virtio_scsi_handle_cmd_vq() is only called from hw/scsi/virtio-scsi.c
> now and its return value is no longer used. Remove the function
> prototype from virtio-scsi.h and drop the return value.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   include/hw/virtio/virtio-scsi.h | 1 -
>   hw/scsi/virtio-scsi.c           | 5 +----
>   2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index 44dc3b81ec..2497530064 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -151,7 +151,6 @@ void virtio_scsi_common_realize(DeviceState *dev,
>                                   Error **errp);
>   
>   void virtio_scsi_common_unrealize(DeviceState *dev);
> -bool virtio_scsi_handle_cmd_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,
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index eefda16e4b..12c6a21202 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -685,12 +685,11 @@ static void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req)
>       scsi_req_unref(sreq);
>   }
>   
> -bool virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
> +static void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
>   {
>       VirtIOSCSIReq *req, *next;
>       int ret = 0;
>       bool suppress_notifications = virtio_queue_get_notification(vq);
> -    bool progress = false;
>   
>       QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
>   
> @@ -700,7 +699,6 @@ bool virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
>           }
>   
>           while ((req = virtio_scsi_pop_req(s, vq))) {
> -            progress = true;
>               ret = virtio_scsi_handle_cmd_req_prepare(s, req);
>               if (!ret) {
>                   QTAILQ_INSERT_TAIL(&reqs, req, next);
> @@ -725,7 +723,6 @@ bool virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
>       QTAILQ_FOREACH_SAFE(req, &reqs, next, next) {
>           virtio_scsi_handle_cmd_req_submit(s, req);
>       }
> -    return progress;
>   }
>   
>   static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 6/6] virtio-scsi: move request-related items from .h to .c

[Paolo Bonzini]

On 4/27/22 16:35, Stefan Hajnoczi wrote:
> There is no longer a need to expose the request and related APIs in
> virtio-scsi.h since there are no callers outside virtio-scsi.c.
> 
> Note the block comment in VirtIOSCSIReq has been adjusted to meet the
> coding style.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   include/hw/virtio/virtio-scsi.h | 40 -----------------------------
>   hw/scsi/virtio-scsi.c           | 45 ++++++++++++++++++++++++++++++---
>   2 files changed, 41 insertions(+), 44 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index 2497530064..abdda2cbd0 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -94,42 +94,6 @@ struct VirtIOSCSI {
>       uint32_t host_features;
>   };
>   
> -typedef struct VirtIOSCSIReq {
> -    /* Note:
> -     * - fields up to resp_iov are initialized by virtio_scsi_init_req;
> -     * - fields starting at vring are zeroed by virtio_scsi_init_req.
> -     * */
> -    VirtQueueElement elem;
> -
> -    VirtIOSCSI *dev;
> -    VirtQueue *vq;
> -    QEMUSGList qsgl;
> -    QEMUIOVector resp_iov;
> -
> -    union {
> -        /* Used for two-stage request submission */
> -        QTAILQ_ENTRY(VirtIOSCSIReq) next;
> -
> -        /* Used for cancellation of request during TMFs */
> -        int remaining;
> -    };
> -
> -    SCSIRequest *sreq;
> -    size_t resp_size;
> -    enum SCSIXferMode mode;
> -    union {
> -        VirtIOSCSICmdResp     cmd;
> -        VirtIOSCSICtrlTMFResp tmf;
> -        VirtIOSCSICtrlANResp  an;
> -        VirtIOSCSIEvent       event;
> -    } resp;
> -    union {
> -        VirtIOSCSICmdReq      cmd;
> -        VirtIOSCSICtrlTMFReq  tmf;
> -        VirtIOSCSICtrlANReq   an;
> -    } req;
> -} VirtIOSCSIReq;
> -
>   static inline void virtio_scsi_acquire(VirtIOSCSI *s)
>   {
>       if (s->ctx) {
> @@ -151,10 +115,6 @@ void virtio_scsi_common_realize(DeviceState *dev,
>                                   Error **errp);
>   
>   void virtio_scsi_common_unrealize(DeviceState *dev);
> -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,
> -                            uint32_t event, uint32_t reason);
>   
>   void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp);
>   int virtio_scsi_dataplane_start(VirtIODevice *s);
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 12c6a21202..db54d104be 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -29,6 +29,43 @@
>   #include "hw/virtio/virtio-access.h"
>   #include "trace.h"
>   
> +typedef struct VirtIOSCSIReq {
> +    /*
> +     * Note:
> +     * - fields up to resp_iov are initialized by virtio_scsi_init_req;
> +     * - fields starting at vring are zeroed by virtio_scsi_init_req.
> +     */
> +    VirtQueueElement elem;
> +
> +    VirtIOSCSI *dev;
> +    VirtQueue *vq;
> +    QEMUSGList qsgl;
> +    QEMUIOVector resp_iov;
> +
> +    union {
> +        /* Used for two-stage request submission */
> +        QTAILQ_ENTRY(VirtIOSCSIReq) next;
> +
> +        /* Used for cancellation of request during TMFs */
> +        int remaining;
> +    };
> +
> +    SCSIRequest *sreq;
> +    size_t resp_size;
> +    enum SCSIXferMode mode;
> +    union {
> +        VirtIOSCSICmdResp     cmd;
> +        VirtIOSCSICtrlTMFResp tmf;
> +        VirtIOSCSICtrlANResp  an;
> +        VirtIOSCSIEvent       event;
> +    } resp;
> +    union {
> +        VirtIOSCSICmdReq      cmd;
> +        VirtIOSCSICtrlTMFReq  tmf;
> +        VirtIOSCSICtrlANReq   an;
> +    } req;
> +} VirtIOSCSIReq;
> +
>   static inline int virtio_scsi_get_lun(uint8_t *lun)
>   {
>       return ((lun[2] << 8) | lun[3]) & 0x3FFF;
> @@ -45,7 +82,7 @@ static inline SCSIDevice *virtio_scsi_device_get(VirtIOSCSI *s, uint8_t *lun)
>       return scsi_device_get(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun));
>   }
>   
> -void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
> +static void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
>   {
>       VirtIODevice *vdev = VIRTIO_DEVICE(s);
>       const size_t zero_skip =
> @@ -58,7 +95,7 @@ void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
>       memset((uint8_t *)req + zero_skip, 0, sizeof(*req) - zero_skip);
>   }
>   
> -void virtio_scsi_free_req(VirtIOSCSIReq *req)
> +static void virtio_scsi_free_req(VirtIOSCSIReq *req)
>   {
>       qemu_iovec_destroy(&req->resp_iov);
>       qemu_sglist_destroy(&req->qsgl);
> @@ -801,8 +838,8 @@ static void virtio_scsi_reset(VirtIODevice *vdev)
>       s->events_dropped = false;
>   }
>   
> -void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
> -                            uint32_t event, uint32_t reason)
> +static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
> +                                   uint32_t event, uint32_t reason)
>   {
>       VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
>       VirtIOSCSIReq *req;

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 1/6] virtio-scsi: fix ctrl and event handler functions in dataplane mode

[Paolo Bonzini]

On 4/27/22 16:35, Stefan Hajnoczi wrote:
> Commit f34e8d8b8d48d73f36a67b6d5e492ef9784b5012 ("virtio-scsi: prepare
> virtio_scsi_handle_cmd for dataplane") prepared the virtio-scsi cmd
> virtqueue handler function to by used in both the dataplane and
> non-datpalane code paths.
> 
> It failed to convert the ctrl and event virtqueue handler functions,
> which are not designed to be called from the dataplane code path but
> will be since the ioeventfd is set up for those virtqueues when
> dataplane starts.
> 
> Convert the ctrl and event virtqueue handler functions now so they
> operate correctly when called from the dataplane code path. Avoid code
> duplication by extracting this code into a helper function.
> 
> Fixes: f34e8d8b8d48d73f36a67b6d5e492ef9784b5012 ("virtio-scsi: prepare virtio_scsi_handle_cmd for dataplane")
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   hw/scsi/virtio-scsi.c | 42 +++++++++++++++++++++++++++---------------
>   1 file changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 34a968ecfb..417fbc71d6 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -472,16 +472,32 @@ bool virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq)
>       return progress;
>   }
>   
> +/*
> + * If dataplane is configured but not yet started, do so now and return true on
> + * success.
> + *
> + * Dataplane is started by the core virtio code but virtqueue handler functions
> + * can also be invoked when a guest kicks before DRIVER_OK, so this helper
> + * function helps us deal with manually starting ioeventfd in that case.
> + */
> +static bool virtio_scsi_defer_to_dataplane(VirtIOSCSI *s)
> +{
> +    if (!s->ctx || s->dataplane_started) {
> +        return false;
> +    }
> +
> +    virtio_device_start_ioeventfd(&s->parent_obj.parent_obj);
> +    return !s->dataplane_fenced;
> +}
> +
>   static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>   {
>       VirtIOSCSI *s = (VirtIOSCSI *)vdev;
>   
> -    if (s->ctx) {
> -        virtio_device_start_ioeventfd(vdev);
> -        if (!s->dataplane_fenced) {
> -            return;
> -        }
> +    if (virtio_scsi_defer_to_dataplane(s)) {
> +        return;
>       }
> +
>       virtio_scsi_acquire(s);
>       virtio_scsi_handle_ctrl_vq(s, vq);
>       virtio_scsi_release(s);
> @@ -720,12 +736,10 @@ 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 && !s->dataplane_started) {
> -        virtio_device_start_ioeventfd(vdev);
> -        if (!s->dataplane_fenced) {
> -            return;
> -        }
> +    if (virtio_scsi_defer_to_dataplane(s)) {
> +        return;
>       }
> +
>       virtio_scsi_acquire(s);
>       virtio_scsi_handle_cmd_vq(s, vq);
>       virtio_scsi_release(s);
> @@ -855,12 +869,10 @@ static void virtio_scsi_handle_event(VirtIODevice *vdev, VirtQueue *vq)
>   {
>       VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>   
> -    if (s->ctx) {
> -        virtio_device_start_ioeventfd(vdev);
> -        if (!s->dataplane_fenced) {
> -            return;
> -        }
> +    if (virtio_scsi_defer_to_dataplane(s)) {
> +        return;
>       }
> +
>       virtio_scsi_acquire(s);
>       virtio_scsi_handle_event_vq(s, vq);
>       virtio_scsi_release(s);

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 2/6] virtio-scsi: don't waste CPU polling the event virtqueue

[Stefan Hajnoczi]

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

On Fri, Apr 29, 2022 at 01:17:05AM +0200, Paolo Bonzini wrote:
> On 4/27/22 16:35, Stefan Hajnoczi wrote:
> > This is typical for rx virtqueues where the device uses buffers when
> > some event occurs (e.g. a packet is received, an error condition
> > happens, etc).
> > 
> > Polling non-empty virtqueues wastes CPU cycles. We are not waiting for
> > new buffers to become available, we are waiting for an event to occur,
> > so it's a misuse of CPU resources to poll for buffers.
> 
> Shouldn't polling wait for _used_ buffers, rather than available ones?
> 
> I agree that it's generally useless to poll the event queue, but not because
> it doesn't empty the virtqueue.

This is device emulation code, not driver code. It's the device that
uses buffers and the driver that waits for used buffers. So the device
shouldn't poll for used buffers.

Stefan

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

Re: [PATCH 0/6] virtio-scsi: fix 100% CPU consumption in IOThread

[Stefan Hajnoczi]

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

On Wed, Apr 27, 2022 at 03:35:35PM +0100, Stefan Hajnoczi wrote:
> Nir Soffer reported that virtio-scsi,iothread=... consumes 100% CPU in QEMU
> 7.0. This patch series addresses two bugs in hw/scsi/virtio-scsi.c (see patches
> 1 & 2) and follows up with code cleanups.
> 
> Stefan Hajnoczi (6):
>   virtio-scsi: fix ctrl and event handler functions in dataplane mode
>   virtio-scsi: don't waste CPU polling the event virtqueue
>   virtio-scsi: clean up virtio_scsi_handle_event_vq()
>   virtio-scsi: clean up virtio_scsi_handle_ctrl_vq()
>   virtio-scsi: clean up virtio_scsi_handle_cmd_vq()
>   virtio-scsi: move request-related items from .h to .c
> 
>  include/hw/virtio/virtio-scsi.h |  43 --------------
>  include/hw/virtio/virtio.h      |   1 +
>  hw/scsi/virtio-scsi-dataplane.c |   2 +-
>  hw/scsi/virtio-scsi.c           | 101 ++++++++++++++++++++++----------
>  hw/virtio/virtio.c              |  13 ++++
>  5 files changed, 86 insertions(+), 74 deletions(-)
> 
> -- 
> 2.35.1
> 

Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block

Stefan

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