Re: [PATCH 05/10] device-core: use RCU for list of children of a bus

[Maxim Levitsky]

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;