Patchwork [05/10] qdev: finalize of qbus, qdev will not the right place to free children

login
register
mail settings
Submitter pingfan liu
Date Aug. 24, 2012, 9:49 a.m.
Message ID <1345801763-24227-6-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/179811/
State New
Headers show

Comments

pingfan liu - Aug. 24, 2012, 9:49 a.m.
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

When breaking big lock, the child object can be hold by mmio-dispatch,
and it is not right to free them when their parent gone.
We will isolate and release the children by qdev_delete_subtree(),
and let each object manage its own life cycle.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/qdev.c |   30 ++++++++++++------------------
 1 files changed, 12 insertions(+), 18 deletions(-)
Paolo Bonzini - Aug. 24, 2012, 2:50 p.m.
Il 24/08/2012 11:49, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> When breaking big lock, the child object can be hold by mmio-dispatch,
> and it is not right to free them when their parent gone.
> We will isolate and release the children by qdev_delete_subtree(),
> and let each object manage its own life cycle.

I think this makes sense, but is it already working at this point of the
series?  That is, before patch 7 starts using qdev_delete_subtree()?

Paolo

> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/qdev.c |   30 ++++++++++++------------------
>  1 files changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 570f0bf..d6c8130 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -756,10 +756,8 @@ static void device_finalize(Object *obj)
>      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);
> -        }
> +
> +        g_assert(QLIST_EMPTY(&dev->child_bus));
>          if (qdev_get_vmsd(dev)) {
>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>          }
> @@ -770,9 +768,8 @@ static void device_finalize(Object *obj)
>              qemu_opts_del(dev->opts);
>          }
>      }
> -    if (dev->parent_bus) {
> -        bus_remove_child(dev->parent_bus, dev);
> -    }
> +
> +    g_assert((dev->parent_bus == NULL));
>  }
>  
>  static void device_class_base_init(ObjectClass *class, void *data)
> @@ -826,19 +823,16 @@ static void qbus_initfn(Object *obj)
>  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);
> +    assert(bus != sysbus_get_default()); /* main_system_bus is never freed */
> +    /* just blind check, since bus->parent has been set to NULL */
> +    qemu_unregister_reset(qbus_reset_all_fn, bus);
> +    if (bus->overlap != NULL) {
> +        object_unref(OBJECT(bus->overlap));
>      }
> +    assert(QTAILQ_EMPTY(&bus->children));
> +    /* have reset it to NULL, to prevent reader */
> +    assert((bus->parent == NULL));
>      g_free((char *)bus->name);
>  }
>  
>

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index 570f0bf..d6c8130 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -756,10 +756,8 @@  static void device_finalize(Object *obj)
     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);
-        }
+
+        g_assert(QLIST_EMPTY(&dev->child_bus));
         if (qdev_get_vmsd(dev)) {
             vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
         }
@@ -770,9 +768,8 @@  static void device_finalize(Object *obj)
             qemu_opts_del(dev->opts);
         }
     }
-    if (dev->parent_bus) {
-        bus_remove_child(dev->parent_bus, dev);
-    }
+
+    g_assert((dev->parent_bus == NULL));
 }
 
 static void device_class_base_init(ObjectClass *class, void *data)
@@ -826,19 +823,16 @@  static void qbus_initfn(Object *obj)
 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);
+    assert(bus != sysbus_get_default()); /* main_system_bus is never freed */
+    /* just blind check, since bus->parent has been set to NULL */
+    qemu_unregister_reset(qbus_reset_all_fn, bus);
+    if (bus->overlap != NULL) {
+        object_unref(OBJECT(bus->overlap));
     }
+    assert(QTAILQ_EMPTY(&bus->children));
+    /* have reset it to NULL, to prevent reader */
+    assert((bus->parent == NULL));
     g_free((char *)bus->name);
 }