Re: [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock
On Mon, Dec 04, 2023 at 01:46:13PM +0100, Kevin Wolf wrote: > Am 29.11.2023 um 20:55 hat Stefan Hajnoczi geschrieben: > > Protect the Task Management Function BH state with a lock. The TMF BH > > runs in the main loop thread. An IOThread might process a TMF at the > > same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh > > must be protected by a lock. > > > > Run TMF request completion in the IOThread using aio_wait_bh_oneshot(). > > This avoids more locking to protect the virtqueue and SCSI layer state. > > > > Signed-off-by: Stefan Hajnoczi > > The second part reminds me that the implicit protection of the virtqueue > and SCSI data structures by having all accesses in a single thread is > hard to review and I think we wanted to put some assertions there to > check that we're really running in the right thread. I don't think we > have done that so far, so I suppose after this patch would be the place > in the series to add them, before we remove the protection by the > AioContext lock? Thanks for reminding me. I will add assertions in the next revision of this series. Stefan signature.asc Description: PGP signature
Re: [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock
On Thu, Nov 30, 2023 at 09:25:52AM -0600, Eric Blake wrote: > On Wed, Nov 29, 2023 at 02:55:42PM -0500, Stefan Hajnoczi wrote: > > Protect the Task Management Function BH state with a lock. The TMF BH > > runs in the main loop thread. An IOThread might process a TMF at the > > same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh > > must be protected by a lock. > > > > Run TMF request completion in the IOThread using aio_wait_bh_oneshot(). > > This avoids more locking to protect the virtqueue and SCSI layer state. > > Are we trying to get this into 8.2? No. 8.2 still has the AioContext lock is therefore safe without this patch. Stefan > > > > > Signed-off-by: Stefan Hajnoczi > > --- > > include/hw/virtio/virtio-scsi.h | 3 +- > > hw/scsi/virtio-scsi.c | 62 ++--- > > 2 files changed, 43 insertions(+), 22 deletions(-) > > > > Reviewed-by: Eric Blake > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. > Virtualization: qemu.org | libguestfs.org > signature.asc Description: PGP signature
Re: [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock
Am 29.11.2023 um 20:55 hat Stefan Hajnoczi geschrieben: > Protect the Task Management Function BH state with a lock. The TMF BH > runs in the main loop thread. An IOThread might process a TMF at the > same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh > must be protected by a lock. > > Run TMF request completion in the IOThread using aio_wait_bh_oneshot(). > This avoids more locking to protect the virtqueue and SCSI layer state. > > Signed-off-by: Stefan Hajnoczi The second part reminds me that the implicit protection of the virtqueue and SCSI data structures by having all accesses in a single thread is hard to review and I think we wanted to put some assertions there to check that we're really running in the right thread. I don't think we have done that so far, so I suppose after this patch would be the place in the series to add them, before we remove the protection by the AioContext lock? Kevin
Re: [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock
Am 29.11.2023 um 20:55 hat Stefan Hajnoczi geschrieben: > Protect the Task Management Function BH state with a lock. The TMF BH > runs in the main loop thread. An IOThread might process a TMF at the > same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh > must be protected by a lock. > > Run TMF request completion in the IOThread using aio_wait_bh_oneshot(). > This avoids more locking to protect the virtqueue and SCSI layer state. > > Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf
Re: [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock
On Wed, Nov 29, 2023 at 02:55:42PM -0500, Stefan Hajnoczi wrote: > Protect the Task Management Function BH state with a lock. The TMF BH > runs in the main loop thread. An IOThread might process a TMF at the > same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh > must be protected by a lock. > > Run TMF request completion in the IOThread using aio_wait_bh_oneshot(). > This avoids more locking to protect the virtqueue and SCSI layer state. Are we trying to get this into 8.2? > > Signed-off-by: Stefan Hajnoczi > --- > include/hw/virtio/virtio-scsi.h | 3 +- > hw/scsi/virtio-scsi.c | 62 ++--- > 2 files changed, 43 insertions(+), 22 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
[PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock
Protect the Task Management Function BH state with a lock. The TMF BH runs in the main loop thread. An IOThread might process a TMF at the same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh must be protected by a lock. Run TMF request completion in the IOThread using aio_wait_bh_oneshot(). This avoids more locking to protect the virtqueue and SCSI layer state. Signed-off-by: Stefan Hajnoczi --- include/hw/virtio/virtio-scsi.h | 3 +- hw/scsi/virtio-scsi.c | 62 ++--- 2 files changed, 43 insertions(+), 22 deletions(-) diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index 779568ab5d..da8cb928d9 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -85,8 +85,9 @@ struct VirtIOSCSI { /* * TMFs deferred to main loop BH. These fields are protected by - * virtio_scsi_acquire(). + * tmf_bh_lock. */ +QemuMutex tmf_bh_lock; QEMUBH *tmf_bh; QTAILQ_HEAD(, VirtIOSCSIReq) tmf_bh_list; diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 9c751bf296..4f8d35facc 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -123,6 +123,30 @@ 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) { virtio_error(VIRTIO_DEVICE(req->dev), "wrong size for virtio-scsi headers"); @@ -338,10 +362,7 @@ static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req) out: object_unref(OBJECT(d)); - -virtio_scsi_acquire(s); -virtio_scsi_complete_req(req); -virtio_scsi_release(s); +virtio_scsi_complete_req_from_main_loop(req); } /* Some TMFs must be processed from the main loop thread */ @@ -354,18 +375,16 @@ static void virtio_scsi_do_tmf_bh(void *opaque) GLOBAL_STATE_CODE(); -virtio_scsi_acquire(s); +WITH_QEMU_LOCK_GUARD(>tmf_bh_lock) { +QTAILQ_FOREACH_SAFE(req, >tmf_bh_list, next, tmp) { +QTAILQ_REMOVE(>tmf_bh_list, req, next); +QTAILQ_INSERT_TAIL(, req, next); +} -QTAILQ_FOREACH_SAFE(req, >tmf_bh_list, next, tmp) { -QTAILQ_REMOVE(>tmf_bh_list, req, next); -QTAILQ_INSERT_TAIL(, req, next); +qemu_bh_delete(s->tmf_bh); +s->tmf_bh = NULL; } -qemu_bh_delete(s->tmf_bh); -s->tmf_bh = NULL; - -virtio_scsi_release(s); - QTAILQ_FOREACH_SAFE(req, , next, tmp) { QTAILQ_REMOVE(, req, next); virtio_scsi_do_one_tmf_bh(req); @@ -379,8 +398,7 @@ static void virtio_scsi_reset_tmf_bh(VirtIOSCSI *s) GLOBAL_STATE_CODE(); -virtio_scsi_acquire(s); - +/* 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; @@ -393,19 +411,19 @@ static void virtio_scsi_reset_tmf_bh(VirtIOSCSI *s) req->resp.tmf.response = VIRTIO_SCSI_S_TARGET_FAILURE; virtio_scsi_complete_req(req); } - -virtio_scsi_release(s); } static void virtio_scsi_defer_tmf_to_bh(VirtIOSCSIReq *req) { VirtIOSCSI *s = req->dev; -QTAILQ_INSERT_TAIL(>tmf_bh_list, req, next); +WITH_QEMU_LOCK_GUARD(>tmf_bh_lock) { +QTAILQ_INSERT_TAIL(>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); +if (!s->tmf_bh) { +s->tmf_bh = qemu_bh_new(virtio_scsi_do_tmf_bh, s); +qemu_bh_schedule(s->tmf_bh); +} } } @@ -1235,6 +1253,7 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp) Error *err = NULL; QTAILQ_INIT(>tmf_bh_list); +qemu_mutex_init(>tmf_bh_lock); virtio_scsi_common_realize(dev, virtio_scsi_handle_ctrl, @@ -1277,6 +1296,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev) qbus_set_hotplug_handler(BUS(>bus), NULL); virtio_scsi_common_unrealize(dev); +qemu_mutex_destroy(>tmf_bh_lock); } static Property virtio_scsi_properties[] = { -- 2.42.0