Message ID | 50227AB9.7010206@msgid.tls.msk.ru |
---|---|
State | New |
Headers | show |
On 08.08.2012 18:42, Michael Tokarev wrote:
> Should it go to qemu/stable-1.1 as well?
qemu/stable-1.1 also includes f63e60327b8e239ae97fa71060940ca20a8bf38e.
FWIW.
Michael Tokarev <mjt@tls.msk.ru> writes: > On 08.08.2012 17:09, Michael Tokarev wrote: > [] >> Something similar should be applied to 1.1-stable. FWIW, some >> changes are not needed there. > > Cherry-pick to stable-1.1 removes the two unneeded hunks. > This is what I plan to include into debian package. It > fixes the original usb_del issue, and I didn't find new > regressions so far - tried a few device_del and similar. > > Should it go to qemu/stable-1.1 as well? > > Thank you! > > /mjtAuthor: Paolo Bonzini <pbonzini@redhat.com> > Date: Wed Aug 8 14:39:11 2012 +0200 > Bug-Debian: http://bugs.debian.org/684282 > Comment: cherry-picked from qemu/master to stable-1.1 (mjt) > > 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> > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c > index 0345490..585da4e 100644 > --- a/hw/acpi_piix4.c > +++ b/hw/acpi_piix4.c > @@ -299,7 +299,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 6a8f6bd..9bb1c6b 100644 > --- a/hw/qdev.c > +++ b/hw/qdev.c > @@ -240,7 +240,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/xen_platform.c b/hw/xen_platform.c > index 0214f37..84221df 100644 > --- a/hw/xen_platform.c > +++ b/hw/xen_platform.c > @@ -87,9 +87,6 @@ static void unplug_nic(PCIBus *b, PCIDevice *d) > { > 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 6f839ad..58dd886 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -347,8 +347,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) > @@ -385,8 +383,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); > } This won't work with composition. object_delete() is never called for child<> objects. Regards, Anthony Liguori >
Il 20/08/2012 19:58, Anthony Liguori ha scritto: > Michael Tokarev <mjt@tls.msk.ru> writes: > >> On 08.08.2012 17:09, Michael Tokarev wrote: >> [] >>> Something similar should be applied to 1.1-stable. FWIW, some >>> changes are not needed there. >> >> Cherry-pick to stable-1.1 removes the two unneeded hunks. >> This is what I plan to include into debian package. It >> fixes the original usb_del issue, and I didn't find new >> regressions so far - tried a few device_del and similar. >> >> Should it go to qemu/stable-1.1 as well? >> >> Thank you! >> >> /mjtAuthor: Paolo Bonzini <pbonzini@redhat.com> >> Date: Wed Aug 8 14:39:11 2012 +0200 >> Bug-Debian: http://bugs.debian.org/684282 >> Comment: cherry-picked from qemu/master to stable-1.1 (mjt) >> >> 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> >> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> >> >> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c >> index 0345490..585da4e 100644 >> --- a/hw/acpi_piix4.c >> +++ b/hw/acpi_piix4.c >> @@ -299,7 +299,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 6a8f6bd..9bb1c6b 100644 >> --- a/hw/qdev.c >> +++ b/hw/qdev.c >> @@ -240,7 +240,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/xen_platform.c b/hw/xen_platform.c >> index 0214f37..84221df 100644 >> --- a/hw/xen_platform.c >> +++ b/hw/xen_platform.c >> @@ -87,9 +87,6 @@ static void unplug_nic(PCIBus *b, PCIDevice *d) >> { >> 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 6f839ad..58dd886 100644 >> --- a/qom/object.c >> +++ b/qom/object.c >> @@ -347,8 +347,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) >> @@ -385,8 +383,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); >> } > > This won't work with composition. object_delete() is never called for > child<> objects. For non-heap-allocated children, their last ref will go away when the parent's child<> property is eliminated. This will remove the last reference and call object_finalize (which will take care of multiple levels of compositions). The same holds for heap-allocated children, but indeed you will leak the memory for the object because object_delete is not called. However this is already the case, the patch is not introducing a regression. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 20/08/2012 19:58, Anthony Liguori ha scritto: >> Michael Tokarev <mjt@tls.msk.ru> writes: >> >>> On 08.08.2012 17:09, Michael Tokarev wrote: >>> [] >>>> Something similar should be applied to 1.1-stable. FWIW, some >>>> changes are not needed there. >>> >>> Cherry-pick to stable-1.1 removes the two unneeded hunks. >>> This is what I plan to include into debian package. It >>> fixes the original usb_del issue, and I didn't find new >>> regressions so far - tried a few device_del and similar. >>> >>> Should it go to qemu/stable-1.1 as well? >>> >>> Thank you! >>> >>> /mjtAuthor: Paolo Bonzini <pbonzini@redhat.com> >>> Date: Wed Aug 8 14:39:11 2012 +0200 >>> Bug-Debian: http://bugs.debian.org/684282 >>> Comment: cherry-picked from qemu/master to stable-1.1 (mjt) >>> >>> 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> >>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> >>> >>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c >>> index 0345490..585da4e 100644 >>> --- a/hw/acpi_piix4.c >>> +++ b/hw/acpi_piix4.c >>> @@ -299,7 +299,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 6a8f6bd..9bb1c6b 100644 >>> --- a/hw/qdev.c >>> +++ b/hw/qdev.c >>> @@ -240,7 +240,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/xen_platform.c b/hw/xen_platform.c >>> index 0214f37..84221df 100644 >>> --- a/hw/xen_platform.c >>> +++ b/hw/xen_platform.c >>> @@ -87,9 +87,6 @@ static void unplug_nic(PCIBus *b, PCIDevice *d) >>> { >>> 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 6f839ad..58dd886 100644 >>> --- a/qom/object.c >>> +++ b/qom/object.c >>> @@ -347,8 +347,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) >>> @@ -385,8 +383,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); >>> } >> >> This won't work with composition. object_delete() is never called for >> child<> objects. > > For non-heap-allocated children, their last ref will go away when the > parent's child<> property is eliminated. This will remove the last > reference and call object_finalize (which will take care of multiple > levels of compositions). > > The same holds for heap-allocated children, but indeed you will leak the > memory for the object because object_delete is not called. However this > is already the case, the patch is not introducing a regression. Ok, can you submit as a top level patch and I'll apply it for 1.2? Regards, Anthony Liguori > > Paolo
Author: Paolo Bonzini <pbonzini@redhat.com> Date: Wed Aug 8 14:39:11 2012 +0200 Bug-Debian: http://bugs.debian.org/684282 Comment: cherry-picked from qemu/master to stable-1.1 (mjt) 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> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c index 0345490..585da4e 100644 --- a/hw/acpi_piix4.c +++ b/hw/acpi_piix4.c @@ -299,7 +299,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 6a8f6bd..9bb1c6b 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -240,7 +240,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/xen_platform.c b/hw/xen_platform.c index 0214f37..84221df 100644 --- a/hw/xen_platform.c +++ b/hw/xen_platform.c @@ -87,9 +87,6 @@ static void unplug_nic(PCIBus *b, PCIDevice *d) { 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 6f839ad..58dd886 100644 --- a/qom/object.c +++ b/qom/object.c @@ -347,8 +347,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) @@ -385,8 +383,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); }