[Qemu-devel] [PATCH 00/11] qdev: correct reference counting

[Qemu-devel] [PATCH 00/11] qdev: correct reference counting

[Paolo Bonzini]

This series makes the ref_count field of device and bus objects actually
match the number of references that the objects have.  Once this is done,
qdev_free and qbus_free are equivalent to simply object_unparent, and
object_delete can go.

Patches 1-3 fix some warts in the last minute patches that went in 1.3.
Patches 4-9 are the bulk of the series.  Patches 10-11 touches the CPU
classes instead.

Paolo

Paolo Bonzini (11):
  qdev: export and use qbus_init
  qdev: use object_new, not g_malloc to create buses
  qom: preserve object while unparenting it
  qdev: add reference count to a device for the BusChild
  qdev: move deletion of children from finalize to unparent
  qdev: move unrealization of devices from finalize to unparent
  qdev: add reference for the bus while it is referred to by the
    DeviceState
  qdev: inline object_delete into qbus_free/qdev_free
  qdev: drop extra references at creation time
  cpu: do not use object_delete
  qom: remove object_delete

 hw/pci.c              |  11 +++---
 hw/pci.h              |   5 ---
 hw/qdev-core.h        |   1 +
 hw/qdev-monitor.c     |   5 ++-
 hw/qdev.c             | 107 +++++++++++++++++++++++++++++---------------------
 hw/sysbus.c           |   6 +--
 include/qemu/object.h |  17 +-------
 linux-user/syscall.c  |   2 +-
 qom/object.c          |   9 +----
 target-i386/helper.c  |   4 +-
 target-sparc/cpu.c    |   2 +-
 vl.c                  |   1 +
 12 files changed, 84 insertions(+), 86 deletions(-)

-- 
1.8.0.1

[Qemu-devel] [PATCH 01/11] qdev: export and use qbus_init

[Paolo Bonzini]

BusState subclasses need to do their own allocation because
qbus_create_inplace calls object_initialize (which wipes out the
"free" callback).  This patch separates the initialization of the object
(object_initialize) from its insertion in the qdev tree (qbus_realize); to
do so, it moves the remaining bits of qbus_create_inplace to qbus_realize
and export it as qbus_init.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-core.h |  1 +
 hw/qdev.c      | 18 +++++++-----------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index fff7f0f..18f5f73 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -180,6 +180,7 @@ DeviceState *qdev_find_recursive(BusState *bus, const char *id);
 typedef int (qbus_walkerfn)(BusState *bus, void *opaque);
 typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
 
+void qbus_init(BusState *bus, DeviceState *parent, const char *name);
 void qbus_create_inplace(BusState *bus, const char *typename,
                          DeviceState *parent, const char *name);
 BusState *qbus_create(const char *typename, DeviceState *parent, const char *name);
diff --git a/hw/qdev.c b/hw/qdev.c
index 788b4da..e758131 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -403,14 +403,16 @@ DeviceState *qdev_find_recursive(BusState *bus, const char *id)
     return NULL;
 }
 
-static void qbus_realize(BusState *bus)
+void qbus_init(BusState *bus, DeviceState *parent, const char *name)
 {
     const char *typename = object_get_typename(OBJECT(bus));
     char *buf;
     int i,len;
 
-    if (bus->name) {
-        /* use supplied name */
+    bus->parent = parent;
+
+    if (name) {
+        bus->name = g_strdup(name);
     } else if (bus->parent && bus->parent->id) {
         /* parent device has id -> use it for bus name */
         len = strlen(bus->parent->id) + 16;
@@ -443,10 +445,7 @@ void qbus_create_inplace(BusState *bus, const char *typename,
                          DeviceState *parent, const char *name)
 {
     object_initialize(bus, typename);
-
-    bus->parent = parent;
-    bus->name = name ? g_strdup(name) : NULL;
-    qbus_realize(bus);
+    qbus_init(bus, parent, name);
 }
 
 BusState *qbus_create(const char *typename, DeviceState *parent, const char *name)
@@ -454,10 +453,7 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
     BusState *bus;
 
     bus = BUS(object_new(typename));
-
-    bus->parent = parent;
-    bus->name = name ? g_strdup(name) : NULL;
-    qbus_realize(bus);
+    qbus_init(bus, parent, name);
 
     return bus;
 }
-- 
1.8.0.1

Re: [Qemu-devel] [PATCH 01/11] qdev: export and use qbus_init

[Andreas Färber]

Am 05.12.2012 21:44, schrieb Paolo Bonzini:
> BusState subclasses need to do their own allocation because
> qbus_create_inplace calls object_initialize (which wipes out the
> "free" callback).  This patch separates the initialization of the object
> (object_initialize) from its insertion in the qdev tree (qbus_realize); to
> do so, it moves the remaining bits of qbus_create_inplace to qbus_realize
> and export it as qbus_init.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

The code movements look okay, but I wonder if we can resolve the "init"
ambivalence somehow? As in instance_init, which is being called
internally by object_new() and object_initialize().
You're right that it's not realize either. Maybe qbus_setup()?

Andreas

> ---
>  hw/qdev-core.h |  1 +
>  hw/qdev.c      | 18 +++++++-----------
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
> index fff7f0f..18f5f73 100644
> --- a/hw/qdev-core.h
> +++ b/hw/qdev-core.h
> @@ -180,6 +180,7 @@ DeviceState *qdev_find_recursive(BusState *bus, const char *id);
>  typedef int (qbus_walkerfn)(BusState *bus, void *opaque);
>  typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
>  
> +void qbus_init(BusState *bus, DeviceState *parent, const char *name);
>  void qbus_create_inplace(BusState *bus, const char *typename,
>                           DeviceState *parent, const char *name);
>  BusState *qbus_create(const char *typename, DeviceState *parent, const char *name);
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 788b4da..e758131 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -403,14 +403,16 @@ DeviceState *qdev_find_recursive(BusState *bus, const char *id)
>      return NULL;
>  }
>  
> -static void qbus_realize(BusState *bus)
> +void qbus_init(BusState *bus, DeviceState *parent, const char *name)
>  {
>      const char *typename = object_get_typename(OBJECT(bus));
>      char *buf;
>      int i,len;
>  
> -    if (bus->name) {
> -        /* use supplied name */
> +    bus->parent = parent;
> +
> +    if (name) {
> +        bus->name = g_strdup(name);
>      } else if (bus->parent && bus->parent->id) {
>          /* parent device has id -> use it for bus name */
>          len = strlen(bus->parent->id) + 16;
> @@ -443,10 +445,7 @@ void qbus_create_inplace(BusState *bus, const char *typename,
>                           DeviceState *parent, const char *name)
>  {
>      object_initialize(bus, typename);
> -
> -    bus->parent = parent;
> -    bus->name = name ? g_strdup(name) : NULL;
> -    qbus_realize(bus);
> +    qbus_init(bus, parent, name);
>  }
>  
>  BusState *qbus_create(const char *typename, DeviceState *parent, const char *name)
> @@ -454,10 +453,7 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>      BusState *bus;
>  
>      bus = BUS(object_new(typename));
> -
> -    bus->parent = parent;
> -    bus->name = name ? g_strdup(name) : NULL;
> -    qbus_realize(bus);
> +    qbus_init(bus, parent, name);
>  
>      return bus;
>  }
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 02/11] qdev: use object_new, not g_malloc to create buses

[Paolo Bonzini]

After the previous patch, creation of heap-allocated buses can
use qbus_init and does not need to hack the "free" callback.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci.c    | 11 +++++------
 hw/pci.h    |  5 -----
 hw/sysbus.c |  6 ++----
 3 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 97a0cd7..377a563 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -274,13 +274,13 @@ int pci_find_domain(const PCIBus *bus)
     return -1;
 }
 
-void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
+static void pci_bus_init(PCIBus *bus, DeviceState *parent,
                          const char *name,
                          MemoryRegion *address_space_mem,
                          MemoryRegion *address_space_io,
                          uint8_t devfn_min)
 {
-    qbus_create_inplace(&bus->qbus, TYPE_PCI_BUS, parent, name);
+    qbus_init(&bus->qbus, parent, name);
     assert(PCI_FUNC(devfn_min) == 0);
     bus->devfn_min = devfn_min;
     bus->address_space_mem = address_space_mem;
@@ -300,10 +300,9 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
 {
     PCIBus *bus;
 
-    bus = g_malloc0(sizeof(*bus));
-    pci_bus_new_inplace(bus, parent, name, address_space_mem,
-                        address_space_io, devfn_min);
-    OBJECT(bus)->free = g_free;
+    bus = object_new(TYPE_PCI_BUS);
+    pci_bus_init(bus, parent, name, address_space_mem,
+                 address_space_io, devfn_min);
     return bus;
 }
 
diff --git a/hw/pci.h b/hw/pci.h
index 4da0c2a..ab01574 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -306,11 +306,6 @@ typedef enum {
 
 typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
                               PCIHotplugState state);
-void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
-                         const char *name,
-                         MemoryRegion *address_space_mem,
-                         MemoryRegion *address_space_io,
-                         uint8_t devfn_min);
 PCIBus *pci_bus_new(DeviceState *parent, const char *name,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
diff --git a/hw/sysbus.c b/hw/sysbus.c
index ef8ffb6..a13d69e 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -271,10 +271,8 @@ static void main_system_bus_create(void)
 {
     /* assign main_system_bus before qbus_create_inplace()
      * in order to make "if (bus != sysbus_get_default())" work */
-    main_system_bus = g_malloc0(system_bus_info.instance_size);
-    qbus_create_inplace(main_system_bus, TYPE_SYSTEM_BUS, NULL,
-                        "main-system-bus");
-    OBJECT(main_system_bus)->free = g_free;
+    main_system_bus = object_new(TYPE_SYSTEM_BUS);
+    qbus_init(main_system_bus, NULL, "main-system-bus");
     object_property_add_child(container_get(qdev_get_machine(),
                                             "/unattached"),
                               "sysbus", OBJECT(main_system_bus), NULL);
-- 
1.8.0.1

[Qemu-devel] [PATCH 03/11] qom: preserve object while unparenting it

[Paolo Bonzini]

Avoid that the object disappears after it's deleted from the QOM
composition tree, in case that was the only reference to it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qom/object.c b/qom/object.c
index 0739aa2..ecdf164 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -360,12 +360,14 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
 
 void object_unparent(Object *obj)
 {
+    object_ref(obj);
     if (obj->parent) {
         object_property_del_child(obj->parent, obj, NULL);
     }
     if (obj->class->unparent) {
         (obj->class->unparent)(obj);
     }
+    object_unref(obj);
 }
 
 static void object_deinit(Object *obj, TypeImpl *type)
-- 
1.8.0.1

Re: [Qemu-devel] [PATCH 03/11] qom: preserve object while unparenting it

[Andreas Färber]

Am 05.12.2012 21:44, schrieb Paolo Bonzini:
> Avoid that the object disappears after it's deleted from the QOM
> composition tree, in case that was the only reference to it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 04/11] qdev: add reference count to a device for the BusChild

[Paolo Bonzini]

Each device has a reference through the BusChild.  This reference
was not accounted for, add it now.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/qdev.c b/hw/qdev.c
index e758131..87dfcb5 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -65,7 +65,10 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
 
             snprintf(name, sizeof(name), "child[%d]", kid->index);
             QTAILQ_REMOVE(&bus->children, kid, sibling);
+
+            /* This gives back ownership of kid->child back to us.  */
             object_property_del(OBJECT(bus), name, NULL);
+            object_unref(OBJECT(kid->child));
             g_free(kid);
             return;
         }
@@ -83,9 +86,11 @@ static void bus_add_child(BusState *bus, DeviceState *child)
 
     kid->index = bus->max_index++;
     kid->child = child;
+    object_ref(OBJECT(kid->child));
 
     QTAILQ_INSERT_HEAD(&bus->children, kid, sibling);
 
+    /* This transfers ownership of kid->child to the property.  */
     snprintf(name, sizeof(name), "child[%d]", kid->index);
     object_property_add_link(OBJECT(bus), name,
                              object_get_typename(OBJECT(child)),
-- 
1.8.0.1

Re: [Qemu-devel] [PATCH 04/11] qdev: add reference count to a device for the BusChild

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Each device has a reference through the BusChild.  This reference
> was not accounted for, add it now.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/qdev.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index e758131..87dfcb5 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -65,7 +65,10 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>  
>              snprintf(name, sizeof(name), "child[%d]", kid->index);
>              QTAILQ_REMOVE(&bus->children, kid, sibling);
> +
> +            /* This gives back ownership of kid->child back to us.  */
>              object_property_del(OBJECT(bus), name, NULL);
> +            object_unref(OBJECT(kid->child));
>              g_free(kid);
>              return;
>          }
> @@ -83,9 +86,11 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>  
>      kid->index = bus->max_index++;
>      kid->child = child;
> +    object_ref(OBJECT(kid->child));
>  
>      QTAILQ_INSERT_HEAD(&bus->children, kid, sibling);
>  
> +    /* This transfers ownership of kid->child to the property.  */
>      snprintf(name, sizeof(name), "child[%d]", kid->index);
>      object_property_add_link(OBJECT(bus), name,
>                               object_get_typename(OBJECT(child)),


The link property itself holds a reference no?  Or is this not the case
because we aren't checking a link when it's added and taking a reference
if it's !NULL?

Regards,

Anthony Liguori

> -- 
> 1.8.0.1

Re: [Qemu-devel] [PATCH 04/11] qdev: add reference count to a device for the BusChild

[Paolo Bonzini]

Il 07/01/2013 21:26, Anthony Liguori ha scritto:
>> > +    object_ref(OBJECT(kid->child));
>> >  
>> >      QTAILQ_INSERT_HEAD(&bus->children, kid, sibling);
>> >  
>> > +    /* This transfers ownership of kid->child to the property.  */
>> >      snprintf(name, sizeof(name), "child[%d]", kid->index);
>> >      object_property_add_link(OBJECT(bus), name,
>> >                               object_get_typename(OBJECT(child)),
> 
> The link property itself holds a reference no?  Or is this not the case
> because we aren't checking a link when it's added and taking a reference
> if it's !NULL?

Yes.  I think it's okay this way though, and anyway a separate series
that would touch all object_property_add_link callers.

Paolo

[Qemu-devel] [PATCH 05/11] qdev: move deletion of children from finalize to unparent

[Paolo Bonzini]

A device will never be finalized as long as it has a reference from
other devices that sit on its buses.  To ensure that the references
go away, deassociate a bus from its children in the unparent callback
for the bus.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 87dfcb5..12b1529 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -446,6 +446,25 @@ void qbus_init(BusState *bus, DeviceState *parent, const char *name)
     }
 }
 
+static void qbus_remove_children(Object *obj)
+{
+    BusState *bus = BUS(obj);
+    BusChild *kid;
+
+    while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
+        DeviceState *dev = kid->child;
+        qdev_free(dev);
+    }
+    if (bus->parent) {
+        QLIST_REMOVE(bus, sibling);
+        bus->parent->num_child_bus--;
+        bus->parent = NULL;
+    } else {
+        assert(bus != sysbus_get_default()); /* main_system_bus is never freed */
+        qemu_unregister_reset(qbus_reset_all_fn, bus);
+    }
+}
+
 void qbus_create_inplace(BusState *bus, const char *typename,
                          DeviceState *parent, const char *name)
 {
@@ -761,22 +780,15 @@ static void qbus_initfn(Object *obj)
     QTAILQ_INIT(&bus->children);
 }
 
+static void bus_class_init(ObjectClass *class, void *data)
+{
+    class->unparent = qbus_remove_children;
+}
+
 static void qbus_finalize(Object *obj)
 {
     BusState *bus = BUS(obj);
-    BusChild *kid;
 
-    while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
-        DeviceState *dev = kid->child;
-        qdev_free(dev);
-    }
-    if (bus->parent) {
-        QLIST_REMOVE(bus, sibling);
-        bus->parent->num_child_bus--;
-    } else {
-        assert(bus != sysbus_get_default()); /* main_system_bus is never freed */
-        qemu_unregister_reset(qbus_reset_all_fn, bus);
-    }
     g_free((char *)bus->name);
 }
 
@@ -788,6 +800,7 @@ static const TypeInfo bus_info = {
     .class_size = sizeof(BusClass),
     .instance_init = qbus_initfn,
     .instance_finalize = qbus_finalize,
+    .class_init = bus_class_init,
 };
 
 static void qdev_register_types(void)
-- 
1.8.0.1

[Qemu-devel] [PATCH 06/11] qdev: move unrealization of devices from finalize to unparent

[Paolo Bonzini]

Similarly, a bus holds a reference back to the device, and this will
prevent the device from going away as soon as this reference is counted
properly.  To avoid this, move the unrealization of devices to the
unparent callback.  This includes recursively unparenting all the buses
and (after the previous patch) the devices on those buses, which ensures
that the web of references completely disappears for all devices that
reside (in the qdev tree) below the one being unplugged.

After this patch, the qdev tree and the bus<->child relationship is
defined as "A is above B, iff unplugging A will automatically unplug B".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 12b1529..d7f1545 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -699,23 +699,8 @@ static void device_initfn(Object *obj)
 static void device_finalize(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
-    BusState *bus;
-    DeviceClass *dc = DEVICE_GET_CLASS(dev);
-
-    if (dev->state == DEV_STATE_INITIALIZED) {
-        while (dev->num_child_bus) {
-            bus = QLIST_FIRST(&dev->child_bus);
-            qbus_free(bus);
-        }
-        if (qdev_get_vmsd(dev)) {
-            vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
-        }
-        if (dc->exit) {
-            dc->exit(dev);
-        }
-        if (dev->opts) {
-            qemu_opts_del(dev->opts);
-        }
+    if (dev->opts) {
+        qemu_opts_del(dev->opts);
     }
 }
 
@@ -732,8 +717,24 @@ static void device_class_base_init(ObjectClass *class, void *data)
 static void qdev_remove_from_bus(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
+    BusState *bus;
 
-    bus_remove_child(dev->parent_bus, dev);
+    while (dev->num_child_bus) {
+        bus = QLIST_FIRST(&dev->child_bus);
+        qbus_free(bus);
+    }
+    if (dev->state == DEV_STATE_INITIALIZED) {
+        if (qdev_get_vmsd(dev)) {
+            vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
+        }
+        if (dc->exit) {
+            dc->exit(dev);
+        }
+    }
+    if (dev->parent_bus) {
+        bus_remove_child(dev->parent_bus, dev);
+    }
 }
 
 static void device_class_init(ObjectClass *class, void *data)
-- 
1.8.0.1

[Qemu-devel] [PATCH 07/11] qdev: add reference for the bus while it is referred to by the DeviceState

[Paolo Bonzini]

Now that the unparent callbacks are complete, we can correctly account
more missing references.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/qdev.c b/hw/qdev.c
index d7f1545..62b6c14 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -101,6 +101,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
 void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
 {
     dev->parent_bus = bus;
+    object_ref(OBJECT(bus));
     bus_add_child(bus, dev);
 }
 
@@ -734,6 +735,8 @@ static void qdev_remove_from_bus(Object *obj)
     }
     if (dev->parent_bus) {
         bus_remove_child(dev->parent_bus, dev);
+        object_unref(OBJECT(dev->parent_bus));
+        dev->parent_bus = NULL;
     }
 }
 
-- 
1.8.0.1

Re: [Qemu-devel] [PATCH 07/11] qdev: add reference for the bus while it is referred to by the DeviceState

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Now that the unparent callbacks are complete, we can correctly account
> more missing references.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/qdev.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index d7f1545..62b6c14 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -101,6 +101,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>  void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>  {
>      dev->parent_bus = bus;
> +    object_ref(OBJECT(bus));
>      bus_add_child(bus, dev);

If we make parent_bus a link property, we'll get the reference counting
for free.

Regards,

Anthony Liguori

>  }
>  
> @@ -734,6 +735,8 @@ static void qdev_remove_from_bus(Object *obj)
>      }
>      if (dev->parent_bus) {
>          bus_remove_child(dev->parent_bus, dev);
> +        object_unref(OBJECT(dev->parent_bus));
> +        dev->parent_bus = NULL;
>      }
>  }
>  
> -- 
> 1.8.0.1

Re: [Qemu-devel] [PATCH 07/11] qdev: add reference for the bus while it is referred to by the DeviceState

[Paolo Bonzini]

Il 07/01/2013 21:29, Anthony Liguori ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Now that the unparent callbacks are complete, we can correctly account
>> more missing references.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/qdev.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index d7f1545..62b6c14 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -101,6 +101,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>>  void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>>  {
>>      dev->parent_bus = bus;
>> +    object_ref(OBJECT(bus));
>>      bus_add_child(bus, dev);
> 
> If we make parent_bus a link property, we'll get the reference counting
> for free.

True, but I think this should be a separate series.

Paolo

> Regards,
> 
> Anthony Liguori
> 
>>  }
>>  
>> @@ -734,6 +735,8 @@ static void qdev_remove_from_bus(Object *obj)
>>      }
>>      if (dev->parent_bus) {
>>          bus_remove_child(dev->parent_bus, dev);
>> +        object_unref(OBJECT(dev->parent_bus));
>> +        dev->parent_bus = NULL;
>>      }
>>  }
>>  
>> -- 
>> 1.8.0.1
> 

[Qemu-devel] [PATCH 08/11] qdev: inline object_delete into qbus_free/qdev_free

[Paolo Bonzini]

We want object_delete to disappear, and we will do this one class at a
time.  Inline it for the qdev case, which we will tackle first.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 62b6c14..dcd39b3 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -268,7 +268,8 @@ void qdev_init_nofail(DeviceState *dev)
 /* Unlink device from bus and free the structure.  */
 void qdev_free(DeviceState *dev)
 {
-    object_delete(OBJECT(dev));
+    object_unparent(OBJECT(dev));
+    object_unref(OBJECT(dev));
 }
 
 void qdev_machine_creation_done(void)
@@ -485,7 +486,8 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
 
 void qbus_free(BusState *bus)
 {
-    object_delete(OBJECT(bus));
+    object_unparent(OBJECT(bus));
+    object_unref(OBJECT(bus));
 }
 
 static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
-- 
1.8.0.1

[Qemu-devel] [PATCH 09/11] qdev: drop extra references at creation time

[Paolo Bonzini]

qdev_free and qbus_free have to do unparent+unref, because nobody else
drops the initial reference (the one included by object_initialize)
before them.

For device_init_func and do_device_add, this is trivially correct,
since the DeviceState goes out of scope.

For qdev_create, qdev_try_create and qbus_init, it is a bit more tricky.
What we are doing here is just assuming that the caller knows what it's
doing, and won't call qdev_free/qbus_free while the device is still there.
This is a pretty reasonable assumption and (behind the scenes) is also
what GObject/GTK does.  GTK actually has a "floating reference" that
goes away as soon as the caller does gtk_container_add or something
like that, but in the end qbus_init and qdev_try_create are already
adding the new object to its qdev parent!  So in the end the two solutions
are the same.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-monitor.c | 5 ++++-
 hw/qdev.c         | 5 ++---
 vl.c              | 1 +
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index a1b4d6a..ed62fd3 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -578,6 +578,7 @@ int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     Error *local_err = NULL;
     QemuOpts *opts;
+    DeviceState *dev;
 
     opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
     if (error_is_set(&local_err)) {
@@ -589,10 +590,12 @@ int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
         qemu_opts_del(opts);
         return 0;
     }
-    if (!qdev_device_add(opts)) {
+    dev = qdev_device_add(opts);
+    if (!dev) {
         qemu_opts_del(opts);
         return -1;
     }
+    object_unref(OBJECT(dev));
     return 0;
 }
 
diff --git a/hw/qdev.c b/hw/qdev.c
index dcd39b3..9f4fd92 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -142,7 +142,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
     }
 
     qdev_set_parent_bus(dev, bus);
-
+    object_unref(OBJECT(dev));
     return dev;
 }
 
@@ -269,7 +269,6 @@ void qdev_init_nofail(DeviceState *dev)
 void qdev_free(DeviceState *dev)
 {
     object_unparent(OBJECT(dev));
-    object_unref(OBJECT(dev));
 }
 
 void qdev_machine_creation_done(void)
@@ -441,6 +440,7 @@ void qbus_init(BusState *bus, DeviceState *parent, const char *name)
         QLIST_INSERT_HEAD(&bus->parent->child_bus, bus, sibling);
         bus->parent->num_child_bus++;
         object_property_add_child(OBJECT(bus->parent), bus->name, OBJECT(bus), NULL);
+        object_unref(OBJECT(bus));
     } else if (bus != sysbus_get_default()) {
         /* TODO: once all bus devices are qdevified,
            only reset handler for main_system_bus should be registered here. */
@@ -487,7 +487,6 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
 void qbus_free(BusState *bus)
 {
     object_unparent(OBJECT(bus));
-    object_unref(OBJECT(bus));
 }
 
 static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
diff --git a/vl.c b/vl.c
index a3ab384..71c339c 100644
--- a/vl.c
+++ b/vl.c
@@ -2052,6 +2052,7 @@ static int device_init_func(QemuOpts *opts, void *opaque)
     dev = qdev_device_add(opts);
     if (!dev)
         return -1;
+    object_unref(OBJECT(dev));
     return 0;
 }
 
-- 
1.8.0.1

[Qemu-devel] [PATCH 10/11] cpu: do not use object_delete

[Paolo Bonzini]

CPUs are never added to the composition tree, so delete is achieved
simply by removing the last references to them.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 linux-user/syscall.c | 2 +-
 target-i386/helper.c | 4 ++--
 target-sparc/cpu.c   | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e4291ed..64b2610 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5188,7 +5188,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                         NULL, NULL, 0);
           }
           thread_env = NULL;
-          object_delete(OBJECT(ENV_GET_CPU(cpu_env)));
+          object_unref(OBJECT(ENV_GET_CPU(cpu_env)));
           g_free(ts);
           pthread_exit(NULL);
       }
diff --git a/target-i386/helper.c b/target-i386/helper.c
index bf206cf..d628352 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1251,14 +1251,14 @@ X86CPU *cpu_x86_init(const char *cpu_model)
     env->cpu_model_str = cpu_model;
 
     if (cpu_x86_register(cpu, cpu_model) < 0) {
-        object_delete(OBJECT(cpu));
+        object_unref(OBJECT(cpu));
         return NULL;
     }
 
     x86_cpu_realize(OBJECT(cpu), &error);
     if (error) {
         error_free(error);
-        object_delete(OBJECT(cpu));
+        object_unref(OBJECT(cpu));
         return NULL;
     }
     return cpu;
diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index 882d306..b38641b 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -119,7 +119,7 @@ SPARCCPU *cpu_sparc_init(const char *cpu_model)
     }
 
     if (cpu_sparc_register(env, cpu_model) < 0) {
-        object_delete(OBJECT(cpu));
+        object_unref(OBJECT(cpu));
         return NULL;
     }
     qemu_init_vcpu(env);
-- 
1.8.0.1

Re: [Qemu-devel] [PATCH 10/11] cpu: do not use object_delete

[Andreas Färber]

Am 05.12.2012 21:44, schrieb Paolo Bonzini:
> CPUs are never added to the composition tree, so delete is achieved
> simply by removing the last references to them.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Note however that x86 is undergoing major changes and other targets are
growing needs to ... "deconstruct" :) CPUs, too. So this is very likely
to conflict. As I understand it, I can't just unparent a CPU today as it
would not get freed?

Andreas

> ---
>  linux-user/syscall.c | 2 +-
>  target-i386/helper.c | 4 ++--
>  target-sparc/cpu.c   | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index e4291ed..64b2610 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5188,7 +5188,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                          NULL, NULL, 0);
>            }
>            thread_env = NULL;
> -          object_delete(OBJECT(ENV_GET_CPU(cpu_env)));
> +          object_unref(OBJECT(ENV_GET_CPU(cpu_env)));
>            g_free(ts);
>            pthread_exit(NULL);
>        }
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index bf206cf..d628352 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1251,14 +1251,14 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>      env->cpu_model_str = cpu_model;
>  
>      if (cpu_x86_register(cpu, cpu_model) < 0) {
> -        object_delete(OBJECT(cpu));
> +        object_unref(OBJECT(cpu));
>          return NULL;
>      }
>  
>      x86_cpu_realize(OBJECT(cpu), &error);
>      if (error) {
>          error_free(error);
> -        object_delete(OBJECT(cpu));
> +        object_unref(OBJECT(cpu));
>          return NULL;
>      }
>      return cpu;
> diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
> index 882d306..b38641b 100644
> --- a/target-sparc/cpu.c
> +++ b/target-sparc/cpu.c
> @@ -119,7 +119,7 @@ SPARCCPU *cpu_sparc_init(const char *cpu_model)
>      }
>  
>      if (cpu_sparc_register(env, cpu_model) < 0) {
> -        object_delete(OBJECT(cpu));
> +        object_unref(OBJECT(cpu));
>          return NULL;
>      }
>      qemu_init_vcpu(env);

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 10/11] cpu: do not use object_delete

[Paolo Bonzini]

Il 18/12/2012 22:07, Andreas Färber ha scritto:
>> > CPUs are never added to the composition tree, so delete is achieved
>> > simply by removing the last references to them.
>> > 
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> 
> Note however that x86 is undergoing major changes and other targets are
> growing needs to ... "deconstruct" :) CPUs, too. So this is very likely
> to conflict. As I understand it, I can't just unparent a CPU today as it
> would not get freed?

Honestly I don't remember. :)

Paolo

[Qemu-devel] [PATCH 11/11] qom: remove object_delete

[Paolo Bonzini]

This is now unused.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/object.h | 17 ++---------------
 qom/object.c          |  7 -------
 2 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index ed1f47f..c5169ee 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -455,9 +455,7 @@ struct InterfaceClass
  * object_new:
  * @typename: The name of the type of the object to instantiate.
  *
- * This function will initialize a new object using heap allocated memory.  This
- * function should be paired with object_delete() to free the resources
- * associated with the object.
+ * This function will initialize a new object using heap allocated memory.
  *
  * Returns: The newly allocated and instantiated object.
  */
@@ -467,24 +465,13 @@ Object *object_new(const char *typename);
  * object_new_with_type:
  * @type: The type of the object to instantiate.
  *
- * This function will initialize a new object using heap allocated memory.  This
- * function should be paired with object_delete() to free the resources
- * associated with the object.
+ * This function will initialize a new object using heap allocated memory.
  *
  * Returns: The newly allocated and instantiated object.
  */
 Object *object_new_with_type(Type type);
 
 /**
- * object_delete:
- * @obj: The object to free.
- *
- * Finalize an object and then free the memory associated with it.  This should
- * be paired with object_new() to free the resources associated with an object.
- */
-void object_delete(Object *obj);
-
-/**
  * object_initialize_with_type:
  * @obj: A pointer to the memory to be used for the object.
  * @type: The type of the object to instantiate.
diff --git a/qom/object.c b/qom/object.c
index ecdf164..e9212fd 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -416,13 +416,6 @@ Object *object_new(const char *typename)
     return object_new_with_type(ti);
 }
 
-void object_delete(Object *obj)
-{
-    object_unparent(obj);
-    g_assert(obj->ref == 1);
-    object_unref(obj);
-}
-
 Object *object_dynamic_cast(Object *obj, const char *typename)
 {
     if (obj && object_class_dynamic_cast(object_get_class(obj), typename)) {
-- 
1.8.0.1

Re: [Qemu-devel] [PATCH 00/11] qdev: correct reference counting

[Paolo Bonzini]

Il 05/12/2012 21:44, Paolo Bonzini ha scritto:
> This series makes the ref_count field of device and bus objects actually
> match the number of references that the objects have.  Once this is done,
> qdev_free and qbus_free are equivalent to simply object_unparent, and
> object_delete can go.
> 
> Patches 1-3 fix some warts in the last minute patches that went in 1.3.
> Patches 4-9 are the bulk of the series.  Patches 10-11 touches the CPU
> classes instead.
> 
> Paolo
> 
> Paolo Bonzini (11):
>   qdev: export and use qbus_init
>   qdev: use object_new, not g_malloc to create buses
>   qom: preserve object while unparenting it
>   qdev: add reference count to a device for the BusChild
>   qdev: move deletion of children from finalize to unparent
>   qdev: move unrealization of devices from finalize to unparent
>   qdev: add reference for the bus while it is referred to by the
>     DeviceState
>   qdev: inline object_delete into qbus_free/qdev_free
>   qdev: drop extra references at creation time
>   cpu: do not use object_delete
>   qom: remove object_delete
> 
>  hw/pci.c              |  11 +++---
>  hw/pci.h              |   5 ---
>  hw/qdev-core.h        |   1 +
>  hw/qdev-monitor.c     |   5 ++-
>  hw/qdev.c             | 107 +++++++++++++++++++++++++++++---------------------
>  hw/sysbus.c           |   6 +--
>  include/qemu/object.h |  17 +-------
>  linux-user/syscall.c  |   2 +-
>  qom/object.c          |   9 +----
>  target-i386/helper.c  |   4 +-
>  target-sparc/cpu.c    |   2 +-
>  vl.c                  |   1 +
>  12 files changed, 84 insertions(+), 86 deletions(-)
> 

Ping?

Paolo

Re: [Qemu-devel] [PATCH 00/11] qdev: correct reference counting

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This series makes the ref_count field of device and bus objects actually
> match the number of references that the objects have.  Once this is done,
> qdev_free and qbus_free are equivalent to simply object_unparent, and
> object_delete can go.
>
> Patches 1-3 fix some warts in the last minute patches that went in 1.3.
> Patches 4-9 are the bulk of the series.  Patches 10-11 touches the CPU
> classes instead.

Other than the few minor comments:

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Thanks for spending time on this.  It's a PITA to get right.

Regards,

Anthony Liguori

>
> Paolo
>
> Paolo Bonzini (11):
>   qdev: export and use qbus_init
>   qdev: use object_new, not g_malloc to create buses
>   qom: preserve object while unparenting it
>   qdev: add reference count to a device for the BusChild
>   qdev: move deletion of children from finalize to unparent
>   qdev: move unrealization of devices from finalize to unparent
>   qdev: add reference for the bus while it is referred to by the
>     DeviceState
>   qdev: inline object_delete into qbus_free/qdev_free
>   qdev: drop extra references at creation time
>   cpu: do not use object_delete
>   qom: remove object_delete
>
>  hw/pci.c              |  11 +++---
>  hw/pci.h              |   5 ---
>  hw/qdev-core.h        |   1 +
>  hw/qdev-monitor.c     |   5 ++-
>  hw/qdev.c             | 107 +++++++++++++++++++++++++++++---------------------
>  hw/sysbus.c           |   6 +--
>  include/qemu/object.h |  17 +-------
>  linux-user/syscall.c  |   2 +-
>  qom/object.c          |   9 +----
>  target-i386/helper.c  |   4 +-
>  target-sparc/cpu.c    |   2 +-
>  vl.c                  |   1 +
>  12 files changed, 84 insertions(+), 86 deletions(-)
>
> -- 
> 1.8.0.1