Patchwork commit da57febfed "qdev: give all devices a canonical path" broke usb_del

login
register
mail settings
Submitter Paolo Bonzini
Date Aug. 8, 2012, 12:39 p.m.
Message ID <50225DEF.1060206@redhat.com>
Download mbox | patch
Permalink /patch/175921/
State New
Headers show

Comments

Paolo Bonzini - Aug. 8, 2012, 12:39 p.m.
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?
> 
> 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.

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(-)
Andreas Färber - Aug. 8, 2012, 12:48 p.m.
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
Paolo Bonzini - Aug. 8, 2012, 1:02 p.m.
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
Michael Tokarev - Aug. 8, 2012, 1:09 p.m.
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

Patch

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