From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49962) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UucK0-00052c-87 for qemu-devel@nongnu.org; Thu, 04 Jul 2013 01:43:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UucJz-0001oM-3I for qemu-devel@nongnu.org; Thu, 04 Jul 2013 01:43:12 -0400 Received: from cantor2.suse.de ([195.135.220.15]:59739 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UucJy-0001oE-Qq for qemu-devel@nongnu.org; Thu, 04 Jul 2013 01:43:11 -0400 Message-ID: <51D50B67.2040206@suse.de> Date: Thu, 04 Jul 2013 07:43:03 +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> <51D45303.2050100@suse.de> 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: Liu Ping Fan , Jan Kiszka , qemu-devel , Alexander Graf , Peter Crosthwaite , Anthony Liguori , Paolo Bonzini Am 04.07.2013 06:46, schrieb liu ping fan: > On Thu, Jul 4, 2013 at 12:36 AM, Andreas F=E4rber wr= ote: >> 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: >>>> >>>>> 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() a= s >>> the place to release everything. Take the DeviceState as example, we >>> will have >>> >>> -- >8 -- >>> Subject: [PATCH] qom: delay DeviceState destructor until object finia= lize >>> >>> Until refcnt->0, we know that the DeviceState can be safely dropp= ed, >>> so put the destructor there. >>> >>> Signed-off-by: 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 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 =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; >>> >>> + 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 =3D false accessing freed memory. > If my understanding as above is correct, we just need to guarantee > realized=3Dfalse (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=3Dfalse 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); >>> --=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