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