[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH 2/4] device-core: use RCU for list of childs of a bus
From: |
Maxim Levitsky |
Subject: |
[PATCH 2/4] device-core: use RCU for list of childs of a bus |
Date: |
Thu, 16 Apr 2020 23:36:22 +0300 |
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 <address@hidden>
Signed-off-by: Maxim Levitsky <address@hidden>
---
hw/core/bus.c | 43 ++++++++++++++++++++-----------
hw/core/qdev.c | 46 +++++++++++++++++++++++-----------
hw/scsi/scsi-bus.c | 17 ++++++++++---
hw/scsi/virtio-scsi.c | 6 ++++-
include/hw/qdev-core.h | 3 +++
include/hw/virtio/virtio-bus.h | 7 ++++--
6 files changed, 87 insertions(+), 35 deletions(-)
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 3dc0a825f0..cb7756ded1 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,9 +92,13 @@ static void bus_reset_child_foreach(Object *obj,
ResettableChildCallback cb,
BusState *bus = BUS(obj);
BusChild *kid;
- QTAILQ_FOREACH(kid, &bus->children, sibling) {
+ rcu_read_lock();
+
+ QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
cb(OBJECT(kid->child), opaque, type);
}
+
+ rcu_read_unlock();
}
static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
@@ -138,10 +144,15 @@ static void bus_unparent(Object *obj)
/* Only the main system bus has no parent, and that bus is never freed */
assert(bus->parent);
- while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
+ rcu_read_lock();
+
+ while ((kid = QTAILQ_FIRST_RCU(&bus->children)) != NULL) {
DeviceState *dev = kid->child;
object_unparent(OBJECT(dev));
}
+
+ rcu_read_unlock();
+
QLIST_REMOVE(bus, sibling);
bus->parent->num_child_bus--;
bus->parent = NULL;
@@ -185,14 +196,18 @@ 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;
- object_property_set_bool(OBJECT(dev), false, "realized",
- &local_err);
- if (local_err != NULL) {
- break;
+
+ WITH_RCU_READ_LOCK_GUARD(){
+ QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+ DeviceState *dev = kid->child;
+ object_property_set_bool(OBJECT(dev), false, "realized",
+ &local_err);
+ if (local_err != NULL) {
+ break;
+ }
}
}
+
if (bc->unrealize && local_err == NULL) {
bc->unrealize(bus, &local_err);
}
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 85f062def7..f0c87e582e 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -50,26 +50,37 @@ 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;
- QTAILQ_FOREACH(kid, &bus->children, sibling) {
+ rcu_read_lock();
+
+ QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
if (kid->child == 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, NULL);
- 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;
}
}
+
+ rcu_read_unlock();
}
static void bus_add_child(BusState *bus, DeviceState *child)
@@ -82,7 +93,9 @@ static void bus_add_child(BusState *bus, DeviceState *child)
kid->child = child;
object_ref(OBJECT(kid->child));
- QTAILQ_INSERT_HEAD(&bus->children, kid, sibling);
+ rcu_read_lock();
+ QTAILQ_INSERT_HEAD_RCU(&bus->children, kid, sibling);
+ rcu_read_unlock();
/* This transfers ownership of kid->child to the property. */
snprintf(name, sizeof(name), "child[%d]", kid->index);
@@ -681,20 +694,23 @@ 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;
+ }
}
}
}
+
return NULL;
}
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 7bbc37acec..2bf6d005f3 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -412,7 +412,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();
+
+ QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
DeviceState *qdev = kid->child;
SCSIDevice *dev = SCSI_DEVICE(qdev);
@@ -433,7 +436,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);
@@ -442,6 +445,9 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq
*r)
i += 8;
}
}
+
+ rcu_read_unlock();
+
assert(i == n + 8);
r->len = len;
return true;
@@ -1584,12 +1590,15 @@ 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();
+
+ QTAILQ_FOREACH_RCU(kid, &bus->qbus.children, sibling) {
DeviceState *qdev = kid->child;
SCSIDevice *dev = SCSI_DEVICE(qdev);
if (dev->channel == channel && dev->id == id) {
if (dev->lun == lun) {
+ rcu_read_unlock();
return dev;
}
@@ -1603,6 +1612,8 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel,
int id, int lun)
}
}
}
+
+ rcu_read_unlock();
return target_dev;
}
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 472bbd233b..b0f4a35f81 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 1405b8a990..196253978b 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"
@@ -218,6 +220,7 @@ struct BusClass {
};
typedef struct BusChild {
+ struct rcu_head rcu;
DeviceState *child;
int index;
QTAILQ_ENTRY(BusChild) sibling;
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 38c9399cd4..58733f28e2 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -128,8 +128,11 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus,
uint8_t *config);
static inline VirtIODevice *virtio_bus_get_device(VirtioBusState *bus)
{
BusState *qbus = &bus->parent_obj;
- BusChild *kid = QTAILQ_FIRST(&qbus->children);
- DeviceState *qdev = kid ? kid->child : NULL;
+ BusChild *kid;
+ DeviceState *qdev;
+
+ kid = QTAILQ_FIRST(&qbus->children);
+ qdev = kid ? kid->child : NULL;
/* This is used on the data path, the cast is guaranteed
* to succeed by the qdev machinery.
--
2.17.2
- [PATCH 0/4] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, Maxim Levitsky, 2020/04/16
- [PATCH 1/4] scsi/scsi_bus: switch search direction in scsi_device_find, Maxim Levitsky, 2020/04/16
- [PATCH 2/4] device-core: use RCU for list of childs of a bus,
Maxim Levitsky <=
- [PATCH 4/4] virtio-scsi: don't touch scsi devices that are not yet realized, Maxim Levitsky, 2020/04/16
- [PATCH 3/4] device-core: use atomic_set on .realized property, Maxim Levitsky, 2020/04/16
- Re: [PATCH 0/4] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, no-reply, 2020/04/16
- Re: [PATCH 0/4] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, no-reply, 2020/04/16
- Re: [PATCH 0/4] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, no-reply, 2020/04/16