| Message ID | CAJnKYQ=tORXm==fNkt2g6Ag1jjOvWyvLMx=Bx_kkysB0KEiSHg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Am 03.07.2013 03:23, schrieb liu ping fan: > On Wed, Jul 3, 2013 at 12:36 AM, Anthony Liguori <anthony@codemonkey.ws> wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Il 02/07/2013 16:47, Anthony Liguori ha scritto: >>>> Jan Kiszka <jan.kiszka@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.kiszka@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 <pingfank@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); >
On Thu, Jul 4, 2013 at 12:36 AM, Andreas Färber <afaerber@suse.de> wrote: > Am 03.07.2013 03:23, schrieb liu ping fan: >> On Wed, Jul 3, 2013 at 12:36 AM, Anthony Liguori <anthony@codemonkey.ws> wrote: >>> Paolo Bonzini <pbonzini@redhat.com> writes: >>> >>>> Il 02/07/2013 16:47, Anthony Liguori ha scritto: >>>>> Jan Kiszka <jan.kiszka@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.kiszka@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 <pingfank@linux.vnet.ibm.com> > > 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. >> 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()? > 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? 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
Am 04.07.2013 06:46, schrieb liu ping fan: > On Thu, Jul 4, 2013 at 12:36 AM, Andreas Färber <afaerber@suse.de> wrote: >> Am 03.07.2013 03:23, schrieb liu ping fan: >>> On Wed, Jul 3, 2013 at 12:36 AM, Anthony Liguori <anthony@codemonkey.ws> wrote: >>>> Paolo Bonzini <pbonzini@redhat.com> 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 <pingfank@linux.vnet.ibm.com> >> >> 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); >>>
On Thu, Jul 4, 2013 at 1:43 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 04.07.2013 06:46, schrieb liu ping fan: >> On Thu, Jul 4, 2013 at 12:36 AM, Andreas Färber <afaerber@suse.de> 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
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); object_deinit(obj, ti); object_property_del_all(obj);