qom: Use atomics for object refcounting
PATCH 1/1 qom: Use atomics for object refcounting
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.
--- qom/object.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
qom/object.c +2 -3
@@ -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); } }
Signature · 1 line
1.7.3.4
Paolo Bonzini
Reviewed-byIl 02/07/2013 13:15, Andreas Färber ha scritto:
Quoted 8 lines
Quoted 6 lines
qom/object.c GSList *object_class_get_list(const char *implements_type, · line 686
Quoted 6 lines
qom/object.c +1
-1@@ -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 );How widespread are these in GCC/clang? Is there any fallback? I remember seeing some __sync_* warnings on Mac OS X around 4.2...
We are using them already in several places (vhost was the first one to introduce them, I think, but now they are also in migration ad in some tests too). There is no fallback (asm could be a fallback, but we chose to require GCC 4.2 or newer).
I'll change this to atomic_inc/dec when applying. Otherwise
Paolo
Am 02.07.2013 11:36, schrieb Jan Kiszka:
Quoted 15 lines
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 <[email protected]>--- qom/object.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)qom/object.c +0
-0
qom/object.c GSList *object_class_get_list(const char *implements_type, · line 686
Quoted 5 lines
qom/object.c +1
-1@@ -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 );
How widespread are these in GCC/clang? Is there any fallback? I remember seeing some __sync_* warnings on Mac OS X around 4.2...
Andreas
Quoted 13 lines
qom/object.c +1
-2}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); } }
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Quoted 10 lines · Paolo Bonzini
Il 02/07/2013 13:15, Andreas Färber ha scritto:
Quoted 8 lines
Quoted 6 lines
qom/object.c GSList *object_class_get_list(const char *implements_type, · line 686
Quoted 6 lines
qom/object.c +1
-1@@ -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 );How widespread are these in GCC/clang? Is there any fallback? I remember seeing some __sync_* warnings on Mac OS X around 4.2...
Quoted 6 lines
We are using them already in several places (vhost was the first one to introduce them, I think, but now they are also in migration ad in some tests too). There is no fallback (asm could be a fallback, but we chose to require GCC 4.2 or newer).
I'll change this to atomic_inc/dec when applying. Otherwise
But then atomic_dec_and_test or so. Letting the inc/dec return some value leaves room for interpretations (value of before or after the modification?).
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux
Il 02/07/2013 13:44, Jan Kiszka ha scritto:
Quoted 16 lines · Paolo Bonzini
Quoted 15 lines
Il 02/07/2013 13:15, Andreas Färber ha scritto:
Quoted 8 lines
Quoted 6 lines
qom/object.c GSList *object_class_get_list(const char *implements_type, · line 686
Quoted 6 lines
qom/object.c +1
-1@@ -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 );How widespread are these in GCC/clang? Is there any fallback? I remember seeing some __sync_* warnings on Mac OS X around 4.2...
We are using them already in several places (vhost was the first one to introduce them, I think, but now they are also in migration ad in some tests too). There is no fallback (asm could be a fallback, but we chose to require GCC 4.2 or newer).
Quoted 5 lines
Quoted 1 line
I'll change this to atomic_inc/dec when applying. Otherwise
But then
atomic_dec_and_testor so. Letting the inc/dec return some value leaves room for interpretations (value of before or after the modification?).
In qemu, I made all atomic_* functions return the old value. This is consistent with atomic_cmpxchg and atomic_xchg (where returning the new value makes no sense).
Paolo
Quoted 19 lines · Paolo Bonzini
Il 02/07/2013 13:44, Jan Kiszka ha scritto:
Quoted 18 lines · Paolo Bonzini
Quoted 16 lines
Il 02/07/2013 13:15, Andreas Färber ha scritto:
Quoted 8 lines
Quoted 6 lines
qom/object.c GSList *object_class_get_list(const char *implements_type, · line 686
Quoted 6 lines
qom/object.c +1
-1@@ -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 );How widespread are these in GCC/clang? Is there any fallback? I remember seeing some __sync_* warnings on Mac OS X around 4.2...
We are using them already in several places (vhost was the first one to introduce them, I think, but now they are also in migration ad in some tests too). There is no fallback (asm could be a fallback, but we chose to require GCC 4.2 or newer).
I'll change this to atomic_inc/dec when applying. Otherwise
Quoted 7 lines
Quoted 3 lines
But then
atomic_dec_and_testor so. Letting the inc/dec return some value leaves room for interpretations (value of before or after the modification?).In qemu, I made all atomic_* functions return the old value. This is consistent with
atomic_cmpxchgandatomic_xchg(where returning the new value makes no sense).
Please avoid this ambiguity by naming the functions properly. That xchg returns old values is known, that dec and inc do, is surely not.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux
Il 02/07/2013 13:52, Jan Kiszka ha scritto:
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
But then
atomic_dec_and_testor so. Letting the inc/dec return someQuoted 2 lines
Quoted 2 lines
value leaves room for interpretations (value of before or after the modification?).
In qemu, I made all atomic_* functions return the old value. This is consistent with
atomic_cmpxchgandatomic_xchg(where returning the new value makes no sense).Please avoid this ambiguity by naming the functions properly. That xchg returns old values is known, that dec and inc do, is surely not.
IMO the ambiguity is resolved simply by looking at the docs or existing code, but I can rename them to atomic_fetch_{add,sub,and,or,inc,dec} and add void versions without "fetch".
Paolo
Quoted 23 lines · Jan Kiszka
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 <[email protected]>--- qom/object.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)qom/object.c +1
-1@@ -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)
qom/object.c GSList *object_class_get_list(const char *implements_type, · line 697
Quoted 10 lines
qom/object.c +1
-2{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?
Regards,
Anthony Liguori
Quoted 2 lines
Signature · 1 line
1.7.3.4
Il 02/07/2013 16:47, Anthony Liguori ha scritto:
Quoted 29 lines
Quoted 27 lines · Jan Kiszka
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 <[email protected]>--- qom/object.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)qom/object.c +1
-2@@ -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--;
Quoted 9 lines
qom/object.c GSList *object_class_get_list(const char *implements_type, · line 697
Quoted 6 lines
qom/object.c +1
-1/* 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.
Paolo
Quoted 40 lines · Paolo Bonzini
Il 02/07/2013 16:47, Anthony Liguori ha scritto:
Quoted 29 lines
Quoted 27 lines · Jan Kiszka
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 <[email protected]>--- qom/object.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)qom/object.c +1
-2@@ -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--;Quoted 9 lines
qom/object.c GSList *object_class_get_list(const char *implements_type, · line 697
Quoted 6 lines
qom/object.c +1
-1/* 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?
Quoted 3 lines
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?
Regards,
Anthony Liguori
Quoted 2 lines
Paolo
Quoted 42 lines · Anthony Liguori
Quoted 40 lines · Paolo Bonzini
Il 02/07/2013 16:47, Anthony Liguori ha scritto:
Quoted 38 lines
Quoted 23 lines · Jan Kiszka
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 <[email protected]>--- qom/object.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)qom/object.c +1
-1@@ -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)qom/object.c GSList *object_class_get_list(const char *implements_type, · line 697
Quoted 10 lines
qom/object.c +1
-2{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?
Quoted 7 lines
Quoted 3 lines
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.
hw/core/qdev.c +1 -3
@@ -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));
qom/object.c +1 -0
@@ -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);
Am 03.07.2013 03:23, schrieb liu ping fan:
Quoted 54 lines · Anthony Liguori
Quoted 42 lines
Quoted 40 lines · Paolo Bonzini
Il 02/07/2013 16:47, Anthony Liguori ha scritto:
Quoted 38 lines
Quoted 23 lines · Jan Kiszka
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 <[email protected]>--- qom/object.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)qom/object.c +1
-1@@ -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)qom/object.c GSList *object_class_get_list(const char *implements_type, · line 697
Quoted 10 lines
qom/object.c +1
-2{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?
Quoted 7 lines
Quoted 3 lines
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
Quoted 7 lines
-- >8 --
Subject: [PATCH] qom: delay DeviceState destructor until object finializeUntil refcnt->0, we know that the DeviceState can be safely dropped, so put the destructor there.
Signed-off-by Liu Ping Fan <[email protected]>
It would be nice to get CC'ed on such proposals. :)
Quoted 20 lines
hw/core/qdev.c +1
-3@@ -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));qom/object.c +0
-0
qom/object.c static void object_finalize(void *data) · line 396
Quoted 4 lines
qom/object.c +1
-0@@ -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
Quoted 3 lines
qom/object.c +0
-0object_deinit (obj, ti);object_property_del_all (obj);
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Quoted 58 lines · Andreas Färber
Am 03.07.2013 03:23, schrieb liu ping fan:
Quoted 57 lines · Anthony Liguori
Quoted 42 lines
Quoted 40 lines · Paolo Bonzini
Il 02/07/2013 16:47, Anthony Liguori ha scritto:
Quoted 38 lines
Quoted 23 lines · Jan Kiszka
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 <[email protected]>--- qom/object.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)qom/object.c +1
-1@@ -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)qom/object.c GSList *object_class_get_list(const char *implements_type, · line 697
Quoted 10 lines
qom/object.c +1
-2{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?
Quoted 7 lines
Quoted 3 lines
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
Quoted 7 lines
Quoted 4 lines
Until refcnt->0, we know that the DeviceState can be safely dropped, so put the destructor there.
Signed-off-by Liu Ping Fan <[email protected]>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.
Quoted 23 lines
Quoted 23 lines
hw/core/qdev.c +1
-3@@ -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));qom/object.c +0
-0@@ -393,6 +393,7 @@ static void object_finalize(void *data)Object *obj = data;TypeImpl *ti = obj->class->type;
Quoted 8 lines
qom/object.c static void object_finalize(void *data) · line 396
Quoted 1 line
qom/object.c +1
-0+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()?
Quoted 1 line
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
Quoted 12 lines
Regards,
Andreasqom/object.c static void object_finalize(void *data)
Quoted 3 lines
qom/object.c +0
-0object_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:
Quoted 22 lines · Andreas Färber
Quoted 21 lines
Am 03.07.2013 03:23, schrieb liu ping fan:
Quoted 20 lines · Anthony Liguori
Quoted 9 lines
Quoted 3 lines · Paolo Bonzini
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 finializeUntil refcnt->0, we know that the DeviceState can be safely dropped, so put the destructor there.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Liu Ping Fan <[email protected]>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.
Quoted 28 lines
Quoted 28 lines
Quoted 20 lines
hw/core/qdev.c +1
-3@@ -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));qom/object.c +0
-0qom/object.c static void object_finalize(void *data) · line 396
Quoted 4 lines
qom/object.c +1
-0@@ -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.
Quoted 4 lines
Quoted 3 lines
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, whichDo you mean the sequence in
object_deinit()?
Yes.
Quoted 5 lines
Quoted 1 line
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
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
qom/object.c +0
-0object_deinit (obj, ti);object_property_del_all (obj);
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Quoted 3 lines · Andreas Färber
Am 04.07.2013 06:46, schrieb liu ping fan:
Quoted 2 lines · Andreas Färber
Quoted 1 line
Am 03.07.2013 03:23, schrieb liu ping fan:
[...]
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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 :)
Quoted 44 lines
dedicated Device (formerly qdev) maintainers, Paolo and me have been hacking on it as needed.
Quoted 28 lines
Quoted 28 lines
Quoted 20 lines
hw/core/qdev.c +1
-3@@ -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));qom/object.c +0
-0qom/object.c static void object_finalize(void *data) · line 396
Quoted 4 lines
qom/object.c +1
-0@@ -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.
Quoted 4 lines
Quoted 3 lines
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, whichDo you mean the sequence in
object_deinit()?Yes.
Quoted 5 lines
Quoted 1 line
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?
Quoted 9 lines
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.
Quoted 1 line
But at least today,
TYPE_OBJECTdoes not have aninstance_finalize
Think it will not happen. Since instance_finalize is a hook for derived object, as for Object, object_finalize is the one, right?
Quoted 4 lines
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
Quoted 10 lines
Regards,
AndreasQuoted 3 lines
Quoted 3 lines
qom/object.c static void object_finalize(void *data)
Quoted 3 lines
qom/object.c +0
-0object_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
Il 02/07/2013 18:36, Anthony Liguori ha scritto:
Quoted 42 lines
Quoted 40 lines · Paolo Bonzini
Il 02/07/2013 16:47, Anthony Liguori ha scritto:
Quoted 38 lines
Quoted 23 lines · Jan Kiszka
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 <[email protected]>--- qom/object.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)qom/object.c +1
-1@@ -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)qom/object.c GSList *object_class_get_list(const char *implements_type, · line 697
Quoted 10 lines
qom/object.c +1
-2{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?
Quoted 6 lines
Quoted 3 lines
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?
We should make it more robust against objects that are not in the QOM composition tree (adding/removing the "child" property is relatively slow). As things stand, QOM is definitely too slow for something like SCSIRequest.
In the long term, it is definitely nice to use Object more. But if we really had to abstract things, for now I'd just do
#define atomic_ref(x) atomic_inc(x)
#define atomic_unref_test_zero(x) (atomic_fetch_dec(x) == 1)
or something like that.
Paolo