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 <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:
>>>>
>>>>> 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.

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