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 <address@hidden> wrote:
> 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:
[...]
>>> 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