From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47554) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UuQ2c-0002c1-Hc for qemu-devel@nongnu.org; Wed, 03 Jul 2013 12:36:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UuQ2b-00079J-0B for qemu-devel@nongnu.org; Wed, 03 Jul 2013 12:36:26 -0400 Received: from cantor2.suse.de ([195.135.220.15]:40717 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UuQ2a-00079E-Na for qemu-devel@nongnu.org; Wed, 03 Jul 2013 12:36:24 -0400 Message-ID: <51D45303.2050100@suse.de> Date: Wed, 03 Jul 2013 18:36:19 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <51D29F27.9040706@siemens.com> <87bo6lqarq.fsf@codemonkey.ws> <51D2F2D5.90801@redhat.com> <8761wsc426.fsf@codemonkey.ws> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] qom: Use atomics for object refcounting List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: liu ping fan Cc: Paolo Bonzini , Liu Ping Fan , qemu-devel , Anthony Liguori , Jan Kiszka Am 03.07.2013 03:23, schrieb liu ping fan: > On Wed, Jul 3, 2013 at 12:36 AM, Anthony Liguori wrote: >> Paolo Bonzini writes: >> >>> Il 02/07/2013 16:47, Anthony Liguori ha scritto: >>>> Jan Kiszka writes: >>>> >>>>> Objects can soon be referenced/dereference outside the BQL. So we n= eed >>>>> to use atomics in object_ref/unref. >>>>> >>>>> Based on patch by Liu Ping Fan. >>>>> >>>>> Signed-off-by: Jan Kiszka >>>>> --- >>>>> 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 *imp= lements_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 =3D=3D 0) { >>>>> + if (__sync_sub_and_fetch(&obj->ref, 1) =3D=3D 0) { >>>>> object_finalize(obj); >>>>> } >>>>> } >>>> >>>> Should we introduce something akin to kref now that referencing coun= ting >>>> 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 >=20 > -- >8 -- > Subject: [PATCH] qom: delay DeviceState destructor until object finiali= ze >=20 > Until refcnt->0, we know that the DeviceState can be safely dropped= , > so put the destructor there. >=20 > Signed-off-by: Liu Ping Fan 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 =3D 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 =3D data; > TypeImpl *ti =3D obj->class->type; >=20 > + 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 =3D false accessing freed memory. Regards, Andreas > object_deinit(obj, ti); > object_property_del_all(obj); >=20 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg