Re: [Qemu-devel] [PATCH] qom: Use atomics for object refcounting

[liu ping fan]

On Wed, Jul 3, 2013 at 12:36 AM, Anthony Liguori <address@hidden> wrote:
> Paolo Bonzini <address@hidden> writes:
>
>> Il 02/07/2013 16:47, Anthony Liguori ha scritto:
>>> Jan Kiszka <address@hidden> writes:
>>>
>>>> Objects can soon be referenced/dereference outside the BQL. So we need
>>>> to use atomics in object_ref/unref.
>>>>
>>>> Based on patch by Liu Ping Fan.
>>>>
>>>> Signed-off-by: Jan Kiszka <address@hidden>
>>>> ---
>>>>  qom/object.c |    5 ++---
>>>>  1 files changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/qom/object.c b/qom/object.c
>>>> index 803b94b..a76a30b 100644
>>>> --- a/qom/object.c
>>>> +++ b/qom/object.c
>>>> @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char 
>>>> *implements_type,
>>>>
>>>>  void object_ref(Object *obj)
>>>>  {
>>>> -    obj->ref++;
>>>> +     __sync_fetch_and_add(&obj->ref, 1);
>>>>  }
>>>>
>>>>  void object_unref(Object *obj)
>>>>  {
>>>>      g_assert(obj->ref > 0);
>>>> -    obj->ref--;
>>>>
>>>>      /* parent always holds a reference to its children */
>>>> -    if (obj->ref == 0) {
>>>> +    if (__sync_sub_and_fetch(&obj->ref, 1) == 0) {
>>>>          object_finalize(obj);
>>>>      }
>>>>  }
>>>
>>> Should we introduce something akin to kref now that referencing counting
>>> has gotten fancy?
>>
>> I'm not a big fan of kref (it seems _too_ thin a wrapper to me, i.e. it
>> doesn't really wrap enough to be useful), but I wouldn't oppose it if
>> someone else does it.
>
> I had honestly hoped Object was light enough to be used for this
> purpose.  What do you think?
>
I think it is a good idea. So we can consider the object_finalize() as
the place to release everything. Take the DeviceState as example, we
will have

-- >8 --
Subject: [PATCH] qom: delay DeviceState destructor until object finialize

    Until refcnt->0, we know that the DeviceState can be safely dropped,
    so put the destructor there.

    Signed-off-by: Liu Ping Fan <address@hidden>

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6985ad8..1f4e5d8 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -794,9 +794,7 @@ static void device_unparent(Object *obj)
         bus = QLIST_FIRST(&dev->child_bus);
         qbus_free(bus);
     }
-    if (dev->realized) {
-        object_property_set_bool(obj, false, "realized", NULL);
-    }
+
     if (dev->parent_bus) {
         bus_remove_child(dev->parent_bus, dev);
         object_unref(OBJECT(dev->parent_bus));
diff --git a/qom/object.c b/qom/object.c
index 803b94b..2c945f0 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -393,6 +393,7 @@ static void object_finalize(void *data)
     Object *obj = data;
     TypeImpl *ti = obj->class->type;

+    object_property_set_bool(obj, false, "realized", NULL);
     object_deinit(obj, ti);
     object_property_del_all(obj);