Message ID | 50225DEF.1060206@redhat.com |
---|---|
State | New |
Headers | show |
Am 08.08.2012 14:39, schrieb Paolo Bonzini: > Il 08/08/2012 14:22, Michael Tokarev ha scritto: >> [...] should there be a call to object_unref() >> somewhere? > > There should be a call to object_unparent() somewhere actually. > We've been peppering the code with them for a few months now while > waiting for "the" solution, but I fail to see what the solution > could be other than the patch below (something similar has probably > been proposed already). BTW there are probably a lot of other similar > bugs somewhere. > > Paolo > > -------------------- 8< ----------------- > From 6d1ea139acf85184fa721654bcc68a4544a536ca Mon Sep 17 00:00:00 2001 > From: Paolo Bonzini <pbonzini@redhat.com> > Date: Wed, 8 Aug 2012 14:33:02 +0200 > Subject: [PATCH] qom: object_delete should unparent the object first > > object_deinit is only called when the reference count goes to zero, > and yet tries to do an object_unparent. Now, object_unparent > either does nothing or it will decrease the reference count. > Because we know the reference count is zero, the object_unparent > call in object_deinit is useless. > > Instead, we need to disconnect the object from its parent just > before we remove the last reference apart from the parent's. This > happens in object_delete. Once we do this, all calls to > object_unparent peppered through QEMU can go away. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/acpi_piix4.c | 1 - > hw/qdev.c | 2 -- > hw/shpc.c | 1 - > hw/xen_platform.c | 3 --- > qom/object.c | 3 +-- > 5 file modificati, 1 inserzione(+), 9 rimozioni(-) > > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c > index 0aace60..72d6e5c 100644 > --- a/hw/acpi_piix4.c > +++ b/hw/acpi_piix4.c > @@ -305,7 +305,6 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots) > if (pc->no_hotplug) { > slot_free = false; > } else { > - object_unparent(OBJECT(dev)); > qdev_free(qdev); > } > } > diff --git a/hw/qdev.c b/hw/qdev.c > index b5b74b9..b5a52ac 100644 > --- a/hw/qdev.c > +++ b/hw/qdev.c > @@ -159,7 +159,6 @@ int qdev_init(DeviceState *dev) > > rc = dc->init(dev); > if (rc < 0) { > - object_unparent(OBJECT(dev)); > qdev_free(dev); > return rc; > } > @@ -243,7 +242,6 @@ void qbus_reset_all_fn(void *opaque) > int qdev_simple_unplug_cb(DeviceState *dev) > { > /* just zap it */ > - object_unparent(OBJECT(dev)); > qdev_free(dev); > return 0; > } > diff --git a/hw/shpc.c b/hw/shpc.c > index 6b9884d..a5baf24 100644 > --- a/hw/shpc.c > +++ b/hw/shpc.c > @@ -253,7 +253,6 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot) > ++devfn) { > PCIDevice *affected_dev = shpc->sec_bus->devices[devfn]; > if (affected_dev) { > - object_unparent(OBJECT(affected_dev)); > qdev_free(&affected_dev->qdev); > } > } > diff --git a/hw/xen_platform.c b/hw/xen_platform.c > index c1fe984..0d6c2ff 100644 > --- a/hw/xen_platform.c > +++ b/hw/xen_platform.c > @@ -87,9 +87,6 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o) > { > if (pci_get_word(d->config + PCI_CLASS_DEVICE) == > PCI_CLASS_NETWORK_ETHERNET) { > - /* Until qdev_free includes a call to object_unparent, we call it here > - */ > - object_unparent(&d->qdev.parent_obj); > qdev_free(&d->qdev); > } > } > diff --git a/qom/object.c b/qom/object.c > index 00bb3b0..3ccd744 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -366,8 +366,6 @@ static void object_deinit(Object *obj, TypeImpl *type) > if (type_has_parent(type)) { > object_deinit(obj, type_get_parent(type)); > } > - > - object_unparent(obj); > } > > void object_finalize(void *data) > @@ -402,8 +402,9 @@ Object *object_new(const char *typename) > > void object_delete(Object *obj) > { > + object_unparent(obj); > + g_assert(obj->ref == 1); > object_unref(obj); > - g_assert(obj->ref == 0); > g_free(obj); > } > Adding object_unparent() to object_delete() looks okay to me, but we should not forget about the upcoming i440fx and prep_pci use cases where we want to embed children in the parent's struct, so that object_delete() will never be called on it. Thus object_unparent() would need to remain present in the deinit path, no? Andreas
Il 08/08/2012 14:48, Andreas Färber ha scritto: > Adding object_unparent() to object_delete() looks okay to me, but we > should not forget about the upcoming i440fx and prep_pci use cases where > we want to embed children in the parent's struct, so that > object_delete() will never be called on it. Thus object_unparent() would > need to remain present in the deinit path, no? object_property_del_all should take care of it for embedded objects: - the outermost struct is deleted via object_delete - it is unparented and unrefed, so refcount goes to 0 and finalize is called - finalize calls object_property_del_all - object_finalize_child_property calls unref on the nested object, so refcount goes to 0 and finalize is called. Things can then proceed recursively. It would be more complicated (and would cause memory leaks) if you got nested objects that were allocated. To account for this, I understood you would need something like the following: - you add a "void (*release)(void *obj)" member to Object. - object_new sets the release member to g_free - object_delete does not call g_free anymore - object_finalize calls the release member if it is not NULL Do you have time to implement it? Paolo
On 08.08.2012 16:39, Paolo Bonzini wrote: > Il 08/08/2012 14:22, Michael Tokarev ha scritto: >> @@ -152,6 +152,16 @@ int qdev_init(DeviceState *dev) >> + >> + if (!OBJECT(dev)->parent) { >> + static int unattached_count = 0; >> + gchar *name = g_strdup_printf("device[%d]", unattached_count++); >> + >> + object_property_add_child(container_get("/unattached"), name, >> + OBJECT(dev), NULL); >> + g_free(name); >> + } >> >> ie, there's now extra call to object_property_add_child(), >> which does object_ref(). So no doubt there's no a new >> assertion failure: (obj->ref == 0). >> >> Once again, patch description does not reflect what is >> actually done by the patch. > > Huh? The add_child ensures that there is a canonical path. > >> Can we please stop this >> practice of accepting patches with desrciption not >> reflecting reality? I must clarify this. I'm not trying to blame Paolo for the wrong patch which "broke" things (it exposed an old bug in other codepath). I'm just saying that the patch description appears to be "too innocent" -- yes, now each device has a path, or, in other words, a NAME, but this patch ALSO changes refcounting, and THIS is what I'm referring to above -- it isn't only gives a name, but also links "unowned" objects to a bus. To me it is a wrong (too innocent) description. I browsed comits trying to find which change might have caused it, and provided ther was a mention of "references" or "connecting" in this patch description somewhere, I'd found this change much faster, without a painful (*) bisection. Maybe it is just me who does not know this code area well enough, so things aren't as obvious for me as for others, I dunno, but to me the description -- the only thing I'm trying to "blame" Paolo for here -- might be quite a bit better. (*) painful because this bisection come in the process of another bisection dealing with another usb issue, which come to yet another bug in usb handling somewhere (giving another assertion). >> Back to the point: should there be a call to object_unref() >> somewhere? > > There should be a call to object_unparent() somewhere actually. > We've been peppering the code with them for a few months now while > waiting for "the" solution, but I fail to see what the solution > could be other than the patch below (something similar has probably > been proposed already). BTW there are probably a lot of other similar > bugs somewhere. This sounds ecouraging -- "alot of other similar bugs".. :( Something similar should be applied to 1.1-stable. FWIW, some changes are not needed there, like this: > --- a/hw/qdev.c > +++ b/hw/qdev.c > @@ -159,7 +159,6 @@ int qdev_init(DeviceState *dev) > > rc = dc->init(dev); > if (rc < 0) { > - object_unparent(OBJECT(dev)); > qdev_free(dev); > return rc; > } and this: > --- a/hw/shpc.c > +++ b/hw/shpc.c > @@ -253,7 +253,6 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot) > ++devfn) { > PCIDevice *affected_dev = shpc->sec_bus->devices[devfn]; > if (affected_dev) { > - object_unparent(OBJECT(affected_dev)); > qdev_free(&affected_dev->qdev); > } > } the lines being removed does not exist in 1.1-stable. I can guess this is due to previous attempts to fix similar issues in other places. Thank you! /mjt
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c index 0aace60..72d6e5c 100644 --- a/hw/acpi_piix4.c +++ b/hw/acpi_piix4.c @@ -305,7 +305,6 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots) if (pc->no_hotplug) { slot_free = false; } else { - object_unparent(OBJECT(dev)); qdev_free(qdev); } } diff --git a/hw/qdev.c b/hw/qdev.c index b5b74b9..b5a52ac 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -159,7 +159,6 @@ int qdev_init(DeviceState *dev) rc = dc->init(dev); if (rc < 0) { - object_unparent(OBJECT(dev)); qdev_free(dev); return rc; } @@ -243,7 +242,6 @@ void qbus_reset_all_fn(void *opaque) int qdev_simple_unplug_cb(DeviceState *dev) { /* just zap it */ - object_unparent(OBJECT(dev)); qdev_free(dev); return 0; } diff --git a/hw/shpc.c b/hw/shpc.c index 6b9884d..a5baf24 100644 --- a/hw/shpc.c +++ b/hw/shpc.c @@ -253,7 +253,6 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot) ++devfn) { PCIDevice *affected_dev = shpc->sec_bus->devices[devfn]; if (affected_dev) { - object_unparent(OBJECT(affected_dev)); qdev_free(&affected_dev->qdev); } } diff --git a/hw/xen_platform.c b/hw/xen_platform.c index c1fe984..0d6c2ff 100644 --- a/hw/xen_platform.c +++ b/hw/xen_platform.c @@ -87,9 +87,6 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o) { if (pci_get_word(d->config + PCI_CLASS_DEVICE) == PCI_CLASS_NETWORK_ETHERNET) { - /* Until qdev_free includes a call to object_unparent, we call it here - */ - object_unparent(&d->qdev.parent_obj); qdev_free(&d->qdev); } } diff --git a/qom/object.c b/qom/object.c index 00bb3b0..3ccd744 100644 --- a/qom/object.c +++ b/qom/object.c @@ -366,8 +366,6 @@ static void object_deinit(Object *obj, TypeImpl *type) if (type_has_parent(type)) { object_deinit(obj, type_get_parent(type)); } - - object_unparent(obj); } void object_finalize(void *data) @@ -402,8 +402,9 @@ Object *object_new(const char *typename) void object_delete(Object *obj) { + object_unparent(obj); + g_assert(obj->ref == 1); object_unref(obj); - g_assert(obj->ref == 0); g_free(obj); }