Re: [PATCH 2/4] device-core: use RCU for list of childs of a bus

[Stefan Hajnoczi]

On Thu, Apr 16, 2020 at 11:36:22PM +0300, Maxim Levitsky wrote:
> @@ -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();

rcu_read_lock() is called but this looks like a list write operation.
If I understand correctly bus->children list writes can only be called
with the QEMU global mutex and therefore rcu_read_lock() is not required
here?

> 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));

Users like scsi_device_find() do not take a refcount on the child.  If
the device is removed then bus_free_bus_child may call object_unref()
while another thread is still accessing the child.

Maybe I'm missing something that prevents this scenario?

If not, then another patch is necessary first that introduces stricter
refcount discipline across the codebase. This applies both to users who
directly access bus->children as well as to those who call walk() and
stash child pointers in their callback function.

> +    g_free(kid);
> +}
> +
>  static void bus_remove_child(BusState *bus, DeviceState *child)
>  {
>      BusChild *kid;
>  
> -    QTAILQ_FOREACH(kid, &bus->children, sibling) {
> +    rcu_read_lock();

List write under rcu_read_lock().

> @@ -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();

List write under rcu_read_lock().

> 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) {

We need a QTAILQ_FOREACH_WITH_RCU_READ_LOCK() macro that combines
WITH_RCU_READ_LOCK() and QTAILQ_FOREACH_RCU(). :-)

> 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);

QTAILQ_FIRST_RCU()

signature.asc
Description: PGP signature