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

[Paolo Bonzini]

Il 02/07/2013 18:36, Anthony Liguori ha scritto:
> 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?

We should make it more robust against objects that are not in the QOM
composition tree (adding/removing the "child" property is relatively
slow).  As things stand, QOM is definitely too slow for something like
SCSIRequest.

In the long term, it is definitely nice to use Object more.  But if we
really had to abstract things, for now I'd just do

#define atomic_ref(x)              atomic_inc(x)
#define atomic_unref_test_zero(x)  (atomic_fetch_dec(x) == 1)

or something like that.

Paolo