Patchwork [3/9] qbus: remove glib_allocated/qom_allocated and use release hook to free memory

login
register
mail settings
Submitter Anthony Liguori
Date Aug. 26, 2012, 3:51 p.m.
Message ID <1345996298-4892-4-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/180103/
State New
Headers show

Comments

Anthony Liguori - Aug. 26, 2012, 3:51 p.m.
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/pci.c    |    7 ++++++-
 hw/qdev.c   |   15 ---------------
 hw/qdev.h   |    7 -------
 hw/sysbus.c |    7 ++++++-
 4 files changed, 12 insertions(+), 24 deletions(-)
Andreas Färber - Aug. 27, 2012, 2:02 p.m.
Am 26.08.2012 17:51, schrieb Anthony Liguori:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

That's a really nice solution for cleaning this up, thanks!

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

However one conceptional detail...

> ---
>  hw/pci.c    |    7 ++++++-
>  hw/qdev.c   |   15 ---------------
>  hw/qdev.h   |    7 -------
>  hw/sysbus.c |    7 ++++++-
>  4 files changed, 12 insertions(+), 24 deletions(-)
[...]
> diff --git a/hw/qdev.c b/hw/qdev.c
> index b5a52ac..6b61daa 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
[...]
> @@ -468,18 +466,6 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>      return bus;
>  }
>  
> -void qbus_free(BusState *bus)
> -{
> -    if (bus->qom_allocated) {
> -        object_delete(OBJECT(bus));
> -    } else {
> -        object_finalize(OBJECT(bus));
> -        if (bus->glib_allocated) {
> -            g_free(bus);
> -        }
> -    }
> -}
> -
>  static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
>  {
>      BusClass *bc = BUS_GET_CLASS(bus);
> @@ -698,7 +684,6 @@ static void device_finalize(Object *obj)
>      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);

I wonder how this is gonna work: The device used to be in charge of
tearing down its bus children ... now it neither deletes nor finalizes
nor unrefs? Is the while loop even still needed?

Wouldn't the busses still have the device as parent, referencing it,
blocking device_finalize?

Regards,
Andreas
Anthony Liguori - Aug. 27, 2012, 2:22 p.m.
Andreas Färber <afaerber@suse.de> writes:

> Am 26.08.2012 17:51, schrieb Anthony Liguori:
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> That's a really nice solution for cleaning this up, thanks!
>
> Acked-by: Andreas Färber <afaerber@suse.de>
>
> However one conceptional detail...
>
>> ---
>>  hw/pci.c    |    7 ++++++-
>>  hw/qdev.c   |   15 ---------------
>>  hw/qdev.h   |    7 -------
>>  hw/sysbus.c |    7 ++++++-
>>  4 files changed, 12 insertions(+), 24 deletions(-)
> [...]
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index b5a52ac..6b61daa 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
> [...]
>> @@ -468,18 +466,6 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>>      return bus;
>>  }
>>  
>> -void qbus_free(BusState *bus)
>> -{
>> -    if (bus->qom_allocated) {
>> -        object_delete(OBJECT(bus));
>> -    } else {
>> -        object_finalize(OBJECT(bus));
>> -        if (bus->glib_allocated) {
>> -            g_free(bus);
>> -        }
>> -    }
>> -}
>> -
>>  static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
>>  {
>>      BusClass *bc = BUS_GET_CLASS(bus);
>> @@ -698,7 +684,6 @@ static void device_finalize(Object *obj)
>>      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);
>
> I wonder how this is gonna work: The device used to be in charge of
> tearing down its bus children ... now it neither deletes nor finalizes
> nor unrefs? Is the while loop even still needed?
>
> Wouldn't the busses still have the device as parent, referencing it,
> blocking device_finalize?

This has never been right..  Just because a controller goes away, it
doesn't mean that the devices ought to go away too.

There are different types of "remove" so let's consider each.

1) Guest visible eject: if a controller is ejected, then the guest will
   obviously see everything behind it get removed too.  This is an
   emulation detail, not a QOM thing.

2) Final deletion: this only happens when all references go away.  If
   you eject a controller but there are still children that reference
   it, the controller won't go away.  You actually need to delete each
   individual disk (or whatever is behind it) in order to break the
   reference counting.

The eject notifier could walk the full bus and attempt to break the
connections but honestly, I'd much prefer that we deprecate the current
device_del interface and just do everything through QOM properties.
That would mean manually deleting all of the devices behind the bus if
that's really what you wanted to do.

Regards,

Anthony Liguori

>
> Regards,
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber - Aug. 27, 2012, 2:43 p.m.
Am 27.08.2012 16:22, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> I wonder how this is gonna work: The device used to be in charge of
>> tearing down its bus children ... now it neither deletes nor finalizes
>> nor unrefs? Is the while loop even still needed?
>>
>> Wouldn't the busses still have the device as parent, referencing it,
>> blocking device_finalize?
> 
> This has never been right..  Just because a controller goes away, it
> doesn't mean that the devices ought to go away too.
> 
> There are different types of "remove" so let's consider each.
> 
> 1) Guest visible eject: if a controller is ejected, then the guest will
>    obviously see everything behind it get removed too.  This is an
>    emulation detail, not a QOM thing.
> 
> 2) Final deletion: this only happens when all references go away.  If
>    you eject a controller but there are still children that reference
>    it, the controller won't go away.  You actually need to delete each
>    individual disk (or whatever is behind it) in order to break the
>    reference counting.
> 
> The eject notifier could walk the full bus and attempt to break the
> connections but honestly, I'd much prefer that we deprecate the current
> device_del interface and just do everything through QOM properties.
> That would mean manually deleting all of the devices behind the bus if
> that's really what you wanted to do.

I think we're talking about different scenarios here...

I was thinking

PCIHostState has-a PCIBus

(not PCIBus has-a PCIDevice) and final deletion.

In that case I would expect that it must be guaranteed that the device
that created the bus has access to the bus until it destroys it. But
IIUC the PCIHostState, once unparented from its SysBus (bad example!),
has a refcount of 1 (its PCIBus) thereby not being finalized?

I do understand your concept of refcounting matches what Java, .NET,
etc. do for objects but combined with the new QBus I feel this is
blurring the encapsulations and expected semantics of the device-centric
functions we have. To me the uninitfn means "the whole object goes away"
and is incompatible with "part of its children may stay behind if there
are still stray references to them"... we can no longer properly access
them then, only devices have canonical paths, so we'd risk piling up
garbage at runtime.

Regards,
Andreas

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 4d95984..437af70 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -292,6 +292,11 @@  void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
     vmstate_register(NULL, -1, &vmstate_pcibus, bus);
 }
 
+static void pci_bus_release(Object *obj)
+{
+    g_free(obj);
+}
+
 PCIBus *pci_bus_new(DeviceState *parent, const char *name,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
@@ -300,9 +305,9 @@  PCIBus *pci_bus_new(DeviceState *parent, const char *name,
     PCIBus *bus;
 
     bus = g_malloc0(sizeof(*bus));
-    bus->qbus.glib_allocated = true;
     pci_bus_new_inplace(bus, parent, name, address_space_mem,
                         address_space_io, devfn_min);
+    object_set_release_func(OBJECT(bus), pci_bus_release);
     return bus;
 }
 
diff --git a/hw/qdev.c b/hw/qdev.c
index b5a52ac..6b61daa 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -459,8 +459,6 @@  BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
     BusState *bus;
 
     bus = BUS(object_new(typename));
-    bus->qom_allocated = true;
-
     bus->parent = parent;
     bus->name = name ? g_strdup(name) : NULL;
     qbus_realize(bus);
@@ -468,18 +466,6 @@  BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
     return bus;
 }
 
-void qbus_free(BusState *bus)
-{
-    if (bus->qom_allocated) {
-        object_delete(OBJECT(bus));
-    } else {
-        object_finalize(OBJECT(bus));
-        if (bus->glib_allocated) {
-            g_free(bus);
-        }
-    }
-}
-
 static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
 {
     BusClass *bc = BUS_GET_CLASS(bus);
@@ -698,7 +684,6 @@  static void device_finalize(Object *obj)
     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);
diff --git a/hw/qdev.h b/hw/qdev.h
index d699194..3561e3a 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -106,17 +106,12 @@  typedef struct BusChild {
 
 /**
  * BusState:
- * @qom_allocated: Indicates whether the object was allocated by QOM.
- * @glib_allocated: Indicates whether the object was initialized in-place
- * yet is expected to be freed with g_free().
  */
 struct BusState {
     Object obj;
     DeviceState *parent;
     const char *name;
     int allow_hotplug;
-    bool qom_allocated;
-    bool glib_allocated;
     int max_index;
     QTAILQ_HEAD(ChildrenHead, BusChild) children;
     QLIST_ENTRY(BusState) sibling;
@@ -201,8 +196,6 @@  int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
 void qdev_reset_all(DeviceState *dev);
 void qbus_reset_all_fn(void *opaque);
 
-void qbus_free(BusState *bus);
-
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
 
 /* This should go away once we get rid of the NULL bus hack */
diff --git a/hw/sysbus.c b/hw/sysbus.c
index 9d8b1ea..8305337 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -267,6 +267,11 @@  static TypeInfo sysbus_device_type_info = {
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 static BusState *main_system_bus;
 
+static void main_system_bus_release(Object *obj)
+{
+    g_free(obj);
+}
+
 static void main_system_bus_create(void)
 {
     /* assign main_system_bus before qbus_create_inplace()
@@ -274,7 +279,7 @@  static void main_system_bus_create(void)
     main_system_bus = g_malloc0(system_bus_info.instance_size);
     qbus_create_inplace(main_system_bus, TYPE_SYSTEM_BUS, NULL,
                         "main-system-bus");
-    main_system_bus->glib_allocated = true;
+    object_set_release_func(OBJECT(main_system_bus), main_system_bus_release);
     object_property_add_child(container_get(qdev_get_machine(),
                                             "/unattached"),
                               "sysbus", OBJECT(main_system_bus), NULL);