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

["Andreas Färber" <afaerber@suse.de>]

Am 03.07.2013 03:23, schrieb liu ping fan:
> On Wed, Jul 3, 2013 at 12:36 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 02/07/2013 16:47, Anthony Liguori ha scritto:
>>>> Jan Kiszka <jan.kiszka@siemens.com> 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 <jan.kiszka@siemens.com>
>>>>> ---
>>>>>  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 <pingfank@linux.vnet.ibm.com>

It would be nice to get CC'ed on such proposals. :)

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

This is incorrect since we specifically only have "realized" for
devices, not for all QOM objects.

If we want to move it to the finalizer you'll need to use
.instance_finalize on the device type in hw/core/qdev.c.
However the derived type's finalizer is run before its parent's, which
may lead to realized = false accessing freed memory.

Regards,
Andreas

>      object_deinit(obj, ti);
>      object_property_del_all(obj);
> 


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