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

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

[Jan Kiszka]

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

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

[Andreas Färber]

Am 02.07.2013 11:36, schrieb Jan Kiszka:
> 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);

How widespread are these in GCC/clang? Is there any fallback? I remember
seeing some __sync_* warnings on Mac OS X around 4.2...

Andreas

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

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

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

[Paolo Bonzini]

Il 02/07/2013 13:15, Andreas Färber ha scritto:
>> > @@ -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);
> How widespread are these in GCC/clang? Is there any fallback? I remember
> seeing some __sync_* warnings on Mac OS X around 4.2...

We are using them already in several places (vhost was the first one to
introduce them, I think, but now they are also in migration ad in some
tests too).  There is no fallback (asm could be a fallback, but we chose
to require GCC 4.2 or newer).

I'll change this to atomic_inc/dec when applying.  Otherwise

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

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

[Jan Kiszka]

On 2013-07-02 13:28, Paolo Bonzini wrote:
> Il 02/07/2013 13:15, Andreas Färber ha scritto:
>>>> @@ -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);
>> How widespread are these in GCC/clang? Is there any fallback? I remember
>> seeing some __sync_* warnings on Mac OS X around 4.2...
> 
> We are using them already in several places (vhost was the first one to
> introduce them, I think, but now they are also in migration ad in some
> tests too).  There is no fallback (asm could be a fallback, but we chose
> to require GCC 4.2 or newer).
> 
> I'll change this to atomic_inc/dec when applying.  Otherwise

But then atomic_dec_and_test or so. Letting the inc/dec return some
value leaves room for interpretations (value of before or after the
modification?).

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

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

[Paolo Bonzini]

Il 02/07/2013 13:44, Jan Kiszka ha scritto:
> On 2013-07-02 13:28, Paolo Bonzini wrote:
>> Il 02/07/2013 13:15, Andreas Färber ha scritto:
>>>>> @@ -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);
>>> How widespread are these in GCC/clang? Is there any fallback? I remember
>>> seeing some __sync_* warnings on Mac OS X around 4.2...
>>
>> We are using them already in several places (vhost was the first one to
>> introduce them, I think, but now they are also in migration ad in some
>> tests too).  There is no fallback (asm could be a fallback, but we chose
>> to require GCC 4.2 or newer).
>>
>> I'll change this to atomic_inc/dec when applying.  Otherwise
> 
> But then atomic_dec_and_test or so. Letting the inc/dec return some
> value leaves room for interpretations (value of before or after the
> modification?).

In qemu, I made all atomic_* functions return the old value.  This is
consistent with atomic_cmpxchg and atomic_xchg (where returning the new
value makes no sense).

Paolo

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

[Jan Kiszka]

On 2013-07-02 13:49, Paolo Bonzini wrote:
> Il 02/07/2013 13:44, Jan Kiszka ha scritto:
>> On 2013-07-02 13:28, Paolo Bonzini wrote:
>>> Il 02/07/2013 13:15, Andreas Färber ha scritto:
>>>>>> @@ -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);
>>>> How widespread are these in GCC/clang? Is there any fallback? I remember
>>>> seeing some __sync_* warnings on Mac OS X around 4.2...
>>>
>>> We are using them already in several places (vhost was the first one to
>>> introduce them, I think, but now they are also in migration ad in some
>>> tests too).  There is no fallback (asm could be a fallback, but we chose
>>> to require GCC 4.2 or newer).
>>>
>>> I'll change this to atomic_inc/dec when applying.  Otherwise
>>
>> But then atomic_dec_and_test or so. Letting the inc/dec return some
>> value leaves room for interpretations (value of before or after the
>> modification?).
> 
> In qemu, I made all atomic_* functions return the old value.  This is
> consistent with atomic_cmpxchg and atomic_xchg (where returning the new
> value makes no sense).

Please avoid this ambiguity by naming the functions properly. That xchg
returns old values is known, that dec and inc do, is surely not.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

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

[Paolo Bonzini]

Il 02/07/2013 13:52, Jan Kiszka ha scritto:
>>> But then atomic_dec_and_test or so. Letting the inc/dec return some
>>> >> value leaves room for interpretations (value of before or after the
>>> >> modification?).
>> > 
>> > In qemu, I made all atomic_* functions return the old value.  This is
>> > consistent with atomic_cmpxchg and atomic_xchg (where returning the new
>> > value makes no sense).
> Please avoid this ambiguity by naming the functions properly. That xchg
> returns old values is known, that dec and inc do, is surely not.

IMO the ambiguity is resolved simply by looking at the docs or existing
code, but I can rename them to atomic_fetch_{add,sub,and,or,inc,dec} and
add void versions without "fetch".

Paolo

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

[Anthony Liguori]

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?

Regards,

Anthony Liguori

> -- 
> 1.7.3.4

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

[Paolo Bonzini]

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.

Paolo

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

[Anthony Liguori]

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?

Regards,

Anthony Liguori

>
> Paolo

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

[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>

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

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

[Andreas Färber]

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

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 <afaerber@suse.de> wrote:
> 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. :)
>
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

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

[Andreas Färber]

Am 04.07.2013 06:46, schrieb liu ping fan:
> On Thu, Jul 4, 2013 at 12:36 AM, Andreas Färber <afaerber@suse.de> wrote:
>> 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:
>>>>
>>>>> 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. :)
>>
> I will CC you for qom related topic. :)  And according to MAINTAINER,
> I had better CCed maintainer of Device Tree.

Thanks. I was asking because I implemented realized and am working
towards adopting it in the tree.
Device Tree is something different (libfdt/dtc). We do not have
dedicated Device (formerly qdev) maintainers, Paolo and me have been
hacking on it as needed.

>>> 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()?

Yes.

>> 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?

For .instance_finalize you are right.

For realized, it is up to the derived type to choose when to call the
parent's realized implementation, e.g. a PCI device's unrealize
implementation will need to call PCIDevice's unrealize after its own
cleanups if it needs to access the config space or other resources
allocated/free at PCIDevice layer. I doubt we can make it a rule not to
touch the parent's resources at all.

But at least today, TYPE_OBJECT does not have an instance_finalize
implementation, so moving realized=false to
hw/core/qdev.c:device_finalize() instead may be an option - hoping Paolo
can comment more on device_unparent() vs. device_finalize() usage.

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

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

[liu ping fan]

On Thu, Jul 4, 2013 at 1:43 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 04.07.2013 06:46, schrieb liu ping fan:
>> On Thu, Jul 4, 2013 at 12:36 AM, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 03.07.2013 03:23, schrieb liu ping fan:
[...]
>>> 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.
>
> Thanks. I was asking because I implemented realized and am working
> towards adopting it in the tree.
> Device Tree is something different (libfdt/dtc). We do not have

Oh, sorry to disturb, Alexander Graf and Peter Crosthwaite :)
> dedicated Device (formerly qdev) maintainers, Paolo and me have been
> hacking on it as needed.
>
>>>> 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()?
>
> Yes.
>
>>> 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?
>
> For .instance_finalize you are right.
>
> For realized, it is up to the derived type to choose when to call the
> parent's realized implementation, e.g. a PCI device's unrealize
> implementation will need to call PCIDevice's unrealize after its own
> cleanups if it needs to access the config space or other resources
> allocated/free at PCIDevice layer. I doubt we can make it a rule not to
> touch the parent's resources at all.
>
I think we can make rules more simple. When device_finalize() called,
we will let realized=false, and this will reclaim e1000's extra
resource, and then pci extra resource. And there is no issue about
touching freed memory.

> But at least today, TYPE_OBJECT does not have an instance_finalize

Think it will not happen. Since instance_finalize is a hook for
derived object, as for Object, object_finalize is the one, right?
> implementation, so moving realized=false to
> hw/core/qdev.c:device_finalize() instead may be an option - hoping Paolo
> can comment more on device_unparent() vs. device_finalize() usage.
>
I guess device_unparent = isolate and device_finalize = reclaim
resource, basing on the understanding of Paolo's patches "Delay
destruction of memory regions to instance_finalize".

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

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

[Paolo Bonzini]

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

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