[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH v6 05/10] device-core: use RCU for list of children of a bus
From: |
Paolo Bonzini |
Subject: |
[PATCH v6 05/10] device-core: use RCU for list of children of a bus |
Date: |
Fri, 2 Oct 2020 19:35:53 +0200 |
From: Maxim Levitsky <mlevitsk@redhat.com>
This fixes the race between device emulation code that tries to find
a child device to dispatch the request to (e.g a scsi disk),
and hotplug of a new device to that bus.
Note that this doesn't convert all the readers of the list
but only these that might go over that list without BQL held.
This is a very small first step to make this code thread safe.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-5-mlevitsk@redhat.com>
[Use RCU_READ_LOCK_GUARD in more places, adjust testcase now that
the delay in DEVICE_DELETED due to RCU is more consistent. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/core/bus.c | 28 ++++++++++++++++-----------
hw/core/qdev.c | 37 ++++++++++++++++++++++--------------
hw/scsi/scsi-bus.c | 12 +++++++++---
hw/scsi/virtio-scsi.c | 6 +++++-
include/hw/qdev-core.h | 9 +++++++++
tests/qtest/drive_del-test.c | 10 +++-------
6 files changed, 66 insertions(+), 36 deletions(-)
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 6b987b6946..a0483859ae 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -49,12 +49,14 @@ int qbus_walk_children(BusState *bus,
}
}
- QTAILQ_FOREACH(kid, &bus->children, sibling) {
- err = qdev_walk_children(kid->child,
- pre_devfn, pre_busfn,
- post_devfn, post_busfn, opaque);
- if (err < 0) {
- return err;
+ WITH_RCU_READ_LOCK_GUARD() {
+ QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+ err = qdev_walk_children(kid->child,
+ pre_devfn, pre_busfn,
+ post_devfn, post_busfn, opaque);
+ if (err < 0) {
+ return err;
+ }
}
}
@@ -90,8 +92,10 @@ static void bus_reset_child_foreach(Object *obj,
ResettableChildCallback cb,
BusState *bus = BUS(obj);
BusChild *kid;
- QTAILQ_FOREACH(kid, &bus->children, sibling) {
- cb(OBJECT(kid->child), opaque, type);
+ WITH_RCU_READ_LOCK_GUARD() {
+ QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+ cb(OBJECT(kid->child), opaque, type);
+ }
}
}
@@ -194,9 +198,11 @@ static void bus_set_realized(Object *obj, bool value,
Error **errp)
/* TODO: recursive realization */
} else if (!value && bus->realized) {
- QTAILQ_FOREACH(kid, &bus->children, sibling) {
- DeviceState *dev = kid->child;
- qdev_unrealize(dev);
+ WITH_RCU_READ_LOCK_GUARD() {
+ QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+ DeviceState *dev = kid->child;
+ qdev_unrealize(dev);
+ }
}
if (bc->unrealize) {
bc->unrealize(bus);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 74db78df36..59e5e710b7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -51,6 +51,12 @@ const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
return dc->vmsd;
}
+static void bus_free_bus_child(BusChild *kid)
+{
+ object_unref(OBJECT(kid->child));
+ g_free(kid);
+}
+
static void bus_remove_child(BusState *bus, DeviceState *child)
{
BusChild *kid;
@@ -60,15 +66,16 @@ static void bus_remove_child(BusState *bus, DeviceState
*child)
char name[32];
snprintf(name, sizeof(name), "child[%d]", kid->index);
- QTAILQ_REMOVE(&bus->children, kid, sibling);
+ QTAILQ_REMOVE_RCU(&bus->children, kid, sibling);
bus->num_children--;
/* This gives back ownership of kid->child back to us. */
object_property_del(OBJECT(bus), name);
- object_unref(OBJECT(kid->child));
- g_free(kid);
- return;
+
+ /* free the bus kid, when it is safe to do so*/
+ call_rcu(kid, bus_free_bus_child, rcu);
+ break;
}
}
}
@@ -83,7 +90,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
kid->child = child;
object_ref(OBJECT(kid->child));
- QTAILQ_INSERT_HEAD(&bus->children, kid, sibling);
+ QTAILQ_INSERT_HEAD_RCU(&bus->children, kid, sibling);
/* This transfers ownership of kid->child to the property. */
snprintf(name, sizeof(name), "child[%d]", kid->index);
@@ -672,17 +679,19 @@ DeviceState *qdev_find_recursive(BusState *bus, const
char *id)
DeviceState *ret;
BusState *child;
- QTAILQ_FOREACH(kid, &bus->children, sibling) {
- DeviceState *dev = kid->child;
+ WITH_RCU_READ_LOCK_GUARD() {
+ QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+ DeviceState *dev = kid->child;
- if (dev->id && strcmp(dev->id, id) == 0) {
- return dev;
- }
+ if (dev->id && strcmp(dev->id, id) == 0) {
+ return dev;
+ }
- QLIST_FOREACH(child, &dev->child_bus, sibling) {
- ret = qdev_find_recursive(child, id);
- if (ret) {
- return ret;
+ QLIST_FOREACH(child, &dev->child_bus, sibling) {
+ ret = qdev_find_recursive(child, id);
+ if (ret) {
+ return ret;
+ }
}
}
}
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 69d7c3f90c..4ab9811cd8 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -399,7 +399,10 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq
*r)
id = r->req.dev->id;
found_lun0 = false;
n = 0;
- QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) {
+
+ RCU_READ_LOCK_GUARD();
+
+ QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
DeviceState *qdev = kid->child;
SCSIDevice *dev = SCSI_DEVICE(qdev);
@@ -420,7 +423,7 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq
*r)
memset(r->buf, 0, len);
stl_be_p(&r->buf[0], n);
i = found_lun0 ? 8 : 16;
- QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) {
+ QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
DeviceState *qdev = kid->child;
SCSIDevice *dev = SCSI_DEVICE(qdev);
@@ -429,6 +432,7 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq
*r)
i += 8;
}
}
+
assert(i == n + 8);
r->len = len;
return true;
@@ -1571,7 +1575,8 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel,
int id, int lun)
BusChild *kid;
SCSIDevice *target_dev = NULL;
- QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
+ RCU_READ_LOCK_GUARD();
+ QTAILQ_FOREACH_RCU(kid, &bus->qbus.children, sibling) {
DeviceState *qdev = kid->child;
SCSIDevice *dev = SCSI_DEVICE(qdev);
@@ -1590,6 +1595,7 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel,
int id, int lun)
}
}
}
+
return target_dev;
}
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 3a71ea7097..971afbb217 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -367,12 +367,16 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s,
VirtIOSCSIReq *req)
case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
target = req->req.tmf.lun[1];
s->resetting++;
- QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
+
+ rcu_read_lock();
+ QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {
d = SCSI_DEVICE(kid->child);
if (d->channel == 0 && d->id == target) {
qdev_reset_all(&d->qdev);
}
}
+ rcu_read_unlock();
+
s->resetting--;
break;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 14d476c587..2c6307e3ed 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -3,6 +3,8 @@
#include "qemu/queue.h"
#include "qemu/bitmap.h"
+#include "qemu/rcu.h"
+#include "qemu/rcu_queue.h"
#include "qom/object.h"
#include "hw/hotplug.h"
#include "hw/resettable.h"
@@ -238,6 +240,7 @@ struct BusClass {
};
typedef struct BusChild {
+ struct rcu_head rcu;
DeviceState *child;
int index;
QTAILQ_ENTRY(BusChild) sibling;
@@ -258,6 +261,12 @@ struct BusState {
int max_index;
bool realized;
int num_children;
+
+ /*
+ * children is a RCU QTAILQ, thus readers must use RCU to access it,
+ * and writers must hold the big qemu lock
+ */
+
QTAILQ_HEAD(, BusChild) children;
QLIST_ENTRY(BusState) sibling;
ResettableState reset;
diff --git a/tests/qtest/drive_del-test.c b/tests/qtest/drive_del-test.c
index 2d765865ce..05184124a2 100644
--- a/tests/qtest/drive_del-test.c
+++ b/tests/qtest/drive_del-test.c
@@ -15,9 +15,6 @@
#include "libqos/virtio.h"
#include "qapi/qmp/qdict.h"
-/* TODO actually test the results and get rid of this */
-#define qmp_discard_response(q, ...) qobject_unref(qtest_qmp(q, __VA_ARGS__))
-
static void drive_add(QTestState *qts)
{
char *resp = qtest_hmp(qts, "drive_add 0 if=none,id=drive0");
@@ -38,12 +35,11 @@ static void device_del(QTestState *qts)
{
QDict *response;
- /* Complication: ignore DEVICE_DELETED event */
- qmp_discard_response(qts, "{'execute': 'device_del',"
- " 'arguments': { 'id': 'dev0' } }");
- response = qtest_qmp_receive(qts);
+ response = qtest_qmp(qts, "{'execute': 'device_del',"
+ " 'arguments': { 'id': 'dev0' } }");
g_assert(response);
g_assert(qdict_haskey(response, "return"));
+ qtest_qmp_eventwait(qts, "DEVICE_DELETED");
qobject_unref(response);
}
--
2.26.2
- [PATCH v6 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, Paolo Bonzini, 2020/10/02
- [PATCH v6 01/10] qdev: add "check if address free" callback for buses, Paolo Bonzini, 2020/10/02
- [PATCH v6 02/10] scsi: switch to bus->check_address, Paolo Bonzini, 2020/10/02
- [PATCH v6 04/10] device_core: use drain_call_rcu in in qmp_device_add, Paolo Bonzini, 2020/10/02
- [PATCH v6 03/10] scsi/scsi_bus: switch search direction in scsi_device_find, Paolo Bonzini, 2020/10/02
- [PATCH v6 05/10] device-core: use RCU for list of children of a bus,
Paolo Bonzini <=
- [PATCH v6 06/10] device-core: use atomic_set on .realized property, Paolo Bonzini, 2020/10/02
- [PATCH v6 07/10] scsi/scsi-bus: scsi_device_find: don't return unrealized devices, Paolo Bonzini, 2020/10/02
- [PATCH v6 10/10] scsi/scsi_bus: fix races in REPORT LUNS, Paolo Bonzini, 2020/10/02
- [PATCH v6 08/10] scsi/scsi_bus: Add scsi_device_get, Paolo Bonzini, 2020/10/02
- [PATCH v6 09/10] virtio-scsi: use scsi_device_get, Paolo Bonzini, 2020/10/02
- Re: [PATCH v6 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, no-reply, 2020/10/03