[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 02/15] qdev: store DeviceState's canonical p
From: |
Michael Roth |
Subject: |
Re: [Qemu-devel] [RFC PATCH 02/15] qdev: store DeviceState's canonical path to use when unparenting |
Date: |
Thu, 30 Apr 2015 18:03:31 -0500 |
User-agent: |
alot/0.3.6 |
Quoting Paolo Bonzini (2015-04-30 08:35:13)
>
>
> On 29/04/2015 21:20, Michael Roth wrote:
> > If the parent is finalized as a result of object_unparent(), it
> > will still be attached to the composition tree at the time any
> > children are unparented as a result of that same call to
> > object_unparent(). However, in some cases, object_unparent()
> > will complete without finalizing the parent device, due to
> > lingering references that won't be released till some time later.
> > One such example is if the parent has MemoryRegion children (which
> > take a ref on their parent), who in turn have AddressSpace's (which
> > take a ref on their regions), since those AddressSpaces get cleaned
> > up asynchronously by the RCU thread.
> >
> > In this case qdev:device_unparent() may be called for a child Device
> > that no longer has a path to the root/machine container, causing
> > object_get_canonical_path() to assert.
>
> This doesn't seem right. Unparent callbacks are _not_ called when you
> finalize, they are called in post-order as soon as you unplug a device
> (the call tree is object_unparent ==> device_unparent(parent) ==>
> bus_unparent(parent->bus) ==> device_unparent(parent->bus->child[0])
> and so on).
Hmm, well, that does seem to be the case for devices that reside on a
bus, since the post-order traversal from the parent will eventually
reach any such devices.
And for a device that gets unparented before it's parent bus, I think
the fix you posted ends up not being needed because the child is
actually a link property of the parent bus, as opposed to an actual
child property, so removing the property doesn't "disconnect" the
device from the composition tree: presumably the *actual* parent
object/container (/machine/unattached I believe?) is still around
when the DEVICE_DELETED event is sent, so it still has a canonical
path and we don't get the assert from object_get_canonical_path().
In my case though I have a couple device types (spapr_drc, and
spapr_iommu) that are direct child properties of the PHB, and
from what I can tell the clean up path for those when we do
object_unparent(phb) goes something like:
object_unparent(o):
object_property_del_child(o->parent, o, NULL):
object_property_del(p, prop_name):
prop->release(p, prop_name, prop_opaque):
| object_finalize_child_property(p, prop_name, o):
| o->class->unparent(o):
| device_unparent(o) <- (post-order clean-up, but skips
| direct children like spapr_drc/spapr_iommu)
| o->parent = NULL
| object_unref(o):
| object_finalize(o): <- may happen asynchronously due to RCU cleanup
| for AddressSpace/MemoryRegion ref holder.
| object o will no longer be child prop of
| o->parent. actually, this seems like it would
| be the case during synchronous finalization
| as well now that I look at it more closely...
| object_property_del_all(o)
| for prop in o->properties:
| prop->release(o, prop->name, prop->opaque/o->child)
| object_finalize_child_property(o, prop_name, c):
| o->class->unparent(c):
| device_unparent(c) <- (spapr_drc/spapr_iommu children
| get their callbacks after being
| disconnected, no longer have
| canonical paths)
| QTAILQ_REMOVE(&o->properties, prop, node)
| object_deinit(o)
| o->free(o)
QTAILQ_REMOVE(&o->properties, prop, node)
I played around with the idea of temporarilly moving unparented, unfinalized
objects to an "orphan" container. It seemed like a fun way of tracking leaked
objects, and avoids the assert, but that got wierd pretty quickly... and
having DEVICE_DELETED randomly change up the device path didn't seem like
the intended behavior, so this hack ended up seeming pretty reasonable.
The other approach, which I hadn't looked into too closely, was to defer
unparenting an object until it's ref count goes to 0. Could maybe look into
that instead if it seems less hacky.
>
> DEVICE_DELETED is called after a device's children have been
> unparented. It could be called after a bus is dead though. Could it
> be that the patch you want is simply this:
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6e6a65d..46019c4 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1241,11 +1241,6 @@ static void device_unparent(Object *obj)
> bus = QLIST_FIRST(&dev->child_bus);
> object_unparent(OBJECT(bus));
> }
> - if (dev->parent_bus) {
> - bus_remove_child(dev->parent_bus, dev);
> - object_unref(OBJECT(dev->parent_bus));
> - dev->parent_bus = NULL;
> - }
>
> /* Only send event if the device had been completely realized */
> if (dev->pending_deleted_event) {
> @@ -1254,6 +1249,12 @@ static void device_unparent(Object *obj)
> qapi_event_send_device_deleted(!!dev->id, dev->id, path,
> &error_abort);
> g_free(path);
> }
> +
> + if (dev->parent_bus) {
> + bus_remove_child(dev->parent_bus, dev);
> + object_unref(OBJECT(dev->parent_bus));
> + dev->parent_bus = NULL;
> + }
> }
>
> static void device_class_init(ObjectClass *class, void *data)
>
> ?
>
> Paolo
>
- [Qemu-devel] [RFC PATCH 10/15] spapr_events: add support for phb hotplug events, (continued)
- [Qemu-devel] [RFC PATCH 10/15] spapr_events: add support for phb hotplug events, Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 09/15] spapr: populate PHB DRC entries for root DT node, Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 11/15] qdev: add qbus_set_hotplug_handler_generic(), Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 13/15] spapr_pci: provide node start offset via spapr_populate_pci_dt(), Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 12/15] spapr: stub implementation of machine-level HotplugHandler interface, Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 14/15] spapr_pci: add ibm, my-drc-index property for PHB hotplug, Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 01/15] pci: allow cleanup/unregistration of PCI buses, Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 02/15] qdev: store DeviceState's canonical path to use when unparenting, Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 04/15] spapr_iommu: pass object ownership to parent/owner, Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 15/15] spapr: add hotplug hooks for PHB hotplug, Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 03/15] spapr_drc: pass object ownership to parent/owner, Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 05/15] spapr_pci: add PHB unrealize, Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 07/15] spapr: enable PHB hotplug for pseries-2.4, Michael Roth, 2015/04/29
- [Qemu-devel] [RFC PATCH 06/15] spapr_pci: also use 'index' property as DRC index for PHBs, Michael Roth, 2015/04/29