| Message ID | 20200925172604.2142227-6-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread | expand |
On Fri, 2020-09-25 at 13:25 -0400, Paolo Bonzini wrote: > 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. - Paolo] This is a good idea which I don't yet use much. Best regards, Maxim Levitsky > 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 +++++++++ > 5 files changed, 63 insertions(+), 29 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 e62da68a26..8067497074 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" > @@ -228,6 +230,7 @@ struct BusClass { > }; > > typedef struct BusChild { > + struct rcu_head rcu; > DeviceState *child; > int index; > QTAILQ_ENTRY(BusChild) sibling; > @@ -248,6 +251,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/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 e62da68a26..8067497074 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" @@ -228,6 +230,7 @@ struct BusClass { }; typedef struct BusChild { + struct rcu_head rcu; DeviceState *child; int index; QTAILQ_ENTRY(BusChild) sibling; @@ -248,6 +251,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;