* [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter
@ 2025-02-13 18:00 Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 01/12] scsi-disk: drop unused SCSIDiskState->bh field Stefan Hajnoczi
` (13 more replies)
0 siblings, 14 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
Implement --device virtio-scsi-pci,iothread-vq-mapping= support so that
virtqueues can be assigned to different IOThreads. This improves SMP guest
scalability where I/O-intensive applications can become bottlenecked on a
single IOThread.
The following benchmark results show the effect of iothread-vq-mapping. fio
randread 4k iodepth=64 results from a 4 vCPU guest with an Intel P4800X SSD:
iothreads IOPS
------------------------------
1 189576
2 312698
4 346744
The virtio-scsi device model and core SCSI emulation currently assume that
requests are processed in a single AioContext. This patch series goes about
modifying this as follows:
scsi-disk: drop unused SCSIDiskState->bh field
dma: use current AioContext for dma_blk_io()
Make dma-helpers.c support the QEMU multi-queue block layer by using
qemu_get_current_aio_context().
scsi: track per-SCSIRequest AioContext
scsi: introduce requests_lock
Make the core SCSI emulation code support processing requests in multiple
AioContexts by protecting the per-SCSIDevice requests list.
virtio-scsi: introduce event and ctrl virtqueue locks
virtio-scsi: protect events_dropped field
virtio-scsi: perform TMFs in appropriate AioContexts
Make the virtio-scsi emulation code support processing requests in multiple
AioContexts. The event and ctrl virtqueues can interact with multiple
AioContexts. Especially the SCSI Task Management Functions (TMFs) handled by
the ctrl virtqueue need to be made thread-safe.
virtio-blk: extract cleanup_iothread_vq_mapping() function
virtio-blk: tidy up iothread_vq_mapping functions
virtio: extract iothread-vq-mapping.h API
virtio-scsi: add iothread-vq-mapping parameter
Port over the iothread-vq-mapping qdev property from virtio-blk to virtio-scsi.
virtio-scsi: handle ctrl virtqueue in main loop
Simplify TMF handling now that there is no longer a single AioContext where all
requests are processed.
Stefan Hajnoczi (12):
scsi-disk: drop unused SCSIDiskState->bh field
dma: use current AioContext for dma_blk_io()
scsi: track per-SCSIRequest AioContext
scsi: introduce requests_lock
virtio-scsi: introduce event and ctrl virtqueue locks
virtio-scsi: protect events_dropped field
virtio-scsi: perform TMFs in appropriate AioContexts
virtio-blk: extract cleanup_iothread_vq_mapping() function
virtio-blk: tidy up iothread_vq_mapping functions
virtio: extract iothread-vq-mapping.h API
virtio-scsi: add iothread-vq-mapping parameter
virtio-scsi: handle ctrl virtqueue in main loop
include/hw/scsi/scsi.h | 8 +-
include/hw/virtio/iothread-vq-mapping.h | 45 ++
include/hw/virtio/virtio-scsi.h | 15 +-
include/system/dma.h | 3 +-
hw/block/virtio-blk.c | 132 +-----
hw/ide/core.c | 3 +-
hw/ide/macio.c | 3 +-
hw/scsi/scsi-bus.c | 121 ++++--
hw/scsi/scsi-disk.c | 24 +-
hw/scsi/virtio-scsi-dataplane.c | 96 +++--
hw/scsi/virtio-scsi.c | 534 ++++++++++++++----------
hw/virtio/iothread-vq-mapping.c | 131 ++++++
system/dma-helpers.c | 8 +-
hw/virtio/meson.build | 1 +
14 files changed, 672 insertions(+), 452 deletions(-)
create mode 100644 include/hw/virtio/iothread-vq-mapping.h
create mode 100644 hw/virtio/iothread-vq-mapping.c
--
2.48.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 01/12] scsi-disk: drop unused SCSIDiskState->bh field
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
@ 2025-02-13 18:00 ` Stefan Hajnoczi
2025-03-11 9:29 ` Philippe Mathieu-Daudé
2025-02-13 18:00 ` [PATCH 02/12] dma: use current AioContext for dma_blk_io() Stefan Hajnoczi
` (12 subsequent siblings)
13 siblings, 1 reply; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
Commit 71544d30a6f8 ("scsi: push request restart to SCSIDevice") removed
the only user of SCSIDiskState->bh.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/scsi/scsi-disk.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index e7f738b484..caf6c1437f 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -106,7 +106,6 @@ struct SCSIDiskState {
uint64_t max_unmap_size;
uint64_t max_io_size;
uint32_t quirks;
- QEMUBH *bh;
char *version;
char *serial;
char *vendor;
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 02/12] dma: use current AioContext for dma_blk_io()
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 01/12] scsi-disk: drop unused SCSIDiskState->bh field Stefan Hajnoczi
@ 2025-02-13 18:00 ` Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 03/12] scsi: track per-SCSIRequest AioContext Stefan Hajnoczi
` (11 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
In the past a single AioContext was used for block I/O and it was
fetched using blk_get_aio_context(). Nowadays the block layer supports
running I/O from any AioContext and multiple AioContexts at the same
time. Remove the dma_blk_io() AioContext argument and use the current
AioContext instead.
This makes calling the function easier and enables multiple IOThreads to
use dma_blk_io() concurrently for the same block device.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/system/dma.h | 3 +--
hw/ide/core.c | 3 +--
hw/ide/macio.c | 3 +--
hw/scsi/scsi-disk.c | 6 ++----
system/dma-helpers.c | 8 ++++----
5 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/include/system/dma.h b/include/system/dma.h
index 5a49a30628..e142f7efa6 100644
--- a/include/system/dma.h
+++ b/include/system/dma.h
@@ -290,8 +290,7 @@ typedef BlockAIOCB *DMAIOFunc(int64_t offset, QEMUIOVector *iov,
BlockCompletionFunc *cb, void *cb_opaque,
void *opaque);
-BlockAIOCB *dma_blk_io(AioContext *ctx,
- QEMUSGList *sg, uint64_t offset, uint32_t align,
+BlockAIOCB *dma_blk_io(QEMUSGList *sg, uint64_t offset, uint32_t align,
DMAIOFunc *io_func, void *io_func_opaque,
BlockCompletionFunc *cb, void *opaque, DMADirection dir);
BlockAIOCB *dma_blk_read(BlockBackend *blk,
diff --git a/hw/ide/core.c b/hw/ide/core.c
index f9baba59e9..b14983ec54 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -968,8 +968,7 @@ static void ide_dma_cb(void *opaque, int ret)
BDRV_SECTOR_SIZE, ide_dma_cb, s);
break;
case IDE_DMA_TRIM:
- s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk),
- &s->sg, offset, BDRV_SECTOR_SIZE,
+ s->bus->dma->aiocb = dma_blk_io(&s->sg, offset, BDRV_SECTOR_SIZE,
ide_issue_trim, s, ide_dma_cb, s,
DMA_DIRECTION_TO_DEVICE);
break;
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 5fe764b49b..c8e8e44cc9 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -187,8 +187,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
pmac_ide_transfer_cb, io);
break;
case IDE_DMA_TRIM:
- s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk), &s->sg,
- offset, 0x1, ide_issue_trim, s,
+ s->bus->dma->aiocb = dma_blk_io(&s->sg, offset, 0x1, ide_issue_trim, s,
pmac_ide_transfer_cb, io,
DMA_DIRECTION_TO_DEVICE);
break;
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index caf6c1437f..f049a20275 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -487,8 +487,7 @@ static void scsi_do_read(SCSIDiskReq *r, int ret)
if (r->req.sg) {
dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, BLOCK_ACCT_READ);
r->req.residual -= r->req.sg->size;
- r->req.aiocb = dma_blk_io(blk_get_aio_context(s->qdev.conf.blk),
- r->req.sg, r->sector << BDRV_SECTOR_BITS,
+ r->req.aiocb = dma_blk_io(r->req.sg, r->sector << BDRV_SECTOR_BITS,
BDRV_SECTOR_SIZE,
sdc->dma_readv, r, scsi_dma_complete, r,
DMA_DIRECTION_FROM_DEVICE);
@@ -650,8 +649,7 @@ static void scsi_write_data(SCSIRequest *req)
if (r->req.sg) {
dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, BLOCK_ACCT_WRITE);
r->req.residual -= r->req.sg->size;
- r->req.aiocb = dma_blk_io(blk_get_aio_context(s->qdev.conf.blk),
- r->req.sg, r->sector << BDRV_SECTOR_BITS,
+ r->req.aiocb = dma_blk_io(r->req.sg, r->sector << BDRV_SECTOR_BITS,
BDRV_SECTOR_SIZE,
sdc->dma_writev, r, scsi_dma_complete, r,
DMA_DIRECTION_TO_DEVICE);
diff --git a/system/dma-helpers.c b/system/dma-helpers.c
index f6403242f5..6bad75876f 100644
--- a/system/dma-helpers.c
+++ b/system/dma-helpers.c
@@ -211,7 +211,7 @@ static const AIOCBInfo dma_aiocb_info = {
.cancel_async = dma_aio_cancel,
};
-BlockAIOCB *dma_blk_io(AioContext *ctx,
+BlockAIOCB *dma_blk_io(
QEMUSGList *sg, uint64_t offset, uint32_t align,
DMAIOFunc *io_func, void *io_func_opaque,
BlockCompletionFunc *cb,
@@ -223,7 +223,7 @@ BlockAIOCB *dma_blk_io(AioContext *ctx,
dbs->acb = NULL;
dbs->sg = sg;
- dbs->ctx = ctx;
+ dbs->ctx = qemu_get_current_aio_context();
dbs->offset = offset;
dbs->align = align;
dbs->sg_cur_index = 0;
@@ -251,7 +251,7 @@ BlockAIOCB *dma_blk_read(BlockBackend *blk,
QEMUSGList *sg, uint64_t offset, uint32_t align,
void (*cb)(void *opaque, int ret), void *opaque)
{
- return dma_blk_io(blk_get_aio_context(blk), sg, offset, align,
+ return dma_blk_io(sg, offset, align,
dma_blk_read_io_func, blk, cb, opaque,
DMA_DIRECTION_FROM_DEVICE);
}
@@ -269,7 +269,7 @@ BlockAIOCB *dma_blk_write(BlockBackend *blk,
QEMUSGList *sg, uint64_t offset, uint32_t align,
void (*cb)(void *opaque, int ret), void *opaque)
{
- return dma_blk_io(blk_get_aio_context(blk), sg, offset, align,
+ return dma_blk_io(sg, offset, align,
dma_blk_write_io_func, blk, cb, opaque,
DMA_DIRECTION_TO_DEVICE);
}
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 03/12] scsi: track per-SCSIRequest AioContext
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 01/12] scsi-disk: drop unused SCSIDiskState->bh field Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 02/12] dma: use current AioContext for dma_blk_io() Stefan Hajnoczi
@ 2025-02-13 18:00 ` Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 04/12] scsi: introduce requests_lock Stefan Hajnoczi
` (10 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
Until now, a SCSIDevice's I/O requests have run in a single AioContext.
In order to support multiple IOThreads it will be necessary to move to
the concept of a per-SCSIRequest AioContext.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/hw/scsi/scsi.h | 1 +
hw/scsi/scsi-bus.c | 1 +
hw/scsi/scsi-disk.c | 17 ++++++-----------
3 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index c3d5e17e38..ffc48203f9 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -24,6 +24,7 @@ struct SCSIRequest {
SCSIBus *bus;
SCSIDevice *dev;
const SCSIReqOps *ops;
+ AioContext *ctx;
uint32_t refcount;
uint32_t tag;
uint32_t lun;
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 7d4546800f..846bbbf0ec 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -868,6 +868,7 @@ invalid_opcode:
}
}
+ req->ctx = qemu_get_current_aio_context();
req->cmd = cmd;
req->residual = req->cmd.xfer;
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index f049a20275..7cf8c31b98 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -328,9 +328,8 @@ static void scsi_aio_complete(void *opaque, int ret)
SCSIDiskReq *r = (SCSIDiskReq *)opaque;
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
- /* The request must only run in the BlockBackend's AioContext */
- assert(blk_get_aio_context(s->qdev.conf.blk) ==
- qemu_get_current_aio_context());
+ /* The request must run in its AioContext */
+ assert(r->req.ctx == qemu_get_current_aio_context());
assert(r->req.aiocb != NULL);
r->req.aiocb = NULL;
@@ -430,12 +429,10 @@ static void scsi_dma_complete(void *opaque, int ret)
static void scsi_read_complete_noio(SCSIDiskReq *r, int ret)
{
- SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
uint32_t n;
- /* The request must only run in the BlockBackend's AioContext */
- assert(blk_get_aio_context(s->qdev.conf.blk) ==
- qemu_get_current_aio_context());
+ /* The request must run in its AioContext */
+ assert(r->req.ctx == qemu_get_current_aio_context());
assert(r->req.aiocb == NULL);
if (scsi_disk_req_check_error(r, ret, ret > 0)) {
@@ -562,12 +559,10 @@ static void scsi_read_data(SCSIRequest *req)
static void scsi_write_complete_noio(SCSIDiskReq *r, int ret)
{
- SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
uint32_t n;
- /* The request must only run in the BlockBackend's AioContext */
- assert(blk_get_aio_context(s->qdev.conf.blk) ==
- qemu_get_current_aio_context());
+ /* The request must run in its AioContext */
+ assert(r->req.ctx == qemu_get_current_aio_context());
assert (r->req.aiocb == NULL);
if (scsi_disk_req_check_error(r, ret, ret > 0)) {
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 04/12] scsi: introduce requests_lock
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
` (2 preceding siblings ...)
2025-02-13 18:00 ` [PATCH 03/12] scsi: track per-SCSIRequest AioContext Stefan Hajnoczi
@ 2025-02-13 18:00 ` Stefan Hajnoczi
2025-03-10 13:37 ` Kevin Wolf
2025-02-13 18:00 ` [PATCH 05/12] virtio-scsi: introduce event and ctrl virtqueue locks Stefan Hajnoczi
` (9 subsequent siblings)
13 siblings, 1 reply; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
SCSIDevice keeps track of in-flight requests for device reset and Task
Management Functions (TMFs). The request list requires protection so
that multi-threaded SCSI emulation can be implemented in commits that
follow.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/hw/scsi/scsi.h | 7 ++-
hw/scsi/scsi-bus.c | 120 +++++++++++++++++++++++++++++------------
2 files changed, 88 insertions(+), 39 deletions(-)
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index ffc48203f9..90ee192b4d 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -49,6 +49,8 @@ struct SCSIRequest {
bool dma_started;
BlockAIOCB *aiocb;
QEMUSGList *sg;
+
+ /* Protected by SCSIDevice->requests_lock */
QTAILQ_ENTRY(SCSIRequest) next;
};
@@ -77,10 +79,7 @@ struct SCSIDevice
uint8_t sense[SCSI_SENSE_BUF_SIZE];
uint32_t sense_len;
- /*
- * The requests list is only accessed from the AioContext that executes
- * requests or from the main loop when IOThread processing is stopped.
- */
+ QemuMutex requests_lock; /* protects the requests list */
QTAILQ_HEAD(, SCSIRequest) requests;
uint32_t channel;
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 846bbbf0ec..ece1107ee8 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -100,8 +100,15 @@ static void scsi_device_for_each_req_sync(SCSIDevice *s,
assert(!runstate_is_running());
assert(qemu_in_main_thread());
- QTAILQ_FOREACH_SAFE(req, &s->requests, next, next_req) {
- fn(req, opaque);
+ /*
+ * Locking is not necessary because the guest is stopped and no other
+ * threads can be accessing the requests list, but take the lock for
+ * consistency.
+ */
+ WITH_QEMU_LOCK_GUARD(&s->requests_lock) {
+ QTAILQ_FOREACH_SAFE(req, &s->requests, next, next_req) {
+ fn(req, opaque);
+ }
}
}
@@ -115,21 +122,29 @@ static void scsi_device_for_each_req_async_bh(void *opaque)
{
g_autofree SCSIDeviceForEachReqAsyncData *data = opaque;
SCSIDevice *s = data->s;
- AioContext *ctx;
- SCSIRequest *req;
- SCSIRequest *next;
+ g_autoptr(GList) reqs = NULL;
/*
- * The BB cannot have changed contexts between this BH being scheduled and
- * now: BBs' AioContexts, when they have a node attached, can only be
- * changed via bdrv_try_change_aio_context(), in a drained section. While
- * we have the in-flight counter incremented, that drain must block.
+ * Build a list of requests in this AioContext so fn() can be invoked later
+ * outside requests_lock.
*/
- ctx = blk_get_aio_context(s->conf.blk);
- assert(ctx == qemu_get_current_aio_context());
+ WITH_QEMU_LOCK_GUARD(&s->requests_lock) {
+ AioContext *ctx = qemu_get_current_aio_context();
+ SCSIRequest *req;
+ SCSIRequest *next;
- QTAILQ_FOREACH_SAFE(req, &s->requests, next, next) {
- data->fn(req, data->fn_opaque);
+ QTAILQ_FOREACH_SAFE(req, &s->requests, next, next) {
+ if (req->ctx == ctx) {
+ scsi_req_ref(req); /* dropped after calling fn() */
+ reqs = g_list_prepend(reqs, req);
+ }
+ }
+ }
+
+ /* Call fn() on each request */
+ for (GList *elem = g_list_first(reqs); elem; elem = g_list_next(elem)) {
+ data->fn(elem->data, data->fn_opaque);
+ scsi_req_unref(elem->data);
}
/* Drop the reference taken by scsi_device_for_each_req_async() */
@@ -139,9 +154,35 @@ static void scsi_device_for_each_req_async_bh(void *opaque)
blk_dec_in_flight(s->conf.blk);
}
+static void scsi_device_for_each_req_async_do_ctx(gpointer key, gpointer value,
+ gpointer user_data)
+{
+ AioContext *ctx = key;
+ SCSIDeviceForEachReqAsyncData *params = user_data;
+ SCSIDeviceForEachReqAsyncData *data;
+
+ data = g_new(SCSIDeviceForEachReqAsyncData, 1);
+ data->s = params->s;
+ data->fn = params->fn;
+ data->fn_opaque = params->fn_opaque;
+
+ /*
+ * Hold a reference to the SCSIDevice until
+ * scsi_device_for_each_req_async_bh() finishes.
+ */
+ object_ref(OBJECT(data->s));
+
+ /* Paired with scsi_device_for_each_req_async_bh() */
+ blk_inc_in_flight(data->s->conf.blk);
+
+ aio_bh_schedule_oneshot(ctx, scsi_device_for_each_req_async_bh, data);
+}
+
/*
* Schedule @fn() to be invoked for each enqueued request in device @s. @fn()
- * runs in the AioContext that is executing the request.
+ * must be thread-safe because it runs concurrently in each AioContext that is
+ * executing a request.
+ *
* Keeps the BlockBackend's in-flight counter incremented until everything is
* done, so draining it will settle all scheduled @fn() calls.
*/
@@ -151,24 +192,26 @@ static void scsi_device_for_each_req_async(SCSIDevice *s,
{
assert(qemu_in_main_thread());
- SCSIDeviceForEachReqAsyncData *data =
- g_new(SCSIDeviceForEachReqAsyncData, 1);
+ /* The set of AioContexts where the requests are being processed */
+ g_autoptr(GHashTable) aio_contexts = g_hash_table_new(NULL, NULL);
+ WITH_QEMU_LOCK_GUARD(&s->requests_lock) {
+ SCSIRequest *req;
+ QTAILQ_FOREACH(req, &s->requests, next) {
+ g_hash_table_add(aio_contexts, req->ctx);
+ }
+ }
- data->s = s;
- data->fn = fn;
- data->fn_opaque = opaque;
-
- /*
- * Hold a reference to the SCSIDevice until
- * scsi_device_for_each_req_async_bh() finishes.
- */
- object_ref(OBJECT(s));
-
- /* Paired with blk_dec_in_flight() in scsi_device_for_each_req_async_bh() */
- blk_inc_in_flight(s->conf.blk);
- aio_bh_schedule_oneshot(blk_get_aio_context(s->conf.blk),
- scsi_device_for_each_req_async_bh,
- data);
+ /* Schedule a BH for each AioContext */
+ SCSIDeviceForEachReqAsyncData params = {
+ .s = s,
+ .fn = fn,
+ .fn_opaque = opaque,
+ };
+ g_hash_table_foreach(
+ aio_contexts,
+ scsi_device_for_each_req_async_do_ctx,
+ ¶ms
+ );
}
static void scsi_device_realize(SCSIDevice *s, Error **errp)
@@ -349,6 +392,7 @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
dev->lun = lun;
}
+ qemu_mutex_init(&dev->requests_lock);
QTAILQ_INIT(&dev->requests);
scsi_device_realize(dev, &local_err);
if (local_err) {
@@ -369,6 +413,8 @@ static void scsi_qdev_unrealize(DeviceState *qdev)
scsi_device_purge_requests(dev, SENSE_CODE(NO_SENSE));
+ qemu_mutex_destroy(&dev->requests_lock);
+
scsi_device_unrealize(dev);
blockdev_mark_auto_del(dev->conf.blk);
@@ -965,7 +1011,10 @@ static void scsi_req_enqueue_internal(SCSIRequest *req)
req->sg = NULL;
}
req->enqueued = true;
- QTAILQ_INSERT_TAIL(&req->dev->requests, req, next);
+
+ WITH_QEMU_LOCK_GUARD(&req->dev->requests_lock) {
+ QTAILQ_INSERT_TAIL(&req->dev->requests, req, next);
+ }
}
int32_t scsi_req_enqueue(SCSIRequest *req)
@@ -985,7 +1034,9 @@ static void scsi_req_dequeue(SCSIRequest *req)
trace_scsi_req_dequeue(req->dev->id, req->lun, req->tag);
req->retry = false;
if (req->enqueued) {
- QTAILQ_REMOVE(&req->dev->requests, req, next);
+ WITH_QEMU_LOCK_GUARD(&req->dev->requests_lock) {
+ QTAILQ_REMOVE(&req->dev->requests, req, next);
+ }
req->enqueued = false;
scsi_req_unref(req);
}
@@ -1962,8 +2013,7 @@ static void scsi_device_class_init(ObjectClass *klass, void *data)
static void scsi_dev_instance_init(Object *obj)
{
- DeviceState *dev = DEVICE(obj);
- SCSIDevice *s = SCSI_DEVICE(dev);
+ SCSIDevice *s = SCSI_DEVICE(obj);
device_add_bootindex_property(obj, &s->conf.bootindex,
"bootindex", NULL,
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 05/12] virtio-scsi: introduce event and ctrl virtqueue locks
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
` (3 preceding siblings ...)
2025-02-13 18:00 ` [PATCH 04/12] scsi: introduce requests_lock Stefan Hajnoczi
@ 2025-02-13 18:00 ` Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 06/12] virtio-scsi: protect events_dropped field Stefan Hajnoczi
` (8 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
Virtqueues are not thread-safe. Until now this was not a major issue
since all virtqueue processing happened in the same thread. The ctrl
queue's Task Management Function (TMF) requests sometimes need the main
loop, so a BH was used to schedule the virtqueue completion back in the
thread that has virtqueue access.
When IOThread Virtqueue Mapping is introduced in later commits, event
and ctrl virtqueue accesses from other threads will become necessary.
Introduce an optional per-virtqueue lock so the event and ctrl
virtqueues can be protected in the commits that follow.
The addition of the ctrl virtqueue lock makes
virtio_scsi_complete_req_from_main_loop() and its BH unnecessary.
Instead, take the ctrl virtqueue lock from the main loop thread.
The cmd virtqueue does not have a lock because the entirety of SCSI
command processing happens in one thread. Only one thread accesses the
cmd virtqueue and a lock is unnecessary.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/hw/virtio/virtio-scsi.h | 3 ++
hw/scsi/virtio-scsi.c | 90 ++++++++++++++++++---------------
2 files changed, 52 insertions(+), 41 deletions(-)
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index be230cd4bf..4ee98ebf63 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -84,6 +84,9 @@ struct VirtIOSCSI {
int resetting; /* written from main loop thread, read from any thread */
bool events_dropped;
+ QemuMutex ctrl_lock; /* protects ctrl_vq */
+ QemuMutex event_lock; /* protects event_vq */
+
/*
* TMFs deferred to main loop BH. These fields are protected by
* tmf_bh_lock.
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 7d094e1881..073ccd3d5b 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -102,13 +102,18 @@ static void virtio_scsi_free_req(VirtIOSCSIReq *req)
g_free(req);
}
-static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
+static void virtio_scsi_complete_req(VirtIOSCSIReq *req, QemuMutex *vq_lock)
{
VirtIOSCSI *s = req->dev;
VirtQueue *vq = req->vq;
VirtIODevice *vdev = VIRTIO_DEVICE(s);
qemu_iovec_from_buf(&req->resp_iov, 0, &req->resp, req->resp_size);
+
+ if (vq_lock) {
+ qemu_mutex_lock(vq_lock);
+ }
+
virtqueue_push(vq, &req->elem, req->qsgl.size + req->resp_iov.size);
if (s->dataplane_started && !s->dataplane_fenced) {
virtio_notify_irqfd(vdev, vq);
@@ -116,6 +121,10 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
virtio_notify(vdev, vq);
}
+ if (vq_lock) {
+ qemu_mutex_unlock(vq_lock);
+ }
+
if (req->sreq) {
req->sreq->hba_private = NULL;
scsi_req_unref(req->sreq);
@@ -123,34 +132,20 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
virtio_scsi_free_req(req);
}
-static void virtio_scsi_complete_req_bh(void *opaque)
-{
- VirtIOSCSIReq *req = opaque;
-
- virtio_scsi_complete_req(req);
-}
-
-/*
- * Called from virtio_scsi_do_one_tmf_bh() in main loop thread. The main loop
- * thread cannot touch the virtqueue since that could race with an IOThread.
- */
-static void virtio_scsi_complete_req_from_main_loop(VirtIOSCSIReq *req)
-{
- VirtIOSCSI *s = req->dev;
-
- if (!s->ctx || s->ctx == qemu_get_aio_context()) {
- /* No need to schedule a BH when there is no IOThread */
- virtio_scsi_complete_req(req);
- } else {
- /* Run request completion in the IOThread */
- aio_wait_bh_oneshot(s->ctx, virtio_scsi_complete_req_bh, req);
- }
-}
-
-static void virtio_scsi_bad_req(VirtIOSCSIReq *req)
+static void virtio_scsi_bad_req(VirtIOSCSIReq *req, QemuMutex *vq_lock)
{
virtio_error(VIRTIO_DEVICE(req->dev), "wrong size for virtio-scsi headers");
+
+ if (vq_lock) {
+ qemu_mutex_lock(vq_lock);
+ }
+
virtqueue_detach_element(req->vq, &req->elem, 0);
+
+ if (vq_lock) {
+ qemu_mutex_unlock(vq_lock);
+ }
+
virtio_scsi_free_req(req);
}
@@ -235,12 +230,21 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
return 0;
}
-static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq)
+static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq, QemuMutex *vq_lock)
{
VirtIOSCSICommon *vs = (VirtIOSCSICommon *)s;
VirtIOSCSIReq *req;
+ if (vq_lock) {
+ qemu_mutex_lock(vq_lock);
+ }
+
req = virtqueue_pop(vq, sizeof(VirtIOSCSIReq) + vs->cdb_size);
+
+ if (vq_lock) {
+ qemu_mutex_unlock(vq_lock);
+ }
+
if (!req) {
return NULL;
}
@@ -305,7 +309,7 @@ static void virtio_scsi_cancel_notify(Notifier *notifier, void *data)
trace_virtio_scsi_tmf_resp(virtio_scsi_get_lun(req->req.tmf.lun),
req->req.tmf.tag, req->resp.tmf.response);
- virtio_scsi_complete_req(req);
+ virtio_scsi_complete_req(req, &req->dev->ctrl_lock);
}
g_free(n);
}
@@ -361,7 +365,7 @@ static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req)
out:
object_unref(OBJECT(d));
- virtio_scsi_complete_req_from_main_loop(req);
+ virtio_scsi_complete_req(req, &s->ctrl_lock);
}
/* Some TMFs must be processed from the main loop thread */
@@ -408,7 +412,7 @@ static void virtio_scsi_reset_tmf_bh(VirtIOSCSI *s)
/* SAM-6 6.3.2 Hard reset */
req->resp.tmf.response = VIRTIO_SCSI_S_TARGET_FAILURE;
- virtio_scsi_complete_req(req);
+ virtio_scsi_complete_req(req, &req->dev->ctrl_lock);
}
}
@@ -562,7 +566,7 @@ static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
if (iov_to_buf(req->elem.out_sg, req->elem.out_num, 0,
&type, sizeof(type)) < sizeof(type)) {
- virtio_scsi_bad_req(req);
+ virtio_scsi_bad_req(req, &s->ctrl_lock);
return;
}
@@ -570,7 +574,7 @@ static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
if (type == VIRTIO_SCSI_T_TMF) {
if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICtrlTMFReq),
sizeof(VirtIOSCSICtrlTMFResp)) < 0) {
- virtio_scsi_bad_req(req);
+ virtio_scsi_bad_req(req, &s->ctrl_lock);
return;
} else {
r = virtio_scsi_do_tmf(s, req);
@@ -580,7 +584,7 @@ static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
type == VIRTIO_SCSI_T_AN_SUBSCRIBE) {
if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICtrlANReq),
sizeof(VirtIOSCSICtrlANResp)) < 0) {
- virtio_scsi_bad_req(req);
+ virtio_scsi_bad_req(req, &s->ctrl_lock);
return;
} else {
req->req.an.event_requested =
@@ -600,7 +604,7 @@ static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
type == VIRTIO_SCSI_T_AN_SUBSCRIBE)
trace_virtio_scsi_an_resp(virtio_scsi_get_lun(req->req.an.lun),
req->resp.an.response);
- virtio_scsi_complete_req(req);
+ virtio_scsi_complete_req(req, &s->ctrl_lock);
} else {
assert(r == -EINPROGRESS);
}
@@ -610,7 +614,7 @@ static void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq)
{
VirtIOSCSIReq *req;
- while ((req = virtio_scsi_pop_req(s, vq))) {
+ while ((req = virtio_scsi_pop_req(s, vq, &s->ctrl_lock))) {
virtio_scsi_handle_ctrl_req(s, req);
}
}
@@ -654,7 +658,7 @@ static void virtio_scsi_complete_cmd_req(VirtIOSCSIReq *req)
* in virtio_scsi_command_complete.
*/
req->resp_size = sizeof(VirtIOSCSICmdResp);
- virtio_scsi_complete_req(req);
+ virtio_scsi_complete_req(req, NULL);
}
static void virtio_scsi_command_failed(SCSIRequest *r)
@@ -788,7 +792,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
virtio_scsi_fail_cmd_req(req);
return -ENOTSUP;
} else {
- virtio_scsi_bad_req(req);
+ virtio_scsi_bad_req(req, NULL);
return -EINVAL;
}
}
@@ -843,7 +847,7 @@ static void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
virtio_queue_set_notification(vq, 0);
}
- while ((req = virtio_scsi_pop_req(s, vq))) {
+ while ((req = virtio_scsi_pop_req(s, vq, NULL))) {
ret = virtio_scsi_handle_cmd_req_prepare(s, req);
if (!ret) {
QTAILQ_INSERT_TAIL(&reqs, req, next);
@@ -973,7 +977,7 @@ static void virtio_scsi_push_event(VirtIOSCSI *s,
return;
}
- req = virtio_scsi_pop_req(s, vs->event_vq);
+ req = virtio_scsi_pop_req(s, vs->event_vq, &s->event_lock);
if (!req) {
s->events_dropped = true;
return;
@@ -985,7 +989,7 @@ static void virtio_scsi_push_event(VirtIOSCSI *s,
}
if (virtio_scsi_parse_req(req, 0, sizeof(VirtIOSCSIEvent))) {
- virtio_scsi_bad_req(req);
+ virtio_scsi_bad_req(req, &s->event_lock);
return;
}
@@ -1005,7 +1009,7 @@ static void virtio_scsi_push_event(VirtIOSCSI *s,
}
trace_virtio_scsi_event(virtio_scsi_get_lun(evt->lun), event, reason);
- virtio_scsi_complete_req(req);
+ virtio_scsi_complete_req(req, &s->event_lock);
}
static void virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq)
@@ -1236,6 +1240,8 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
Error *err = NULL;
QTAILQ_INIT(&s->tmf_bh_list);
+ qemu_mutex_init(&s->ctrl_lock);
+ qemu_mutex_init(&s->event_lock);
qemu_mutex_init(&s->tmf_bh_lock);
virtio_scsi_common_realize(dev,
@@ -1280,6 +1286,8 @@ static void virtio_scsi_device_unrealize(DeviceState *dev)
qbus_set_hotplug_handler(BUS(&s->bus), NULL);
virtio_scsi_common_unrealize(dev);
qemu_mutex_destroy(&s->tmf_bh_lock);
+ qemu_mutex_destroy(&s->event_lock);
+ qemu_mutex_destroy(&s->ctrl_lock);
}
static const Property virtio_scsi_properties[] = {
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 06/12] virtio-scsi: protect events_dropped field
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
` (4 preceding siblings ...)
2025-02-13 18:00 ` [PATCH 05/12] virtio-scsi: introduce event and ctrl virtqueue locks Stefan Hajnoczi
@ 2025-02-13 18:00 ` Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 07/12] virtio-scsi: perform TMFs in appropriate AioContexts Stefan Hajnoczi
` (7 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
The block layer can invoke the resize callback from any AioContext that
is processing requests. The virtqueue is already protected but the
events_dropped field also needs to be protected against races. Cover it
using the event virtqueue lock because it is closely associated with
accesses to the virtqueue.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/hw/virtio/virtio-scsi.h | 3 ++-
hw/scsi/virtio-scsi.c | 29 ++++++++++++++++++++---------
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 4ee98ebf63..7b7e3ced7a 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -82,10 +82,11 @@ struct VirtIOSCSI {
SCSIBus bus;
int resetting; /* written from main loop thread, read from any thread */
+
+ QemuMutex event_lock; /* protects event_vq and events_dropped */
bool events_dropped;
QemuMutex ctrl_lock; /* protects ctrl_vq */
- QemuMutex event_lock; /* protects event_vq */
/*
* TMFs deferred to main loop BH. These fields are protected by
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 073ccd3d5b..2d796a861b 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -948,7 +948,10 @@ static void virtio_scsi_reset(VirtIODevice *vdev)
vs->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
vs->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
- s->events_dropped = false;
+
+ WITH_QEMU_LOCK_GUARD(&s->event_lock) {
+ s->events_dropped = false;
+ }
}
typedef struct {
@@ -978,14 +981,16 @@ static void virtio_scsi_push_event(VirtIOSCSI *s,
}
req = virtio_scsi_pop_req(s, vs->event_vq, &s->event_lock);
- if (!req) {
- s->events_dropped = true;
- return;
- }
+ WITH_QEMU_LOCK_GUARD(&s->event_lock) {
+ if (!req) {
+ s->events_dropped = true;
+ return;
+ }
- if (s->events_dropped) {
- event |= VIRTIO_SCSI_T_EVENTS_MISSED;
- s->events_dropped = false;
+ if (s->events_dropped) {
+ event |= VIRTIO_SCSI_T_EVENTS_MISSED;
+ s->events_dropped = false;
+ }
}
if (virtio_scsi_parse_req(req, 0, sizeof(VirtIOSCSIEvent))) {
@@ -1014,7 +1019,13 @@ static void virtio_scsi_push_event(VirtIOSCSI *s,
static void virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq)
{
- if (s->events_dropped) {
+ bool events_dropped;
+
+ WITH_QEMU_LOCK_GUARD(&s->event_lock) {
+ events_dropped = s->events_dropped;
+ }
+
+ if (events_dropped) {
VirtIOSCSIEventInfo info = {
.event = VIRTIO_SCSI_T_NO_EVENT,
};
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 07/12] virtio-scsi: perform TMFs in appropriate AioContexts
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
` (5 preceding siblings ...)
2025-02-13 18:00 ` [PATCH 06/12] virtio-scsi: protect events_dropped field Stefan Hajnoczi
@ 2025-02-13 18:00 ` Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 08/12] virtio-blk: extract cleanup_iothread_vq_mapping() function Stefan Hajnoczi
` (6 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
With IOThread Virtqueue Mapping there will be multiple AioContexts
processing SCSI requests. scsi_req_cancel() and other SCSI request
operations must be performed from the AioContext where the request is
running.
Introduce a virtio_scsi_defer_tmf_to_aio_context() function and the
necessary VirtIOSCSIReq->remaining refcount infrastructure to move the
TMF code into the AioContext where the request is running.
For the time being there is still just one AioContext: the main loop or
the IOThread. When the iothread-vq-mapping parameter is added in a later
patch this will be changed to per-virtqueue AioContexts.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/scsi/virtio-scsi.c | 296 +++++++++++++++++++++++++++++++-----------
1 file changed, 219 insertions(+), 77 deletions(-)
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 2d796a861b..2045d27289 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -47,7 +47,7 @@ typedef struct VirtIOSCSIReq {
/* Used for two-stage request submission and TMFs deferred to BH */
QTAILQ_ENTRY(VirtIOSCSIReq) next;
- /* Used for cancellation of request during TMFs */
+ /* Used for cancellation of request during TMFs. Atomic. */
int remaining;
SCSIRequest *sreq;
@@ -298,19 +298,23 @@ typedef struct {
VirtIOSCSIReq *tmf_req;
} VirtIOSCSICancelNotifier;
+static void virtio_scsi_tmf_dec_remaining(VirtIOSCSIReq *tmf)
+{
+ if (qatomic_fetch_dec(&tmf->remaining) == 1) {
+ trace_virtio_scsi_tmf_resp(virtio_scsi_get_lun(tmf->req.tmf.lun),
+ tmf->req.tmf.tag, tmf->resp.tmf.response);
+
+ virtio_scsi_complete_req(tmf, &tmf->dev->ctrl_lock);
+ }
+}
+
static void virtio_scsi_cancel_notify(Notifier *notifier, void *data)
{
VirtIOSCSICancelNotifier *n = container_of(notifier,
VirtIOSCSICancelNotifier,
notifier);
- if (--n->tmf_req->remaining == 0) {
- VirtIOSCSIReq *req = n->tmf_req;
-
- trace_virtio_scsi_tmf_resp(virtio_scsi_get_lun(req->req.tmf.lun),
- req->req.tmf.tag, req->resp.tmf.response);
- virtio_scsi_complete_req(req, &req->dev->ctrl_lock);
- }
+ virtio_scsi_tmf_dec_remaining(n->tmf_req);
g_free(n);
}
@@ -416,7 +420,7 @@ static void virtio_scsi_reset_tmf_bh(VirtIOSCSI *s)
}
}
-static void virtio_scsi_defer_tmf_to_bh(VirtIOSCSIReq *req)
+static void virtio_scsi_defer_tmf_to_main_loop(VirtIOSCSIReq *req)
{
VirtIOSCSI *s = req->dev;
@@ -430,6 +434,137 @@ static void virtio_scsi_defer_tmf_to_bh(VirtIOSCSIReq *req)
}
}
+static void virtio_scsi_tmf_cancel_req(VirtIOSCSIReq *tmf, SCSIRequest *r)
+{
+ VirtIOSCSICancelNotifier *notifier;
+
+ assert(r->ctx == qemu_get_current_aio_context());
+
+ /* Decremented in virtio_scsi_cancel_notify() */
+ qatomic_inc(&tmf->remaining);
+
+ notifier = g_new(VirtIOSCSICancelNotifier, 1);
+ notifier->notifier.notify = virtio_scsi_cancel_notify;
+ notifier->tmf_req = tmf;
+ scsi_req_cancel_async(r, ¬ifier->notifier);
+}
+
+/* Execute a TMF on the requests in the current AioContext */
+static void virtio_scsi_do_tmf_aio_context(void *opaque)
+{
+ AioContext *ctx = qemu_get_current_aio_context();
+ VirtIOSCSIReq *tmf = opaque;
+ VirtIOSCSI *s = tmf->dev;
+ SCSIDevice *d = virtio_scsi_device_get(s, tmf->req.tmf.lun);
+ SCSIRequest *r;
+ bool match_tag;
+
+ if (!d) {
+ tmf->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
+ virtio_scsi_tmf_dec_remaining(tmf);
+ return;
+ }
+
+ /*
+ * This function could handle other subtypes that need to be processed in
+ * the request's AioContext in the future, but for now only request
+ * cancelation subtypes are performed here.
+ */
+ switch (tmf->req.tmf.subtype) {
+ case VIRTIO_SCSI_T_TMF_ABORT_TASK:
+ match_tag = true;
+ break;
+ case VIRTIO_SCSI_T_TMF_ABORT_TASK_SET:
+ case VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET:
+ match_tag = false;
+ break;
+ default:
+ g_assert_not_reached();
+ }
+
+ WITH_QEMU_LOCK_GUARD(&d->requests_lock) {
+ QTAILQ_FOREACH(r, &d->requests, next) {
+ VirtIOSCSIReq *cmd_req = r->hba_private;
+ assert(cmd_req); /* request has hba_private while enqueued */
+
+ if (r->ctx != ctx) {
+ continue;
+ }
+ if (match_tag && cmd_req->req.cmd.tag != tmf->req.tmf.tag) {
+ continue;
+ }
+ virtio_scsi_tmf_cancel_req(tmf, r);
+ }
+ }
+
+ /* Incremented by virtio_scsi_do_tmf() */
+ virtio_scsi_tmf_dec_remaining(tmf);
+
+ object_unref(d);
+}
+
+static void dummy_bh(void *opaque)
+{
+ /* Do nothing */
+}
+
+/*
+ * Wait for pending virtio_scsi_defer_tmf_to_aio_context() BHs.
+ */
+static void virtio_scsi_flush_defer_tmf_to_aio_context(VirtIOSCSI *s)
+{
+ GLOBAL_STATE_CODE();
+
+ assert(!s->dataplane_started);
+
+ if (s->ctx) {
+ /* Our BH only runs after previously scheduled BHs */
+ aio_wait_bh_oneshot(s->ctx, dummy_bh, NULL);
+ }
+}
+
+/*
+ * Run the TMF in a specific AioContext, handling only requests in that
+ * AioContext. This is necessary because requests can run in different
+ * AioContext and it is only possible to cancel them from the AioContext where
+ * they are running.
+ */
+static void virtio_scsi_defer_tmf_to_aio_context(VirtIOSCSIReq *tmf,
+ AioContext *ctx)
+{
+ /* Decremented in virtio_scsi_do_tmf_aio_context() */
+ qatomic_inc(&tmf->remaining);
+
+ /* See virtio_scsi_flush_defer_tmf_to_aio_context() cleanup during reset */
+ aio_bh_schedule_oneshot(ctx, virtio_scsi_do_tmf_aio_context, tmf);
+}
+
+/*
+ * Returns the AioContext for a given TMF's tag field or NULL. Note that the
+ * request identified by the tag may have completed by the time you can execute
+ * a BH in the AioContext, so don't assume the request still exists in your BH.
+ */
+static AioContext *find_aio_context_for_tmf_tag(SCSIDevice *d,
+ VirtIOSCSIReq *tmf)
+{
+ WITH_QEMU_LOCK_GUARD(&d->requests_lock) {
+ SCSIRequest *r;
+ SCSIRequest *next;
+
+ QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
+ VirtIOSCSIReq *cmd_req = r->hba_private;
+
+ /* hba_private is non-NULL while the request is enqueued */
+ assert(cmd_req);
+
+ if (cmd_req->req.cmd.tag == tmf->req.tmf.tag) {
+ return r->ctx;
+ }
+ }
+ }
+ return NULL;
+}
+
/* Return 0 if the request is ready to be completed and return to guest;
* -EINPROGRESS if the request is submitted and will be completed later, in the
* case of async cancellation. */
@@ -437,6 +572,7 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
{
SCSIDevice *d = virtio_scsi_device_get(s, req->req.tmf.lun);
SCSIRequest *r, *next;
+ AioContext *ctx;
int ret = 0;
virtio_scsi_ctx_check(s, d);
@@ -454,7 +590,22 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
req->req.tmf.tag, req->req.tmf.subtype);
switch (req->req.tmf.subtype) {
- case VIRTIO_SCSI_T_TMF_ABORT_TASK:
+ case VIRTIO_SCSI_T_TMF_ABORT_TASK: {
+ if (!d) {
+ goto fail;
+ }
+ if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
+ goto incorrect_lun;
+ }
+
+ ctx = find_aio_context_for_tmf_tag(d, req);
+ if (ctx) {
+ virtio_scsi_defer_tmf_to_aio_context(req, ctx);
+ ret = -EINPROGRESS;
+ }
+ break;
+ }
+
case VIRTIO_SCSI_T_TMF_QUERY_TASK:
if (!d) {
goto fail;
@@ -462,82 +613,72 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
goto incorrect_lun;
}
- QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
- VirtIOSCSIReq *cmd_req = r->hba_private;
- if (cmd_req && cmd_req->req.cmd.tag == req->req.tmf.tag) {
- break;
+
+ WITH_QEMU_LOCK_GUARD(&d->requests_lock) {
+ QTAILQ_FOREACH(r, &d->requests, next) {
+ VirtIOSCSIReq *cmd_req = r->hba_private;
+ assert(cmd_req); /* request has hba_private while enqueued */
+
+ if (cmd_req->req.cmd.tag == req->req.tmf.tag) {
+ /*
+ * "If the specified command is present in the task set,
+ * then return a service response set to FUNCTION
+ * SUCCEEDED".
+ */
+ req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
+ }
}
}
- if (r) {
- /*
- * Assert that the request has not been completed yet, we
- * check for it in the loop above.
- */
- assert(r->hba_private);
- if (req->req.tmf.subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK) {
- /* "If the specified command is present in the task set, then
+ break;
+
+ case VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET:
+ case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
+ virtio_scsi_defer_tmf_to_main_loop(req);
+ ret = -EINPROGRESS;
+ break;
+
+ case VIRTIO_SCSI_T_TMF_ABORT_TASK_SET:
+ case VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET: {
+ if (!d) {
+ goto fail;
+ }
+ if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
+ goto incorrect_lun;
+ }
+
+ qatomic_inc(&req->remaining);
+
+ ctx = s->ctx ?: qemu_get_aio_context();
+ virtio_scsi_defer_tmf_to_aio_context(req, ctx);
+
+ virtio_scsi_tmf_dec_remaining(req);
+ ret = -EINPROGRESS;
+ break;
+ }
+
+ case VIRTIO_SCSI_T_TMF_QUERY_TASK_SET:
+ if (!d) {
+ goto fail;
+ }
+ if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
+ goto incorrect_lun;
+ }
+
+ WITH_QEMU_LOCK_GUARD(&d->requests_lock) {
+ QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
+ /* Request has hba_private while enqueued */
+ assert(r->hba_private);
+
+ /*
+ * "If there is any command present in the task set, then
* return a service response set to FUNCTION SUCCEEDED".
*/
req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
- } else {
- VirtIOSCSICancelNotifier *notifier;
-
- req->remaining = 1;
- notifier = g_new(VirtIOSCSICancelNotifier, 1);
- notifier->tmf_req = req;
- notifier->notifier.notify = virtio_scsi_cancel_notify;
- scsi_req_cancel_async(r, ¬ifier->notifier);
- ret = -EINPROGRESS;
+ break;
}
}
break;
- case VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET:
- case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
- virtio_scsi_defer_tmf_to_bh(req);
- ret = -EINPROGRESS;
- break;
-
- case VIRTIO_SCSI_T_TMF_ABORT_TASK_SET:
- case VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET:
- case VIRTIO_SCSI_T_TMF_QUERY_TASK_SET:
- if (!d) {
- goto fail;
- }
- if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
- goto incorrect_lun;
- }
-
- /* Add 1 to "remaining" until virtio_scsi_do_tmf returns.
- * This way, if the bus starts calling back to the notifiers
- * even before we finish the loop, virtio_scsi_cancel_notify
- * will not complete the TMF too early.
- */
- req->remaining = 1;
- QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
- if (r->hba_private) {
- if (req->req.tmf.subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK_SET) {
- /* "If there is any command present in the task set, then
- * return a service response set to FUNCTION SUCCEEDED".
- */
- req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
- break;
- } else {
- VirtIOSCSICancelNotifier *notifier;
-
- req->remaining++;
- notifier = g_new(VirtIOSCSICancelNotifier, 1);
- notifier->notifier.notify = virtio_scsi_cancel_notify;
- notifier->tmf_req = req;
- scsi_req_cancel_async(r, ¬ifier->notifier);
- }
- }
- }
- if (--req->remaining > 0) {
- ret = -EINPROGRESS;
- }
- break;
-
case VIRTIO_SCSI_T_TMF_CLEAR_ACA:
default:
req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_REJECTED;
@@ -941,6 +1082,7 @@ static void virtio_scsi_reset(VirtIODevice *vdev)
assert(!s->dataplane_started);
virtio_scsi_reset_tmf_bh(s);
+ virtio_scsi_flush_defer_tmf_to_aio_context(s);
qatomic_inc(&s->resetting);
bus_cold_reset(BUS(&s->bus));
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 08/12] virtio-blk: extract cleanup_iothread_vq_mapping() function
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
` (6 preceding siblings ...)
2025-02-13 18:00 ` [PATCH 07/12] virtio-scsi: perform TMFs in appropriate AioContexts Stefan Hajnoczi
@ 2025-02-13 18:00 ` Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 09/12] virtio-blk: tidy up iothread_vq_mapping functions Stefan Hajnoczi
` (5 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
This is the cleanup function that must be called after
apply_iothread_vq_mapping() succeeds. virtio-scsi will need this
function too, so extract it.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/block/virtio-blk.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index a1829e3abd..9af8c51af0 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1485,6 +1485,9 @@ validate_iothread_vq_mapping_list(IOThreadVirtQueueMappingList *list,
* Fill in the AioContext for each virtqueue in the @vq_aio_context array given
* the iothread-vq-mapping parameter in @iothread_vq_mapping_list.
*
+ * cleanup_iothread_vq_mapping() must be called to free IOThread object
+ * references after this function returns success.
+ *
* Returns: %true on success, %false on failure.
**/
static bool apply_iothread_vq_mapping(
@@ -1535,6 +1538,23 @@ static bool apply_iothread_vq_mapping(
return true;
}
+/**
+ * cleanup_iothread_vq_mapping:
+ * @list: The mapping of virtqueues to IOThreads.
+ *
+ * Release IOThread object references that were acquired by
+ * apply_iothread_vq_mapping().
+ */
+static void cleanup_iothread_vq_mapping(IOThreadVirtQueueMappingList *list)
+{
+ IOThreadVirtQueueMappingList *node;
+
+ for (node = list; node; node = node->next) {
+ IOThread *iothread = iothread_by_id(node->value->iothread);
+ object_unref(OBJECT(iothread));
+ }
+}
+
/* Context: BQL held */
static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
{
@@ -1601,12 +1621,7 @@ static void virtio_blk_vq_aio_context_cleanup(VirtIOBlock *s)
assert(!s->ioeventfd_started);
if (conf->iothread_vq_mapping_list) {
- IOThreadVirtQueueMappingList *node;
-
- for (node = conf->iothread_vq_mapping_list; node; node = node->next) {
- IOThread *iothread = iothread_by_id(node->value->iothread);
- object_unref(OBJECT(iothread));
- }
+ cleanup_iothread_vq_mapping(conf->iothread_vq_mapping_list);
}
if (conf->iothread) {
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 09/12] virtio-blk: tidy up iothread_vq_mapping functions
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
` (7 preceding siblings ...)
2025-02-13 18:00 ` [PATCH 08/12] virtio-blk: extract cleanup_iothread_vq_mapping() function Stefan Hajnoczi
@ 2025-02-13 18:00 ` Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 10/12] virtio: extract iothread-vq-mapping.h API Stefan Hajnoczi
` (4 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
Use noun_verb() function naming instead of verb_noun() because the
former is the most common naming style for APIs. The next commit will
move these functions into a header file so that virtio-scsi can call
them.
Shorten iothread_vq_mapping_apply()'s iothread_vq_mapping_list argument
to just "list" like in the other functions.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/block/virtio-blk.c | 33 ++++++++++++++++-----------------
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 9af8c51af0..6464a305d0 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1414,8 +1414,8 @@ static const BlockDevOps virtio_block_ops = {
};
static bool
-validate_iothread_vq_mapping_list(IOThreadVirtQueueMappingList *list,
- uint16_t num_queues, Error **errp)
+iothread_vq_mapping_validate(IOThreadVirtQueueMappingList *list, uint16_t
+ num_queues, Error **errp)
{
g_autofree unsigned long *vqs = bitmap_new(num_queues);
g_autoptr(GHashTable) iothreads =
@@ -1476,22 +1476,22 @@ validate_iothread_vq_mapping_list(IOThreadVirtQueueMappingList *list,
}
/**
- * apply_iothread_vq_mapping:
- * @iothread_vq_mapping_list: The mapping of virtqueues to IOThreads.
+ * iothread_vq_mapping_apply:
+ * @list: The mapping of virtqueues to IOThreads.
* @vq_aio_context: The array of AioContext pointers to fill in.
* @num_queues: The length of @vq_aio_context.
* @errp: If an error occurs, a pointer to the area to store the error.
*
* Fill in the AioContext for each virtqueue in the @vq_aio_context array given
- * the iothread-vq-mapping parameter in @iothread_vq_mapping_list.
+ * the iothread-vq-mapping parameter in @list.
*
- * cleanup_iothread_vq_mapping() must be called to free IOThread object
+ * iothread_vq_mapping_cleanup() must be called to free IOThread object
* references after this function returns success.
*
* Returns: %true on success, %false on failure.
**/
-static bool apply_iothread_vq_mapping(
- IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
+static bool iothread_vq_mapping_apply(
+ IOThreadVirtQueueMappingList *list,
AioContext **vq_aio_context,
uint16_t num_queues,
Error **errp)
@@ -1500,16 +1500,15 @@ static bool apply_iothread_vq_mapping(
size_t num_iothreads = 0;
size_t cur_iothread = 0;
- if (!validate_iothread_vq_mapping_list(iothread_vq_mapping_list,
- num_queues, errp)) {
+ if (!iothread_vq_mapping_validate(list, num_queues, errp)) {
return false;
}
- for (node = iothread_vq_mapping_list; node; node = node->next) {
+ for (node = list; node; node = node->next) {
num_iothreads++;
}
- for (node = iothread_vq_mapping_list; node; node = node->next) {
+ for (node = list; node; node = node->next) {
IOThread *iothread = iothread_by_id(node->value->iothread);
AioContext *ctx = iothread_get_aio_context(iothread);
@@ -1539,13 +1538,13 @@ static bool apply_iothread_vq_mapping(
}
/**
- * cleanup_iothread_vq_mapping:
+ * iothread_vq_mapping_cleanup:
* @list: The mapping of virtqueues to IOThreads.
*
* Release IOThread object references that were acquired by
- * apply_iothread_vq_mapping().
+ * iothread_vq_mapping_apply().
*/
-static void cleanup_iothread_vq_mapping(IOThreadVirtQueueMappingList *list)
+static void iothread_vq_mapping_cleanup(IOThreadVirtQueueMappingList *list)
{
IOThreadVirtQueueMappingList *node;
@@ -1587,7 +1586,7 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
s->vq_aio_context = g_new(AioContext *, conf->num_queues);
if (conf->iothread_vq_mapping_list) {
- if (!apply_iothread_vq_mapping(conf->iothread_vq_mapping_list,
+ if (!iothread_vq_mapping_apply(conf->iothread_vq_mapping_list,
s->vq_aio_context,
conf->num_queues,
errp)) {
@@ -1621,7 +1620,7 @@ static void virtio_blk_vq_aio_context_cleanup(VirtIOBlock *s)
assert(!s->ioeventfd_started);
if (conf->iothread_vq_mapping_list) {
- cleanup_iothread_vq_mapping(conf->iothread_vq_mapping_list);
+ iothread_vq_mapping_cleanup(conf->iothread_vq_mapping_list);
}
if (conf->iothread) {
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 10/12] virtio: extract iothread-vq-mapping.h API
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
` (8 preceding siblings ...)
2025-02-13 18:00 ` [PATCH 09/12] virtio-blk: tidy up iothread_vq_mapping functions Stefan Hajnoczi
@ 2025-02-13 18:00 ` Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 11/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
` (3 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
The code that builds an array of AioContext pointers indexed by the
virtqueue is not specific to virtio-blk. virtio-scsi will need to do the
same thing, so extract the functions.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/hw/virtio/iothread-vq-mapping.h | 45 ++++++++
hw/block/virtio-blk.c | 142 +-----------------------
hw/virtio/iothread-vq-mapping.c | 131 ++++++++++++++++++++++
hw/virtio/meson.build | 1 +
4 files changed, 178 insertions(+), 141 deletions(-)
create mode 100644 include/hw/virtio/iothread-vq-mapping.h
create mode 100644 hw/virtio/iothread-vq-mapping.c
diff --git a/include/hw/virtio/iothread-vq-mapping.h b/include/hw/virtio/iothread-vq-mapping.h
new file mode 100644
index 0000000000..57335c3703
--- /dev/null
+++ b/include/hw/virtio/iothread-vq-mapping.h
@@ -0,0 +1,45 @@
+/*
+ * IOThread Virtqueue Mapping
+ *
+ * Copyright Red Hat, Inc
+ *
+ * SPDX-License-Identifier: GPL-2.0-only
+ */
+
+#ifndef HW_VIRTIO_IOTHREAD_VQ_MAPPING_H
+#define HW_VIRTIO_IOTHREAD_VQ_MAPPING_H
+
+#include "qapi/error.h"
+#include "qapi/qapi-types-virtio.h"
+
+/**
+ * iothread_vq_mapping_apply:
+ * @list: The mapping of virtqueues to IOThreads.
+ * @vq_aio_context: The array of AioContext pointers to fill in.
+ * @num_queues: The length of @vq_aio_context.
+ * @errp: If an error occurs, a pointer to the area to store the error.
+ *
+ * Fill in the AioContext for each virtqueue in the @vq_aio_context array given
+ * the iothread-vq-mapping parameter in @list.
+ *
+ * iothread_vq_mapping_cleanup() must be called to free IOThread object
+ * references after this function returns success.
+ *
+ * Returns: %true on success, %false on failure.
+ **/
+bool iothread_vq_mapping_apply(
+ IOThreadVirtQueueMappingList *list,
+ AioContext **vq_aio_context,
+ uint16_t num_queues,
+ Error **errp);
+
+/**
+ * iothread_vq_mapping_cleanup:
+ * @list: The mapping of virtqueues to IOThreads.
+ *
+ * Release IOThread object references that were acquired by
+ * iothread_vq_mapping_apply().
+ */
+void iothread_vq_mapping_cleanup(IOThreadVirtQueueMappingList *list);
+
+#endif /* HW_VIRTIO_IOTHREAD_VQ_MAPPING_H */
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6464a305d0..04f99d2d22 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -33,6 +33,7 @@
#endif
#include "hw/virtio/virtio-bus.h"
#include "migration/qemu-file-types.h"
+#include "hw/virtio/iothread-vq-mapping.h"
#include "hw/virtio/virtio-access.h"
#include "hw/virtio/virtio-blk-common.h"
#include "qemu/coroutine.h"
@@ -1413,147 +1414,6 @@ static const BlockDevOps virtio_block_ops = {
.drained_end = virtio_blk_drained_end,
};
-static bool
-iothread_vq_mapping_validate(IOThreadVirtQueueMappingList *list, uint16_t
- num_queues, Error **errp)
-{
- g_autofree unsigned long *vqs = bitmap_new(num_queues);
- g_autoptr(GHashTable) iothreads =
- g_hash_table_new(g_str_hash, g_str_equal);
-
- for (IOThreadVirtQueueMappingList *node = list; node; node = node->next) {
- const char *name = node->value->iothread;
- uint16List *vq;
-
- if (!iothread_by_id(name)) {
- error_setg(errp, "IOThread \"%s\" object does not exist", name);
- return false;
- }
-
- if (!g_hash_table_add(iothreads, (gpointer)name)) {
- error_setg(errp,
- "duplicate IOThread name \"%s\" in iothread-vq-mapping",
- name);
- return false;
- }
-
- if (node != list) {
- if (!!node->value->vqs != !!list->value->vqs) {
- error_setg(errp, "either all items in iothread-vq-mapping "
- "must have vqs or none of them must have it");
- return false;
- }
- }
-
- for (vq = node->value->vqs; vq; vq = vq->next) {
- if (vq->value >= num_queues) {
- error_setg(errp, "vq index %u for IOThread \"%s\" must be "
- "less than num_queues %u in iothread-vq-mapping",
- vq->value, name, num_queues);
- return false;
- }
-
- if (test_and_set_bit(vq->value, vqs)) {
- error_setg(errp, "cannot assign vq %u to IOThread \"%s\" "
- "because it is already assigned", vq->value, name);
- return false;
- }
- }
- }
-
- if (list->value->vqs) {
- for (uint16_t i = 0; i < num_queues; i++) {
- if (!test_bit(i, vqs)) {
- error_setg(errp,
- "missing vq %u IOThread assignment in iothread-vq-mapping",
- i);
- return false;
- }
- }
- }
-
- return true;
-}
-
-/**
- * iothread_vq_mapping_apply:
- * @list: The mapping of virtqueues to IOThreads.
- * @vq_aio_context: The array of AioContext pointers to fill in.
- * @num_queues: The length of @vq_aio_context.
- * @errp: If an error occurs, a pointer to the area to store the error.
- *
- * Fill in the AioContext for each virtqueue in the @vq_aio_context array given
- * the iothread-vq-mapping parameter in @list.
- *
- * iothread_vq_mapping_cleanup() must be called to free IOThread object
- * references after this function returns success.
- *
- * Returns: %true on success, %false on failure.
- **/
-static bool iothread_vq_mapping_apply(
- IOThreadVirtQueueMappingList *list,
- AioContext **vq_aio_context,
- uint16_t num_queues,
- Error **errp)
-{
- IOThreadVirtQueueMappingList *node;
- size_t num_iothreads = 0;
- size_t cur_iothread = 0;
-
- if (!iothread_vq_mapping_validate(list, num_queues, errp)) {
- return false;
- }
-
- for (node = list; node; node = node->next) {
- num_iothreads++;
- }
-
- for (node = list; node; node = node->next) {
- IOThread *iothread = iothread_by_id(node->value->iothread);
- AioContext *ctx = iothread_get_aio_context(iothread);
-
- /* Released in virtio_blk_vq_aio_context_cleanup() */
- object_ref(OBJECT(iothread));
-
- if (node->value->vqs) {
- uint16List *vq;
-
- /* Explicit vq:IOThread assignment */
- for (vq = node->value->vqs; vq; vq = vq->next) {
- assert(vq->value < num_queues);
- vq_aio_context[vq->value] = ctx;
- }
- } else {
- /* Round-robin vq:IOThread assignment */
- for (unsigned i = cur_iothread; i < num_queues;
- i += num_iothreads) {
- vq_aio_context[i] = ctx;
- }
- }
-
- cur_iothread++;
- }
-
- return true;
-}
-
-/**
- * iothread_vq_mapping_cleanup:
- * @list: The mapping of virtqueues to IOThreads.
- *
- * Release IOThread object references that were acquired by
- * iothread_vq_mapping_apply().
- */
-static void iothread_vq_mapping_cleanup(IOThreadVirtQueueMappingList *list)
-{
- IOThreadVirtQueueMappingList *node;
-
- for (node = list; node; node = node->next) {
- IOThread *iothread = iothread_by_id(node->value->iothread);
- object_unref(OBJECT(iothread));
- }
-}
-
/* Context: BQL held */
static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
{
diff --git a/hw/virtio/iothread-vq-mapping.c b/hw/virtio/iothread-vq-mapping.c
new file mode 100644
index 0000000000..15909eb933
--- /dev/null
+++ b/hw/virtio/iothread-vq-mapping.c
@@ -0,0 +1,131 @@
+/*
+ * IOThread Virtqueue Mapping
+ *
+ * Copyright Red Hat, Inc
+ *
+ * SPDX-License-Identifier: GPL-2.0-only
+ */
+
+#include "qemu/osdep.h"
+#include "system/iothread.h"
+#include "hw/virtio/iothread-vq-mapping.h"
+
+static bool
+iothread_vq_mapping_validate(IOThreadVirtQueueMappingList *list, uint16_t
+ num_queues, Error **errp)
+{
+ g_autofree unsigned long *vqs = bitmap_new(num_queues);
+ g_autoptr(GHashTable) iothreads =
+ g_hash_table_new(g_str_hash, g_str_equal);
+
+ for (IOThreadVirtQueueMappingList *node = list; node; node = node->next) {
+ const char *name = node->value->iothread;
+ uint16List *vq;
+
+ if (!iothread_by_id(name)) {
+ error_setg(errp, "IOThread \"%s\" object does not exist", name);
+ return false;
+ }
+
+ if (!g_hash_table_add(iothreads, (gpointer)name)) {
+ error_setg(errp,
+ "duplicate IOThread name \"%s\" in iothread-vq-mapping",
+ name);
+ return false;
+ }
+
+ if (node != list) {
+ if (!!node->value->vqs != !!list->value->vqs) {
+ error_setg(errp, "either all items in iothread-vq-mapping "
+ "must have vqs or none of them must have it");
+ return false;
+ }
+ }
+
+ for (vq = node->value->vqs; vq; vq = vq->next) {
+ if (vq->value >= num_queues) {
+ error_setg(errp, "vq index %u for IOThread \"%s\" must be "
+ "less than num_queues %u in iothread-vq-mapping",
+ vq->value, name, num_queues);
+ return false;
+ }
+
+ if (test_and_set_bit(vq->value, vqs)) {
+ error_setg(errp, "cannot assign vq %u to IOThread \"%s\" "
+ "because it is already assigned", vq->value, name);
+ return false;
+ }
+ }
+ }
+
+ if (list->value->vqs) {
+ for (uint16_t i = 0; i < num_queues; i++) {
+ if (!test_bit(i, vqs)) {
+ error_setg(errp,
+ "missing vq %u IOThread assignment in iothread-vq-mapping",
+ i);
+ return false;
+ }
+ }
+ }
+
+ return true;
+}
+
+bool iothread_vq_mapping_apply(
+ IOThreadVirtQueueMappingList *list,
+ AioContext **vq_aio_context,
+ uint16_t num_queues,
+ Error **errp)
+{
+ IOThreadVirtQueueMappingList *node;
+ size_t num_iothreads = 0;
+ size_t cur_iothread = 0;
+
+ if (!iothread_vq_mapping_validate(list, num_queues, errp)) {
+ return false;
+ }
+
+ for (node = list; node; node = node->next) {
+ num_iothreads++;
+ }
+
+ for (node = list; node; node = node->next) {
+ IOThread *iothread = iothread_by_id(node->value->iothread);
+ AioContext *ctx = iothread_get_aio_context(iothread);
+
+ /* Released in virtio_blk_vq_aio_context_cleanup() */
+ object_ref(OBJECT(iothread));
+
+ if (node->value->vqs) {
+ uint16List *vq;
+
+ /* Explicit vq:IOThread assignment */
+ for (vq = node->value->vqs; vq; vq = vq->next) {
+ assert(vq->value < num_queues);
+ vq_aio_context[vq->value] = ctx;
+ }
+ } else {
+ /* Round-robin vq:IOThread assignment */
+ for (unsigned i = cur_iothread; i < num_queues;
+ i += num_iothreads) {
+ vq_aio_context[i] = ctx;
+ }
+ }
+
+ cur_iothread++;
+ }
+
+ return true;
+}
+
+void iothread_vq_mapping_cleanup(IOThreadVirtQueueMappingList *list)
+{
+ IOThreadVirtQueueMappingList *node;
+
+ for (node = list; node; node = node->next) {
+ IOThread *iothread = iothread_by_id(node->value->iothread);
+ object_unref(OBJECT(iothread));
+ }
+}
+
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index a5f9f7999d..19b04c4d9c 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -1,5 +1,6 @@
system_virtio_ss = ss.source_set()
system_virtio_ss.add(files('virtio-bus.c'))
+system_virtio_ss.add(files('iothread-vq-mapping.c'))
system_virtio_ss.add(when: 'CONFIG_VIRTIO_PCI', if_true: files('virtio-pci.c'))
system_virtio_ss.add(when: 'CONFIG_VIRTIO_MMIO', if_true: files('virtio-mmio.c'))
system_virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('virtio-crypto.c'))
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 11/12] virtio-scsi: add iothread-vq-mapping parameter
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
` (9 preceding siblings ...)
2025-02-13 18:00 ` [PATCH 10/12] virtio: extract iothread-vq-mapping.h API Stefan Hajnoczi
@ 2025-02-13 18:00 ` Stefan Hajnoczi
2025-03-10 14:33 ` Kevin Wolf
2025-02-13 18:00 ` [PATCH 12/12] virtio-scsi: handle ctrl virtqueue in main loop Stefan Hajnoczi
` (2 subsequent siblings)
13 siblings, 1 reply; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
Allow virtio-scsi virtqueues to be assigned to different IOThreads. This
makes it possible to take advantage of host multi-queue block layer
scalability by assigning virtqueues that have affinity with vCPUs to
different IOThreads that have affinity with host CPUs. The same feature
was introduced for virtio-blk in the past:
https://developers.redhat.com/articles/2024/09/05/scaling-virtio-blk-disk-io-iothread-virtqueue-mapping
Here are fio randread 4k iodepth=64 results from a 4 vCPU guest with an
Intel P4800X SSD:
iothreads IOPS
------------------------------
1 189576
2 312698
4 346744
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/hw/virtio/virtio-scsi.h | 5 +-
hw/scsi/virtio-scsi-dataplane.c | 90 ++++++++++++++++++++++++---------
hw/scsi/virtio-scsi.c | 63 ++++++++++++++---------
3 files changed, 107 insertions(+), 51 deletions(-)
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 7b7e3ced7a..086201efa2 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -22,6 +22,7 @@
#include "hw/virtio/virtio.h"
#include "hw/scsi/scsi.h"
#include "chardev/char-fe.h"
+#include "qapi/qapi-types-virtio.h"
#include "system/iothread.h"
#define TYPE_VIRTIO_SCSI_COMMON "virtio-scsi-common"
@@ -60,6 +61,7 @@ struct VirtIOSCSIConf {
CharBackend chardev;
uint32_t boot_tpgt;
IOThread *iothread;
+ IOThreadVirtQueueMappingList *iothread_vq_mapping_list;
};
struct VirtIOSCSI;
@@ -97,7 +99,7 @@ struct VirtIOSCSI {
QTAILQ_HEAD(, VirtIOSCSIReq) tmf_bh_list;
/* Fields for dataplane below */
- AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
+ AioContext **vq_aio_context; /* per-virtqueue AioContext pointer */
bool dataplane_started;
bool dataplane_starting;
@@ -115,6 +117,7 @@ void virtio_scsi_common_realize(DeviceState *dev,
void virtio_scsi_common_unrealize(DeviceState *dev);
void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp);
+void virtio_scsi_dataplane_cleanup(VirtIOSCSI *s);
int virtio_scsi_dataplane_start(VirtIODevice *s);
void virtio_scsi_dataplane_stop(VirtIODevice *s);
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index f49ab98ecc..6bb368c8a5 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -18,6 +18,7 @@
#include "system/block-backend.h"
#include "hw/scsi/scsi.h"
#include "scsi/constants.h"
+#include "hw/virtio/iothread-vq-mapping.h"
#include "hw/virtio/virtio-bus.h"
/* Context: BQL held */
@@ -27,8 +28,16 @@ void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp)
VirtIODevice *vdev = VIRTIO_DEVICE(s);
BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+ uint16_t num_vqs = vs->conf.num_queues + VIRTIO_SCSI_VQ_NUM_FIXED;
- if (vs->conf.iothread) {
+ if (vs->conf.iothread && vs->conf.iothread_vq_mapping_list) {
+ error_setg(errp,
+ "iothread and iothread-vq-mapping properties cannot be set "
+ "at the same time");
+ return;
+ }
+
+ if (vs->conf.iothread || vs->conf.iothread_vq_mapping_list) {
if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
error_setg(errp,
"device is incompatible with iothread "
@@ -39,15 +48,50 @@ void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp)
error_setg(errp, "ioeventfd is required for iothread");
return;
}
- s->ctx = iothread_get_aio_context(vs->conf.iothread);
+ }
+
+ s->vq_aio_context = g_new(AioContext *, num_vqs);
+
+ if (vs->conf.iothread_vq_mapping_list) {
+ if (!iothread_vq_mapping_apply(vs->conf.iothread_vq_mapping_list,
+ s->vq_aio_context, num_vqs, errp)) {
+ g_free(s->vq_aio_context);
+ s->vq_aio_context = NULL;
+ return;
+ }
+ } else if (vs->conf.iothread) {
+ AioContext *ctx = iothread_get_aio_context(vs->conf.iothread);
+ for (uint16_t i = 0; i < num_vqs; i++) {
+ s->vq_aio_context[i] = ctx;
+ }
+
+ /* Released in virtio_scsi_dataplane_cleanup() */
+ object_ref(OBJECT(vs->conf.iothread));
} else {
- if (!virtio_device_ioeventfd_enabled(vdev)) {
- return;
+ AioContext *ctx = qemu_get_aio_context();
+ for (unsigned i = 0; i < num_vqs; i++) {
+ s->vq_aio_context[i] = ctx;
}
- s->ctx = qemu_get_aio_context();
}
}
+/* Context: BQL held */
+void virtio_scsi_dataplane_cleanup(VirtIOSCSI *s)
+{
+ VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
+
+ if (vs->conf.iothread_vq_mapping_list) {
+ iothread_vq_mapping_cleanup(vs->conf.iothread_vq_mapping_list);
+ }
+
+ if (vs->conf.iothread) {
+ object_unref(OBJECT(vs->conf.iothread));
+ }
+
+ g_free(s->vq_aio_context);
+ s->vq_aio_context = NULL;
+}
+
static int virtio_scsi_set_host_notifier(VirtIOSCSI *s, VirtQueue *vq, int n)
{
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
@@ -66,31 +110,20 @@ static int virtio_scsi_set_host_notifier(VirtIOSCSI *s, VirtQueue *vq, int n)
}
/* Context: BH in IOThread */
-static void virtio_scsi_dataplane_stop_bh(void *opaque)
+static void virtio_scsi_dataplane_stop_vq_bh(void *opaque)
{
- VirtIOSCSI *s = opaque;
- VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
+ AioContext *ctx = qemu_get_current_aio_context();
+ VirtQueue *vq = opaque;
EventNotifier *host_notifier;
- int i;
- virtio_queue_aio_detach_host_notifier(vs->ctrl_vq, s->ctx);
- host_notifier = virtio_queue_get_host_notifier(vs->ctrl_vq);
+ virtio_queue_aio_detach_host_notifier(vq, ctx);
+ host_notifier = virtio_queue_get_host_notifier(vq);
/*
* Test and clear notifier after disabling event, in case poll callback
* didn't have time to run.
*/
virtio_queue_host_notifier_read(host_notifier);
-
- virtio_queue_aio_detach_host_notifier(vs->event_vq, s->ctx);
- host_notifier = virtio_queue_get_host_notifier(vs->event_vq);
- virtio_queue_host_notifier_read(host_notifier);
-
- for (i = 0; i < vs->conf.num_queues; i++) {
- virtio_queue_aio_detach_host_notifier(vs->cmd_vqs[i], s->ctx);
- host_notifier = virtio_queue_get_host_notifier(vs->cmd_vqs[i]);
- virtio_queue_host_notifier_read(host_notifier);
- }
}
/* Context: BQL held */
@@ -154,11 +187,14 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
smp_wmb(); /* paired with aio_notify_accept() */
if (s->bus.drain_count == 0) {
- virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, s->ctx);
- virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, s->ctx);
+ virtio_queue_aio_attach_host_notifier(vs->ctrl_vq,
+ s->vq_aio_context[0]);
+ virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq,
+ s->vq_aio_context[1]);
for (i = 0; i < vs->conf.num_queues; i++) {
- virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], s->ctx);
+ AioContext *ctx = s->vq_aio_context[VIRTIO_SCSI_VQ_NUM_FIXED + i];
+ virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], ctx);
}
}
return 0;
@@ -207,7 +243,11 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
s->dataplane_stopping = true;
if (s->bus.drain_count == 0) {
- aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_stop_bh, s);
+ for (i = 0; i < vs->conf.num_queues + VIRTIO_SCSI_VQ_NUM_FIXED; i++) {
+ VirtQueue *vq = virtio_get_queue(&vs->parent_obj, i);
+ AioContext *ctx = s->vq_aio_context[i];
+ aio_wait_bh_oneshot(ctx, virtio_scsi_dataplane_stop_vq_bh, vq);
+ }
}
blk_drain_all(); /* ensure there are no in-flight requests */
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 2045d27289..dabf8ace23 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -27,6 +27,7 @@
#include "hw/qdev-properties.h"
#include "hw/scsi/scsi.h"
#include "scsi/constants.h"
+#include "hw/virtio/iothread-vq-mapping.h"
#include "hw/virtio/virtio-bus.h"
#include "hw/virtio/virtio-access.h"
#include "trace.h"
@@ -318,13 +319,6 @@ static void virtio_scsi_cancel_notify(Notifier *notifier, void *data)
g_free(n);
}
-static inline void virtio_scsi_ctx_check(VirtIOSCSI *s, SCSIDevice *d)
-{
- if (s->dataplane_started && d && blk_is_available(d->conf.blk)) {
- assert(blk_get_aio_context(d->conf.blk) == s->ctx);
- }
-}
-
static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req)
{
VirtIOSCSI *s = req->dev;
@@ -517,9 +511,11 @@ static void virtio_scsi_flush_defer_tmf_to_aio_context(VirtIOSCSI *s)
assert(!s->dataplane_started);
- if (s->ctx) {
+ for (uint32_t i = 0; i < s->parent_obj.conf.num_queues; i++) {
+ AioContext *ctx = s->vq_aio_context[VIRTIO_SCSI_VQ_NUM_FIXED + i];
+
/* Our BH only runs after previously scheduled BHs */
- aio_wait_bh_oneshot(s->ctx, dummy_bh, NULL);
+ aio_wait_bh_oneshot(ctx, dummy_bh, NULL);
}
}
@@ -575,7 +571,6 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
AioContext *ctx;
int ret = 0;
- virtio_scsi_ctx_check(s, d);
/* Here VIRTIO_SCSI_S_OK means "FUNCTION COMPLETE". */
req->resp.tmf.response = VIRTIO_SCSI_S_OK;
@@ -639,6 +634,8 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
case VIRTIO_SCSI_T_TMF_ABORT_TASK_SET:
case VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET: {
+ g_autoptr(GHashTable) aio_contexts = g_hash_table_new(NULL, NULL);
+
if (!d) {
goto fail;
}
@@ -648,8 +645,15 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
qatomic_inc(&req->remaining);
- ctx = s->ctx ?: qemu_get_aio_context();
- virtio_scsi_defer_tmf_to_aio_context(req, ctx);
+ for (uint32_t i = 0; i < s->parent_obj.conf.num_queues; i++) {
+ ctx = s->vq_aio_context[VIRTIO_SCSI_VQ_NUM_FIXED + i];
+
+ if (!g_hash_table_add(aio_contexts, ctx)) {
+ continue; /* skip previously added AioContext */
+ }
+
+ virtio_scsi_defer_tmf_to_aio_context(req, ctx);
+ }
virtio_scsi_tmf_dec_remaining(req);
ret = -EINPROGRESS;
@@ -770,9 +774,12 @@ 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->dataplane_started) {
return false;
}
+ if (s->vq_aio_context[0] == qemu_get_aio_context()) {
+ return false; /* not using IOThreads */
+ }
virtio_device_start_ioeventfd(&s->parent_obj.parent_obj);
return !s->dataplane_fenced;
@@ -946,7 +953,6 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
virtio_scsi_complete_cmd_req(req);
return -ENOENT;
}
- virtio_scsi_ctx_check(s, d);
req->sreq = scsi_req_new(d, req->req.cmd.tag,
virtio_scsi_get_lun(req->req.cmd.lun),
req->req.cmd.cdb, vs->cdb_size, req);
@@ -1218,14 +1224,16 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
{
VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
VirtIOSCSI *s = VIRTIO_SCSI(vdev);
+ AioContext *ctx = s->vq_aio_context[0];
SCSIDevice *sd = SCSI_DEVICE(dev);
- int ret;
- if (s->ctx && !s->dataplane_fenced) {
- ret = blk_set_aio_context(sd->conf.blk, s->ctx, errp);
- if (ret < 0) {
- return;
- }
+ if (ctx != qemu_get_aio_context() && !s->dataplane_fenced) {
+ /*
+ * Try to make the BlockBackend's AioContext match ours. Ignore failure
+ * because I/O will still work although block jobs and other users
+ * might be slower when multiple AioContexts use a BlockBackend.
+ */
+ blk_set_aio_context(sd->conf.blk, ctx, errp);
}
if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
@@ -1260,7 +1268,7 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
- if (s->ctx) {
+ if (s->vq_aio_context[0] != qemu_get_aio_context()) {
/* If other users keep the BlockBackend in the iothread, that's ok */
blk_set_aio_context(sd->conf.blk, qemu_get_aio_context(), NULL);
}
@@ -1294,7 +1302,7 @@ static void virtio_scsi_drained_begin(SCSIBus *bus)
for (uint32_t i = 0; i < total_queues; i++) {
VirtQueue *vq = virtio_get_queue(vdev, i);
- virtio_queue_aio_detach_host_notifier(vq, s->ctx);
+ virtio_queue_aio_detach_host_notifier(vq, s->vq_aio_context[i]);
}
}
@@ -1320,10 +1328,12 @@ static void virtio_scsi_drained_end(SCSIBus *bus)
for (uint32_t i = 0; i < total_queues; i++) {
VirtQueue *vq = virtio_get_queue(vdev, i);
+ AioContext *ctx = s->vq_aio_context[i];
+
if (vq == vs->event_vq) {
- virtio_queue_aio_attach_host_notifier_no_poll(vq, s->ctx);
+ virtio_queue_aio_attach_host_notifier_no_poll(vq, ctx);
} else {
- virtio_queue_aio_attach_host_notifier(vq, s->ctx);
+ virtio_queue_aio_attach_host_notifier(vq, ctx);
}
}
}
@@ -1430,12 +1440,13 @@ void virtio_scsi_common_unrealize(DeviceState *dev)
virtio_cleanup(vdev);
}
+/* main loop */
static void virtio_scsi_device_unrealize(DeviceState *dev)
{
VirtIOSCSI *s = VIRTIO_SCSI(dev);
virtio_scsi_reset_tmf_bh(s);
-
+ virtio_scsi_dataplane_cleanup(s);
qbus_set_hotplug_handler(BUS(&s->bus), NULL);
virtio_scsi_common_unrealize(dev);
qemu_mutex_destroy(&s->tmf_bh_lock);
@@ -1460,6 +1471,8 @@ static const Property virtio_scsi_properties[] = {
VIRTIO_SCSI_F_CHANGE, true),
DEFINE_PROP_LINK("iothread", VirtIOSCSI, parent_obj.conf.iothread,
TYPE_IOTHREAD, IOThread *),
+ DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST("iothread-vq-mapping", VirtIOSCSI,
+ parent_obj.conf.iothread_vq_mapping_list),
};
static const VMStateDescription vmstate_virtio_scsi = {
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 12/12] virtio-scsi: handle ctrl virtqueue in main loop
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
` (10 preceding siblings ...)
2025-02-13 18:00 ` [PATCH 11/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
@ 2025-02-13 18:00 ` Stefan Hajnoczi
2025-03-10 14:43 ` Kevin Wolf
2025-02-20 17:04 ` [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Peter Krempa
2025-03-10 14:43 ` Kevin Wolf
13 siblings, 1 reply; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-02-13 18:00 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Stefan Hajnoczi, Kevin Wolf, David Hildenbrand, pkrempa,
Hanna Reitz
Previously the ctrl virtqueue was handled in the AioContext where SCSI
requests are processed. When IOThread Virtqueue Mapping was added things
become more complicated because SCSI requests could run in other
AioContexts.
Simplify by handling the ctrl virtqueue in the main loop where reset
operations can be performed. Note that BHs are still used canceling SCSI
requests in their AioContexts but at least the mean loop activity
doesn't need BHs anymore.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/hw/virtio/virtio-scsi.h | 8 --
hw/scsi/virtio-scsi-dataplane.c | 6 ++
hw/scsi/virtio-scsi.c | 144 ++++++--------------------------
3 files changed, 33 insertions(+), 125 deletions(-)
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 086201efa2..31e852ed6c 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -90,14 +90,6 @@ struct VirtIOSCSI {
QemuMutex ctrl_lock; /* protects ctrl_vq */
- /*
- * TMFs deferred to main loop BH. These fields are protected by
- * tmf_bh_lock.
- */
- QemuMutex tmf_bh_lock;
- QEMUBH *tmf_bh;
- QTAILQ_HEAD(, VirtIOSCSIReq) tmf_bh_list;
-
/* Fields for dataplane below */
AioContext **vq_aio_context; /* per-virtqueue AioContext pointer */
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 6bb368c8a5..2d37fa6712 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -73,6 +73,12 @@ void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp)
s->vq_aio_context[i] = ctx;
}
}
+
+ /*
+ * Always handle the ctrl virtqueue in the main loop thread where device
+ * resets can be performed.
+ */
+ s->vq_aio_context[0] = qemu_get_aio_context();
}
/* Context: BQL held */
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index dabf8ace23..1d97f8c5f4 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -319,115 +319,6 @@ static void virtio_scsi_cancel_notify(Notifier *notifier, void *data)
g_free(n);
}
-static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req)
-{
- VirtIOSCSI *s = req->dev;
- SCSIDevice *d = virtio_scsi_device_get(s, req->req.tmf.lun);
- BusChild *kid;
- int target;
-
- switch (req->req.tmf.subtype) {
- case VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET:
- if (!d) {
- req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
- goto out;
- }
- if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
- req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN;
- goto out;
- }
- qatomic_inc(&s->resetting);
- device_cold_reset(&d->qdev);
- qatomic_dec(&s->resetting);
- break;
-
- case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
- target = req->req.tmf.lun[1];
- qatomic_inc(&s->resetting);
-
- rcu_read_lock();
- QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {
- SCSIDevice *d1 = SCSI_DEVICE(kid->child);
- if (d1->channel == 0 && d1->id == target) {
- device_cold_reset(&d1->qdev);
- }
- }
- rcu_read_unlock();
-
- qatomic_dec(&s->resetting);
- break;
-
- default:
- g_assert_not_reached();
- }
-
-out:
- object_unref(OBJECT(d));
- virtio_scsi_complete_req(req, &s->ctrl_lock);
-}
-
-/* Some TMFs must be processed from the main loop thread */
-static void virtio_scsi_do_tmf_bh(void *opaque)
-{
- VirtIOSCSI *s = opaque;
- QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
- VirtIOSCSIReq *req;
- VirtIOSCSIReq *tmp;
-
- GLOBAL_STATE_CODE();
-
- WITH_QEMU_LOCK_GUARD(&s->tmf_bh_lock) {
- QTAILQ_FOREACH_SAFE(req, &s->tmf_bh_list, next, tmp) {
- QTAILQ_REMOVE(&s->tmf_bh_list, req, next);
- QTAILQ_INSERT_TAIL(&reqs, req, next);
- }
-
- qemu_bh_delete(s->tmf_bh);
- s->tmf_bh = NULL;
- }
-
- QTAILQ_FOREACH_SAFE(req, &reqs, next, tmp) {
- QTAILQ_REMOVE(&reqs, req, next);
- virtio_scsi_do_one_tmf_bh(req);
- }
-}
-
-static void virtio_scsi_reset_tmf_bh(VirtIOSCSI *s)
-{
- VirtIOSCSIReq *req;
- VirtIOSCSIReq *tmp;
-
- GLOBAL_STATE_CODE();
-
- /* Called after ioeventfd has been stopped, so tmf_bh_lock is not needed */
- if (s->tmf_bh) {
- qemu_bh_delete(s->tmf_bh);
- s->tmf_bh = NULL;
- }
-
- QTAILQ_FOREACH_SAFE(req, &s->tmf_bh_list, next, tmp) {
- QTAILQ_REMOVE(&s->tmf_bh_list, req, next);
-
- /* SAM-6 6.3.2 Hard reset */
- req->resp.tmf.response = VIRTIO_SCSI_S_TARGET_FAILURE;
- virtio_scsi_complete_req(req, &req->dev->ctrl_lock);
- }
-}
-
-static void virtio_scsi_defer_tmf_to_main_loop(VirtIOSCSIReq *req)
-{
- VirtIOSCSI *s = req->dev;
-
- WITH_QEMU_LOCK_GUARD(&s->tmf_bh_lock) {
- QTAILQ_INSERT_TAIL(&s->tmf_bh_list, req, next);
-
- if (!s->tmf_bh) {
- s->tmf_bh = qemu_bh_new(virtio_scsi_do_tmf_bh, s);
- qemu_bh_schedule(s->tmf_bh);
- }
- }
-}
-
static void virtio_scsi_tmf_cancel_req(VirtIOSCSIReq *tmf, SCSIRequest *r)
{
VirtIOSCSICancelNotifier *notifier;
@@ -627,11 +518,35 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
break;
case VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET:
- case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
- virtio_scsi_defer_tmf_to_main_loop(req);
- ret = -EINPROGRESS;
+ if (!d) {
+ goto fail;
+ }
+ if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
+ goto incorrect_lun;
+ }
+ qatomic_inc(&s->resetting);
+ device_cold_reset(&d->qdev);
+ qatomic_dec(&s->resetting);
break;
+ case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET: {
+ BusChild *kid;
+ int target = req->req.tmf.lun[1];
+ qatomic_inc(&s->resetting);
+
+ rcu_read_lock();
+ QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {
+ SCSIDevice *d1 = SCSI_DEVICE(kid->child);
+ if (d1->channel == 0 && d1->id == target) {
+ device_cold_reset(&d1->qdev);
+ }
+ }
+ rcu_read_unlock();
+
+ qatomic_dec(&s->resetting);
+ break;
+ }
+
case VIRTIO_SCSI_T_TMF_ABORT_TASK_SET:
case VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET: {
g_autoptr(GHashTable) aio_contexts = g_hash_table_new(NULL, NULL);
@@ -1087,7 +1002,6 @@ static void virtio_scsi_reset(VirtIODevice *vdev)
assert(!s->dataplane_started);
- virtio_scsi_reset_tmf_bh(s);
virtio_scsi_flush_defer_tmf_to_aio_context(s);
qatomic_inc(&s->resetting);
@@ -1402,10 +1316,8 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
VirtIOSCSI *s = VIRTIO_SCSI(dev);
Error *err = NULL;
- QTAILQ_INIT(&s->tmf_bh_list);
qemu_mutex_init(&s->ctrl_lock);
qemu_mutex_init(&s->event_lock);
- qemu_mutex_init(&s->tmf_bh_lock);
virtio_scsi_common_realize(dev,
virtio_scsi_handle_ctrl,
@@ -1445,11 +1357,9 @@ static void virtio_scsi_device_unrealize(DeviceState *dev)
{
VirtIOSCSI *s = VIRTIO_SCSI(dev);
- virtio_scsi_reset_tmf_bh(s);
virtio_scsi_dataplane_cleanup(s);
qbus_set_hotplug_handler(BUS(&s->bus), NULL);
virtio_scsi_common_unrealize(dev);
- qemu_mutex_destroy(&s->tmf_bh_lock);
qemu_mutex_destroy(&s->event_lock);
qemu_mutex_destroy(&s->ctrl_lock);
}
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
` (11 preceding siblings ...)
2025-02-13 18:00 ` [PATCH 12/12] virtio-scsi: handle ctrl virtqueue in main loop Stefan Hajnoczi
@ 2025-02-20 17:04 ` Peter Krempa
2025-03-10 14:43 ` Kevin Wolf
13 siblings, 0 replies; 23+ messages in thread
From: Peter Krempa @ 2025-02-20 17:04 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
Kevin Wolf, David Hildenbrand, Hanna Reitz
On Thu, Feb 13, 2025 at 13:00:31 -0500, Stefan Hajnoczi wrote:
> Implement --device virtio-scsi-pci,iothread-vq-mapping= support so that
> virtqueues can be assigned to different IOThreads. This improves SMP guest
> scalability where I/O-intensive applications can become bottlenecked on a
> single IOThread.
Hi I was playing around with this along with the patches and I can see
that multiple iothreads do get loaded when I configure the mapping. It
was a bit tricky to spot it though among the 200-odd 'worker' threads
qemu spawned :D (but spawns also without this feature being used).
I didn't do any benchmarks ...
> The following benchmark results show the effect of iothread-vq-mapping. fio
> randread 4k iodepth=64 results from a 4 vCPU guest with an Intel P4800X SSD:
> iothreads IOPS
> ------------------------------
> 1 189576
> 2 312698
> 4 346744
... so I'll trust you here, but at least configuration wise this seems
to work.
As discussed on the libvirt patchset it might be a good idea to
docuement that the ctrl and event queues need to be mapped as well if
you'll be dealing with docs.
Tested-by: Peter Krempa <pkrempa@redhat.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 04/12] scsi: introduce requests_lock
2025-02-13 18:00 ` [PATCH 04/12] scsi: introduce requests_lock Stefan Hajnoczi
@ 2025-03-10 13:37 ` Kevin Wolf
2025-03-11 9:11 ` Stefan Hajnoczi
0 siblings, 1 reply; 23+ messages in thread
From: Kevin Wolf @ 2025-03-10 13:37 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
David Hildenbrand, pkrempa, Hanna Reitz
Am 13.02.2025 um 19:00 hat Stefan Hajnoczi geschrieben:
> SCSIDevice keeps track of in-flight requests for device reset and Task
> Management Functions (TMFs). The request list requires protection so
> that multi-threaded SCSI emulation can be implemented in commits that
> follow.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Some of this feels quite heavy-handed, and I imagine that having to take
the lock in every request could cause considerable lock contention only
so that we can iterate all requests in a slow path.
This works for now, but maybe in the long run, we want to teach the
SCSI layer about (virt)queues, and have a separate request list per
queue (= AioContext)?
Kevin
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 11/12] virtio-scsi: add iothread-vq-mapping parameter
2025-02-13 18:00 ` [PATCH 11/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
@ 2025-03-10 14:33 ` Kevin Wolf
2025-03-10 14:37 ` Peter Krempa
0 siblings, 1 reply; 23+ messages in thread
From: Kevin Wolf @ 2025-03-10 14:33 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
David Hildenbrand, pkrempa, Hanna Reitz
Am 13.02.2025 um 19:00 hat Stefan Hajnoczi geschrieben:
> Allow virtio-scsi virtqueues to be assigned to different IOThreads. This
> makes it possible to take advantage of host multi-queue block layer
> scalability by assigning virtqueues that have affinity with vCPUs to
> different IOThreads that have affinity with host CPUs. The same feature
> was introduced for virtio-blk in the past:
> https://developers.redhat.com/articles/2024/09/05/scaling-virtio-blk-disk-io-iothread-virtqueue-mapping
>
> Here are fio randread 4k iodepth=64 results from a 4 vCPU guest with an
> Intel P4800X SSD:
> iothreads IOPS
> ------------------------------
> 1 189576
> 2 312698
> 4 346744
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
As Peter already noted, the interface is a bit confusing in that it
considers the control and event queue just normal queues like any other
and you need to specify a mapping for them, too (even though you
probably don't care about them).
I wonder if it wouldn't be better to use the iothread-vq-mapping
property only for command queues and to have separate properties for the
event and control queue. I think this would be less surprising to users.
It would also allow you to use the round robin allocation for command
queues while using a different setting for the special queues - in
particular, the event queue is currently no_poll, which disables polling
for the whole AioContext, so you probably want to have it just anywhere
else, but not in the iothreads you use for command queues. This should
probably also be the default.
Kevin
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 11/12] virtio-scsi: add iothread-vq-mapping parameter
2025-03-10 14:33 ` Kevin Wolf
@ 2025-03-10 14:37 ` Peter Krempa
2025-03-10 15:17 ` Kevin Wolf
0 siblings, 1 reply; 23+ messages in thread
From: Peter Krempa @ 2025-03-10 14:37 UTC (permalink / raw)
To: Kevin Wolf
Cc: Stefan Hajnoczi, qemu-devel, Paolo Bonzini, John Snow, Fam Zheng,
Peter Xu, Philippe Mathieu-Daudé, Michael S. Tsirkin,
qemu-block, David Hildenbrand, Hanna Reitz
On Mon, Mar 10, 2025 at 15:33:02 +0100, Kevin Wolf wrote:
> Am 13.02.2025 um 19:00 hat Stefan Hajnoczi geschrieben:
> > Allow virtio-scsi virtqueues to be assigned to different IOThreads. This
> > makes it possible to take advantage of host multi-queue block layer
> > scalability by assigning virtqueues that have affinity with vCPUs to
> > different IOThreads that have affinity with host CPUs. The same feature
> > was introduced for virtio-blk in the past:
> > https://developers.redhat.com/articles/2024/09/05/scaling-virtio-blk-disk-io-iothread-virtqueue-mapping
> >
> > Here are fio randread 4k iodepth=64 results from a 4 vCPU guest with an
> > Intel P4800X SSD:
> > iothreads IOPS
> > ------------------------------
> > 1 189576
> > 2 312698
> > 4 346744
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> As Peter already noted, the interface is a bit confusing in that it
> considers the control and event queue just normal queues like any other
> and you need to specify a mapping for them, too (even though you
> probably don't care about them).
>
> I wonder if it wouldn't be better to use the iothread-vq-mapping
> property only for command queues and to have separate properties for the
> event and control queue. I think this would be less surprising to users.
In the v2 of libvirt's patches I've proposed:
<driver queues='3'>
<iothreads>
<iothread id='2'>
<queue id='ctrl'/>
<queue id='event'/>
<queue id='1'/>
</iothread>
<iothread id='3'>
<queue id='0'/>
<queue id='2'/>
</iothread>
</iothreads>
</driver>
To map the queues by name explicitly so that it's clear what's
happening.
In my proposed it auto-translates ctrl and event into 0 and 1 and the
command queues into N+2.
> It would also allow you to use the round robin allocation for command
> queues while using a different setting for the special queues - in
> particular, the event queue is currently no_poll, which disables polling
> for the whole AioContext, so you probably want to have it just anywhere
> else, but not in the iothreads you use for command queues. This should
> probably also be the default.
This sounds like an important bit of information. If that stays like
this I think libvirt should also document this.
The proposed libvirt patch also words the recommendation to use the
round-robin approach unless specific needs arise so if qemu did the
correct thing here it would be great.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 12/12] virtio-scsi: handle ctrl virtqueue in main loop
2025-02-13 18:00 ` [PATCH 12/12] virtio-scsi: handle ctrl virtqueue in main loop Stefan Hajnoczi
@ 2025-03-10 14:43 ` Kevin Wolf
0 siblings, 0 replies; 23+ messages in thread
From: Kevin Wolf @ 2025-03-10 14:43 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
David Hildenbrand, pkrempa, Hanna Reitz
Am 13.02.2025 um 19:00 hat Stefan Hajnoczi geschrieben:
> Previously the ctrl virtqueue was handled in the AioContext where SCSI
> requests are processed. When IOThread Virtqueue Mapping was added things
> become more complicated because SCSI requests could run in other
> AioContexts.
>
> Simplify by handling the ctrl virtqueue in the main loop where reset
> operations can be performed. Note that BHs are still used canceling SCSI
> requests in their AioContexts but at least the mean loop activity
> doesn't need BHs anymore.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index 6bb368c8a5..2d37fa6712 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -73,6 +73,12 @@ void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp)
> s->vq_aio_context[i] = ctx;
> }
> }
> +
> + /*
> + * Always handle the ctrl virtqueue in the main loop thread where device
> + * resets can be performed.
> + */
> + s->vq_aio_context[0] = qemu_get_aio_context();
> }
Hmm... So now it's mandatory to provide a mapping for the control queue
if you're using iothread virtqueue mappings, but it's always ignored?
Looks like another reason why we should change the interface to have
separate properties for the command queues and the event queue (and no
property for the control queue if we want it to be fixed).
In fact, maybe just tie the event queue to the main loop, too?
Kevin
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
` (12 preceding siblings ...)
2025-02-20 17:04 ` [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Peter Krempa
@ 2025-03-10 14:43 ` Kevin Wolf
2025-03-11 9:22 ` Stefan Hajnoczi
13 siblings, 1 reply; 23+ messages in thread
From: Kevin Wolf @ 2025-03-10 14:43 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
David Hildenbrand, pkrempa, Hanna Reitz
Am 13.02.2025 um 19:00 hat Stefan Hajnoczi geschrieben:
> Implement --device virtio-scsi-pci,iothread-vq-mapping= support so that
> virtqueues can be assigned to different IOThreads. This improves SMP guest
> scalability where I/O-intensive applications can become bottlenecked on a
> single IOThread.
>
> The following benchmark results show the effect of iothread-vq-mapping. fio
> randread 4k iodepth=64 results from a 4 vCPU guest with an Intel P4800X SSD:
> iothreads IOPS
> ------------------------------
> 1 189576
> 2 312698
> 4 346744
>
> The virtio-scsi device model and core SCSI emulation currently assume that
> requests are processed in a single AioContext. This patch series goes about
> modifying this as follows:
>
> scsi-disk: drop unused SCSIDiskState->bh field
> dma: use current AioContext for dma_blk_io()
>
> Make dma-helpers.c support the QEMU multi-queue block layer by using
> qemu_get_current_aio_context().
>
> scsi: track per-SCSIRequest AioContext
> scsi: introduce requests_lock
>
> Make the core SCSI emulation code support processing requests in multiple
> AioContexts by protecting the per-SCSIDevice requests list.
>
> virtio-scsi: introduce event and ctrl virtqueue locks
> virtio-scsi: protect events_dropped field
> virtio-scsi: perform TMFs in appropriate AioContexts
>
> Make the virtio-scsi emulation code support processing requests in multiple
> AioContexts. The event and ctrl virtqueues can interact with multiple
> AioContexts. Especially the SCSI Task Management Functions (TMFs) handled by
> the ctrl virtqueue need to be made thread-safe.
>
> virtio-blk: extract cleanup_iothread_vq_mapping() function
> virtio-blk: tidy up iothread_vq_mapping functions
> virtio: extract iothread-vq-mapping.h API
> virtio-scsi: add iothread-vq-mapping parameter
>
> Port over the iothread-vq-mapping qdev property from virtio-blk to virtio-scsi.
>
> virtio-scsi: handle ctrl virtqueue in main loop
>
> Simplify TMF handling now that there is no longer a single AioContext where all
> requests are processed.
>
> Stefan Hajnoczi (12):
> scsi-disk: drop unused SCSIDiskState->bh field
> dma: use current AioContext for dma_blk_io()
> scsi: track per-SCSIRequest AioContext
> scsi: introduce requests_lock
> virtio-scsi: introduce event and ctrl virtqueue locks
> virtio-scsi: protect events_dropped field
> virtio-scsi: perform TMFs in appropriate AioContexts
> virtio-blk: extract cleanup_iothread_vq_mapping() function
> virtio-blk: tidy up iothread_vq_mapping functions
> virtio: extract iothread-vq-mapping.h API
> virtio-scsi: add iothread-vq-mapping parameter
> virtio-scsi: handle ctrl virtqueue in main loop
Patches 1-10: Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 11/12] virtio-scsi: add iothread-vq-mapping parameter
2025-03-10 14:37 ` Peter Krempa
@ 2025-03-10 15:17 ` Kevin Wolf
0 siblings, 0 replies; 23+ messages in thread
From: Kevin Wolf @ 2025-03-10 15:17 UTC (permalink / raw)
To: Peter Krempa
Cc: Stefan Hajnoczi, qemu-devel, Paolo Bonzini, John Snow, Fam Zheng,
Peter Xu, Philippe Mathieu-Daudé, Michael S. Tsirkin,
qemu-block, David Hildenbrand, Hanna Reitz
Am 10.03.2025 um 15:37 hat Peter Krempa geschrieben:
> On Mon, Mar 10, 2025 at 15:33:02 +0100, Kevin Wolf wrote:
> > Am 13.02.2025 um 19:00 hat Stefan Hajnoczi geschrieben:
> > > Allow virtio-scsi virtqueues to be assigned to different IOThreads. This
> > > makes it possible to take advantage of host multi-queue block layer
> > > scalability by assigning virtqueues that have affinity with vCPUs to
> > > different IOThreads that have affinity with host CPUs. The same feature
> > > was introduced for virtio-blk in the past:
> > > https://developers.redhat.com/articles/2024/09/05/scaling-virtio-blk-disk-io-iothread-virtqueue-mapping
> > >
> > > Here are fio randread 4k iodepth=64 results from a 4 vCPU guest with an
> > > Intel P4800X SSD:
> > > iothreads IOPS
> > > ------------------------------
> > > 1 189576
> > > 2 312698
> > > 4 346744
> > >
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >
> > As Peter already noted, the interface is a bit confusing in that it
> > considers the control and event queue just normal queues like any other
> > and you need to specify a mapping for them, too (even though you
> > probably don't care about them).
> >
> > I wonder if it wouldn't be better to use the iothread-vq-mapping
> > property only for command queues and to have separate properties for the
> > event and control queue. I think this would be less surprising to users.
>
> In the v2 of libvirt's patches I've proposed:
>
> <driver queues='3'>
> <iothreads>
> <iothread id='2'>
> <queue id='ctrl'/>
> <queue id='event'/>
> <queue id='1'/>
> </iothread>
> <iothread id='3'>
> <queue id='0'/>
> <queue id='2'/>
> </iothread>
> </iothreads>
> </driver>
>
> To map the queues by name explicitly so that it's clear what's
> happening.
>
> In my proposed it auto-translates ctrl and event into 0 and 1 and the
> command queues into N+2.
Note that if I understand patch 12 correctly, the 'ctrl' queue setting
will never actually take effect. So libvirt probably shouln't even offer
it (and neither should QEMU).
> > It would also allow you to use the round robin allocation for command
> > queues while using a different setting for the special queues - in
> > particular, the event queue is currently no_poll, which disables polling
> > for the whole AioContext, so you probably want to have it just anywhere
> > else, but not in the iothreads you use for command queues. This should
> > probably also be the default.
>
> This sounds like an important bit of information. If that stays like
> this I think libvirt should also document this.
>
> The proposed libvirt patch also words the recommendation to use the
> round-robin approach unless specific needs arise so if qemu did the
> correct thing here it would be great.
Yes, I consider this a QEMU bug that should be fixed. It's no_poll not
in the sense that we must use the eventfd because we can't otherwise
figure out if it's ready, but that we don't usually care about new
things being ready in the queue.
But if we always tie the event queue to the main loop, too, it would
already be worked around for most cases - the main loop generally won't
be able to poll anyway because of other fd handlers that don't support
polling.
Kevin
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 04/12] scsi: introduce requests_lock
2025-03-10 13:37 ` Kevin Wolf
@ 2025-03-11 9:11 ` Stefan Hajnoczi
0 siblings, 0 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-03-11 9:11 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-devel, Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
David Hildenbrand, pkrempa, Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 1287 bytes --]
On Mon, Mar 10, 2025 at 02:37:37PM +0100, Kevin Wolf wrote:
> Am 13.02.2025 um 19:00 hat Stefan Hajnoczi geschrieben:
> > SCSIDevice keeps track of in-flight requests for device reset and Task
> > Management Functions (TMFs). The request list requires protection so
> > that multi-threaded SCSI emulation can be implemented in commits that
> > follow.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> Some of this feels quite heavy-handed, and I imagine that having to take
> the lock in every request could cause considerable lock contention only
> so that we can iterate all requests in a slow path.
>
> This works for now, but maybe in the long run, we want to teach the
> SCSI layer about (virt)queues, and have a separate request list per
> queue (= AioContext)?
I had ideas about AioContext local storage but decided it was complex.
Now that I think about it, maybe it's not that much more complex because
TMF processing still needs to schedule BHs in different AioContexts even
when there is a single requests lock. So keeping AioContext local
request lists might be in the same ballpark while avoiding contention.
For the time being let's use this code and if optimization is needed,
then this would be a place to start.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter
2025-03-10 14:43 ` Kevin Wolf
@ 2025-03-11 9:22 ` Stefan Hajnoczi
0 siblings, 0 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2025-03-11 9:22 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-devel, Paolo Bonzini, John Snow, Fam Zheng, Peter Xu,
Philippe Mathieu-Daudé, Michael S. Tsirkin, qemu-block,
David Hildenbrand, pkrempa, Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 3213 bytes --]
On Mon, Mar 10, 2025 at 03:43:57PM +0100, Kevin Wolf wrote:
> Am 13.02.2025 um 19:00 hat Stefan Hajnoczi geschrieben:
> > Implement --device virtio-scsi-pci,iothread-vq-mapping= support so that
> > virtqueues can be assigned to different IOThreads. This improves SMP guest
> > scalability where I/O-intensive applications can become bottlenecked on a
> > single IOThread.
> >
> > The following benchmark results show the effect of iothread-vq-mapping. fio
> > randread 4k iodepth=64 results from a 4 vCPU guest with an Intel P4800X SSD:
> > iothreads IOPS
> > ------------------------------
> > 1 189576
> > 2 312698
> > 4 346744
> >
> > The virtio-scsi device model and core SCSI emulation currently assume that
> > requests are processed in a single AioContext. This patch series goes about
> > modifying this as follows:
> >
> > scsi-disk: drop unused SCSIDiskState->bh field
> > dma: use current AioContext for dma_blk_io()
> >
> > Make dma-helpers.c support the QEMU multi-queue block layer by using
> > qemu_get_current_aio_context().
> >
> > scsi: track per-SCSIRequest AioContext
> > scsi: introduce requests_lock
> >
> > Make the core SCSI emulation code support processing requests in multiple
> > AioContexts by protecting the per-SCSIDevice requests list.
> >
> > virtio-scsi: introduce event and ctrl virtqueue locks
> > virtio-scsi: protect events_dropped field
> > virtio-scsi: perform TMFs in appropriate AioContexts
> >
> > Make the virtio-scsi emulation code support processing requests in multiple
> > AioContexts. The event and ctrl virtqueues can interact with multiple
> > AioContexts. Especially the SCSI Task Management Functions (TMFs) handled by
> > the ctrl virtqueue need to be made thread-safe.
> >
> > virtio-blk: extract cleanup_iothread_vq_mapping() function
> > virtio-blk: tidy up iothread_vq_mapping functions
> > virtio: extract iothread-vq-mapping.h API
> > virtio-scsi: add iothread-vq-mapping parameter
> >
> > Port over the iothread-vq-mapping qdev property from virtio-blk to virtio-scsi.
> >
> > virtio-scsi: handle ctrl virtqueue in main loop
> >
> > Simplify TMF handling now that there is no longer a single AioContext where all
> > requests are processed.
> >
> > Stefan Hajnoczi (12):
> > scsi-disk: drop unused SCSIDiskState->bh field
> > dma: use current AioContext for dma_blk_io()
> > scsi: track per-SCSIRequest AioContext
> > scsi: introduce requests_lock
> > virtio-scsi: introduce event and ctrl virtqueue locks
> > virtio-scsi: protect events_dropped field
> > virtio-scsi: perform TMFs in appropriate AioContexts
> > virtio-blk: extract cleanup_iothread_vq_mapping() function
> > virtio-blk: tidy up iothread_vq_mapping functions
> > virtio: extract iothread-vq-mapping.h API
> > virtio-scsi: add iothread-vq-mapping parameter
> > virtio-scsi: handle ctrl virtqueue in main loop
>
> Patches 1-10: Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Thank you. I'm preparing another revision that applies a fixed mapping
to the ctrl and event vqs and only exposes the command vqs through the
iothread-vq-mapping property.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 01/12] scsi-disk: drop unused SCSIDiskState->bh field
2025-02-13 18:00 ` [PATCH 01/12] scsi-disk: drop unused SCSIDiskState->bh field Stefan Hajnoczi
@ 2025-03-11 9:29 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-03-11 9:29 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Paolo Bonzini, John Snow, Fam Zheng, Peter Xu, Michael S. Tsirkin,
qemu-block, Kevin Wolf, David Hildenbrand, pkrempa, Hanna Reitz
On 13/2/25 19:00, Stefan Hajnoczi wrote:
> Commit 71544d30a6f8 ("scsi: push request restart to SCSIDevice") removed
> the only user of SCSIDiskState->bh.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> hw/scsi/scsi-disk.c | 1 -
> 1 file changed, 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2025-03-11 9:30 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-13 18:00 [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 01/12] scsi-disk: drop unused SCSIDiskState->bh field Stefan Hajnoczi
2025-03-11 9:29 ` Philippe Mathieu-Daudé
2025-02-13 18:00 ` [PATCH 02/12] dma: use current AioContext for dma_blk_io() Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 03/12] scsi: track per-SCSIRequest AioContext Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 04/12] scsi: introduce requests_lock Stefan Hajnoczi
2025-03-10 13:37 ` Kevin Wolf
2025-03-11 9:11 ` Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 05/12] virtio-scsi: introduce event and ctrl virtqueue locks Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 06/12] virtio-scsi: protect events_dropped field Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 07/12] virtio-scsi: perform TMFs in appropriate AioContexts Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 08/12] virtio-blk: extract cleanup_iothread_vq_mapping() function Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 09/12] virtio-blk: tidy up iothread_vq_mapping functions Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 10/12] virtio: extract iothread-vq-mapping.h API Stefan Hajnoczi
2025-02-13 18:00 ` [PATCH 11/12] virtio-scsi: add iothread-vq-mapping parameter Stefan Hajnoczi
2025-03-10 14:33 ` Kevin Wolf
2025-03-10 14:37 ` Peter Krempa
2025-03-10 15:17 ` Kevin Wolf
2025-02-13 18:00 ` [PATCH 12/12] virtio-scsi: handle ctrl virtqueue in main loop Stefan Hajnoczi
2025-03-10 14:43 ` Kevin Wolf
2025-02-20 17:04 ` [PATCH 00/12] virtio-scsi: add iothread-vq-mapping parameter Peter Krempa
2025-03-10 14:43 ` Kevin Wolf
2025-03-11 9:22 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).