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

[liu ping fan]

On Thu, Jul 4, 2013 at 12:36 AM, Andreas Färber <address@hidden> wrote:
> Am 03.07.2013 03:23, schrieb 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>
>
> It would be nice to get CC'ed on such proposals. :)
>
I will CC you for qom related topic. :)  And according to MAINTAINER,
I had better CCed maintainer of Device Tree.

>> 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
Do you mean the sequence in object_deinit()?
> may lead to realized = false accessing freed memory.
If my understanding as above is correct, we just need to guarantee
realized=false (e.g. pci_e1000_uninit )for  derived type will only
free the resource at its layer, and not touch its parent's, then it
can not access freed memory, right?

Regards,
Pingfan
>
> 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