Re: [Qemu-devel] [PATCH] qom: removal of link property need to release its target

2012-08-23 Thread liu ping fan 
On Thu, Aug 23, 2012 at 12:36 AM, Anthony Liguori anth...@codemonkey.ws wrote:
 Paolo Bonzini pbonz...@redhat.com writes:

 Il 22/08/2012 05:02, Liu Ping Fan ha scritto:
 From: Liu Ping Fan pingf...@linux.vnet.ibm.com

 Currently, link property's target is only managed by
 object_set_link_property(). This will raise such issue that when
 the property is finalized, its target has no opportunity to release.

 Fix this issue by introduce object_finalize_link_property()

 Signed-off-by: Liu Ping Fan pingf...@linux.vnet.ibm.com

 Acked-by: Paolo Bonzini pbonz...@redhat.com

 This patch is correct but it uncovers a bigger problem.  Short of
 reworking most of the hotplug code, I can't find an easy fix.

 The first problem is that with this patch, all links are unreferenced at
 property removal.  Right now, bus children are added as links but when
 they are added, the link is already set (there is no explicit set).
 This means that those links never get the extra reference.

 We can fix this by adding an extra reference in add_link but this

Yeah, I was wondering about why it does not like this

 creates yet another problem with hotplug.  Specificially, qdev_free()
 asserts that ref  0 because there is now a reference being held by the
 bus.

 This is the same problem we have with object_unparent.

 The key problem here is how unplug is implemented.  Unplug wants to be
 both synchronous and asynchronous.

 I think we need to do the following:

 1) Move object_unparent to qdev_device_del (the parent is added by
qdev_device_add so this is quite logical).

 2) Make DeviceState::unplug *never* call qdev_free().

 3) Add an unplugged NotifierList to DeviceState.

 4) Change the various hotplug consumers to call qdev_set_parent_bus() to
NULL to unplug the device from the bus.  Change qdev_set_parent_bus()
to allow this and remove the bus link and invoke the unplugged notifier.

 5) Change qdev_device_del() to add a notifier to the object that calls
object_unparent() and object_unref.

 6) Rename DeviceState::unplug to DeviceState::request_unplug

I like this good name!

 7) Take Ping Fan's patch + another patch to add a reference count in
object_property_add_link

In these days, I had thought about the way to do the hot unplug like
the following:
Suppose we start from devA
qdev_tree_delete(devA)
{
  qdev_walk_children(devA,  qdev_device_del,  qdev_bus_del, NULL)
  qdev_device_del(devA)
}

For qdev_device_del() do the following things:
--. delete link from parent bus
--. detached from parent bus-children
--. dev-parent_bus = NULL
--. object_unref(dev);

For qdev_bus_del(),
-- delete child property of parent
-- detach from parent-child_bus
-- bus-parent = NULL
-- object_unref(bus)

So leave anything handled by object_unref(), no call to qdev_free and so on

Thanks and regards,
pingfan
 Regards,

 Anthony Liguori


 Paolo

 ---
  qom/object.c |   12 +++-
  1 files changed, 11 insertions(+), 1 deletions(-)

 diff --git a/qom/object.c b/qom/object.c
 index a552be2..76b3d34 100644
 --- a/qom/object.c
 +++ b/qom/object.c
 @@ -957,6 +957,16 @@ static void object_set_link_property(Object *obj, 
 Visitor *v, void *opaque,
  }
  }

 +static void object_finalize_link_property(Object *obj, const char *name,
 +   void *opaque)
 +{
 +Object **child = opaque;
 +
 +if (*child != NULL) {
 +object_unref(*child);
 +}
 +}
 +
  void object_property_add_link(Object *obj, const char *name,
const char *type, Object **child,
Error **errp)
 @@ -968,7 +978,7 @@ void object_property_add_link(Object *obj, const char 
 *name,
  object_property_add(obj, name, full_type,
  object_get_link_property,
  object_set_link_property,
 -NULL, child, errp);
 +object_finalize_link_property, child, errp);

  g_free(full_type);
  }

Re: [Qemu-devel] [PATCH] qom: removal of link property need to release its target

2012-08-23 Thread Paolo Bonzini 
Il 23/08/2012 00:40, Anthony Liguori ha scritto:
 I don't really like the notion of a forced eject where we delete a
 device when the guest is using it and not cooperative.  I don't see the
 benefit at all.
 
 Forcing detachment of a BlockDriverState from a device followed by EIO
 being reported to the guest for all I/O ops makes sense to me.  But not
 forced removal of virtio-blk-pci.

PCI express even has support for detecting surprise removal early (with
card presence detection pins, that break contact before others) and
remove power before damaging the hardware.  It's very much a real thing.

You could surprise-remove an assigned card from under the feet of a
non-cooperating guest, for example.

Paolo

Re: [Qemu-devel] [PATCH] qom: removal of link property need to release its target

2012-08-22 Thread Paolo Bonzini 
Il 22/08/2012 05:02, Liu Ping Fan ha scritto:
 From: Liu Ping Fan pingf...@linux.vnet.ibm.com
 
 Currently, link property's target is only managed by
 object_set_link_property(). This will raise such issue that when
 the property is finalized, its target has no opportunity to release.
 
 Fix this issue by introduce object_finalize_link_property()
 
 Signed-off-by: Liu Ping Fan pingf...@linux.vnet.ibm.com

Acked-by: Paolo Bonzini pbonz...@redhat.com

Paolo

 ---
  qom/object.c |   12 +++-
  1 files changed, 11 insertions(+), 1 deletions(-)
 
 diff --git a/qom/object.c b/qom/object.c
 index a552be2..76b3d34 100644
 --- a/qom/object.c
 +++ b/qom/object.c
 @@ -957,6 +957,16 @@ static void object_set_link_property(Object *obj, 
 Visitor *v, void *opaque,
  }
  }
  
 +static void object_finalize_link_property(Object *obj, const char *name,
 +   void *opaque)
 +{
 +Object **child = opaque;
 +
 +if (*child != NULL) {
 +object_unref(*child);
 +}
 +}
 +
  void object_property_add_link(Object *obj, const char *name,
const char *type, Object **child,
Error **errp)
 @@ -968,7 +978,7 @@ void object_property_add_link(Object *obj, const char 
 *name,
  object_property_add(obj, name, full_type,
  object_get_link_property,
  object_set_link_property,
 -NULL, child, errp);
 +object_finalize_link_property, child, errp);
  
  g_free(full_type);
  }

Re: [Qemu-devel] [PATCH] qom: removal of link property need to release its target

2012-08-22 Thread Anthony Liguori 
Paolo Bonzini pbonz...@redhat.com writes:

 Il 22/08/2012 05:02, Liu Ping Fan ha scritto:
 From: Liu Ping Fan pingf...@linux.vnet.ibm.com
 
 Currently, link property's target is only managed by
 object_set_link_property(). This will raise such issue that when
 the property is finalized, its target has no opportunity to release.
 
 Fix this issue by introduce object_finalize_link_property()
 
 Signed-off-by: Liu Ping Fan pingf...@linux.vnet.ibm.com

 Acked-by: Paolo Bonzini pbonz...@redhat.com

This patch is correct but it uncovers a bigger problem.  Short of
reworking most of the hotplug code, I can't find an easy fix.

The first problem is that with this patch, all links are unreferenced at
property removal.  Right now, bus children are added as links but when
they are added, the link is already set (there is no explicit set).
This means that those links never get the extra reference.

We can fix this by adding an extra reference in add_link but this
creates yet another problem with hotplug.  Specificially, qdev_free()
asserts that ref  0 because there is now a reference being held by the
bus.

This is the same problem we have with object_unparent.

The key problem here is how unplug is implemented.  Unplug wants to be
both synchronous and asynchronous.

I think we need to do the following:

1) Move object_unparent to qdev_device_del (the parent is added by
   qdev_device_add so this is quite logical).

2) Make DeviceState::unplug *never* call qdev_free().

3) Add an unplugged NotifierList to DeviceState.

4) Change the various hotplug consumers to call qdev_set_parent_bus() to
   NULL to unplug the device from the bus.  Change qdev_set_parent_bus()
   to allow this and remove the bus link and invoke the unplugged notifier.

5) Change qdev_device_del() to add a notifier to the object that calls
   object_unparent() and object_unref.

6) Rename DeviceState::unplug to DeviceState::request_unplug

7) Take Ping Fan's patch + another patch to add a reference count in
   object_property_add_link

Regards,

Anthony Liguori


 Paolo

 ---
  qom/object.c |   12 +++-
  1 files changed, 11 insertions(+), 1 deletions(-)
 
 diff --git a/qom/object.c b/qom/object.c
 index a552be2..76b3d34 100644
 --- a/qom/object.c
 +++ b/qom/object.c
 @@ -957,6 +957,16 @@ static void object_set_link_property(Object *obj, 
 Visitor *v, void *opaque,
  }
  }
  
 +static void object_finalize_link_property(Object *obj, const char *name,
 +   void *opaque)
 +{
 +Object **child = opaque;
 +
 +if (*child != NULL) {
 +object_unref(*child);
 +}
 +}
 +
  void object_property_add_link(Object *obj, const char *name,
const char *type, Object **child,
Error **errp)
 @@ -968,7 +978,7 @@ void object_property_add_link(Object *obj, const char 
 *name,
  object_property_add(obj, name, full_type,
  object_get_link_property,
  object_set_link_property,
 -NULL, child, errp);
 +object_finalize_link_property, child, errp);
  
  g_free(full_type);
  }

Re: [Qemu-devel] [PATCH] qom: removal of link property need to release its target

2012-08-22 Thread Andreas Färber 
Am 22.08.2012 05:02, schrieb Liu Ping Fan:
 From: Liu Ping Fan pingf...@linux.vnet.ibm.com
 
 Currently, link property's target is only managed by
 object_set_link_property(). This will raise such issue that when
 the property is finalized, its target has no opportunity to release.
 
 Fix this issue by introduce object_finalize_link_property()
 
 Signed-off-by: Liu Ping Fan pingf...@linux.vnet.ibm.com
 ---
  qom/object.c |   12 +++-
  1 files changed, 11 insertions(+), 1 deletions(-)
 
 diff --git a/qom/object.c b/qom/object.c
 index a552be2..76b3d34 100644
 --- a/qom/object.c
 +++ b/qom/object.c
 @@ -957,6 +957,16 @@ static void object_set_link_property(Object *obj, 
 Visitor *v, void *opaque,
  }
  }
  
 +static void object_finalize_link_property(Object *obj, const char *name,
 +   void *opaque)
 +{
 +Object **child = opaque;
 +
 +if (*child != NULL) {
 +object_unref(*child);
 +}
 +}

The naming is confusing: In ObjectProperty this hook is called release
and you're not finalizing the value either, just unref'ing, which may or
may not lead to the finalizer being called.

I'd thus propose object_release_link_property for whatever gets
decided its implementation should do.

Regards,
Andreas

 +
  void object_property_add_link(Object *obj, const char *name,
const char *type, Object **child,
Error **errp)
 @@ -968,7 +978,7 @@ void object_property_add_link(Object *obj, const char 
 *name,
  object_property_add(obj, name, full_type,
  object_get_link_property,
  object_set_link_property,
 -NULL, child, errp);
 +object_finalize_link_property, child, errp);
  
  g_free(full_type);
  }

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] qom: removal of link property need to release its target

2012-08-22 Thread Paolo Bonzini 
Il 22/08/2012 18:36, Anthony Liguori ha scritto:
 We can fix this by adding an extra reference in add_link but this
 creates yet another problem with hotplug.  Specificially, qdev_free()
 asserts that ref  0 because there is now a reference being held by the
 bus.

I think that's correct.  Unplugging needs to remove the circular
reference before object_delete is called.

 This is the same problem we have with object_unparent.

No, it's not.  Parent-to-child is a one-way relationship, it cannot have
circular references.  It is also a cosmetic relationship, in that the
void * is usually not accessed except when using QOM paths.  If child
properties are known to the parent (they are not, for example, when the
parent is a TYPE_CONTAINER), the parent knows how to reach the child
using a C expression.  So unparenting at object_delete time (not before)
makes sense.

 The key problem here is how unplug is implemented.  Unplug wants to be
 both synchronous and asynchronous.
 
 I think we need to do the following:
 
 1) Move object_unparent to qdev_device_del (the parent is added by
qdev_device_add so this is quite logical).

There is no qdev_device_del, but there is qdev_unplug.  We could rename
qdev_unplug to qdev_request_unplug, and qdev_simple_unplug_cb to
qdev_unplug.

 2) Make DeviceState::unplug *never* call qdev_free().

Right.  It should always go through qdev_simple_unplug_cb.

 3) Add an unplugged NotifierList to DeviceState.

Is this really needed?

 4) Change the various hotplug consumers to call qdev_set_parent_bus() to
NULL to unplug the device from the bus.  Change qdev_set_parent_bus()
to allow this and remove the bus link and invoke the unplugged notifier.

This too, is it needed?... qdev_simple_unplug_cb could simply be the
place where you call qdev_set_parent_bus(qdev, NULL), before qdev_free.
 That would break the circular link and keep object_delete() happy.

 5) Change qdev_device_del() to add a notifier to the object that calls
object_unparent() and object_unref.

No need.

 6) Rename DeviceState::unplug to DeviceState::request_unplug

Cosmetic, but I agree. :)

 7) Take Ping Fan's patch + another patch to add a reference count in
object_property_add_link

Yes.

BTW, the patch to fix usb_del is on its way.

Paolo

Re: [Qemu-devel] [PATCH] qom: removal of link property need to release its target

2012-08-22 Thread Anthony Liguori 
Hi Paolo,

Paolo Bonzini pbonz...@redhat.com writes:

 Il 22/08/2012 18:36, Anthony Liguori ha scritto:
 We can fix this by adding an extra reference in add_link but this
 creates yet another problem with hotplug.  Specificially, qdev_free()
 asserts that ref  0 because there is now a reference being held by the
 bus.

 I think that's correct.  Unplugging needs to remove the circular
 reference before object_delete is called.

 This is the same problem we have with object_unparent.

 No, it's not.  Parent-to-child is a one-way relationship, it cannot have
 circular references. 

 It is also a cosmetic relationship, in that the
 void * is usually not accessed except when using QOM paths.  If child
 properties are known to the parent (they are not, for example, when the
 parent is a TYPE_CONTAINER), the parent knows how to reach the child
 using a C expression.  So unparenting at object_delete time (not before)
 makes sense.

Let's ignore parents in this discussion.  I didn't mean to open up this debate.

 The key problem here is how unplug is implemented.  Unplug wants to be
 both synchronous and asynchronous.
 
 I think we need to do the following:
 
 1) Move object_unparent to qdev_device_del (the parent is added by
qdev_device_add so this is quite logical).

 There is no qdev_device_del,

I meant qmp_device_del and qmp_device_add.  Sorry, typo on my parent.

 but there is qdev_unplug.  We could rename
 qdev_unplug to qdev_request_unplug, and qdev_simple_unplug_cb to
 qdev_unplug.

I'm trying to separate the following things:

1) Requesting a device to be unplugged

2) Detaching a device from the bus

3) Deleting the device

(1) happens based on issuing the qmp_device_del monitor command.  (2)
is typically only done based on a guest action but sometimes as a direct
result of (1).

(3) Should happen when all pending references are dropped.  Normally,
the bus holds a reference to a device, the container holds a reference,
and an additional reference floats and is obstensively owned by the
monitor.

(1) should drop the floating reference and the reference held by the
container.  That's what I meant by calling object_unparent in
qmp_device_del.

(2) should simply remove the device from the bus (further releasing a
reference).

(3) would happen automatically from (1) and (2) if they were called in
that order.

If the guest instantiates a remove on it's own, the device would be
disconnected from the bus (functionally unplugged) but still in the
container so it would *not* go away.

I think this is desirable behavior.

 2) Make DeviceState::unplug *never* call qdev_free().

 Right.  It should always go through qdev_simple_unplug_cb.

qdev_simple_unplug_cb() should just qdev_set_parent_bus(NULL).  Or just
drop that function entirely and call the above.  That will drop a
reference and *may* free the object.

(Yes, I'm aware that object_unref doesn't free--that needs to be fixed too).

 3) Add an unplugged NotifierList to DeviceState.

 Is this really needed?

Probably can just be a QMP event.  I'd really like to find a bridge
between notifier lists and qmp events so that the same event can be
consumed by through the management API and programmatically though...

 4) Change the various hotplug consumers to call qdev_set_parent_bus() to
NULL to unplug the device from the bus.  Change qdev_set_parent_bus()
to allow this and remove the bus link and invoke the unplugged notifier.

 This too, is it needed?... qdev_simple_unplug_cb could simply be the
 place where you call qdev_set_parent_bus(qdev, NULL), before qdev_free.
  That would break the circular link and keep object_delete() happy.

Yeah, whether it's done via a wrapper like qdev_simple_unplug_cb()
doesn't matter that much.

 5) Change qdev_device_del() to add a notifier to the object that calls
object_unparent() and object_unref.

 No need.

 6) Rename DeviceState::unplug to DeviceState::request_unplug

 Cosmetic, but I agree. :)

 7) Take Ping Fan's patch + another patch to add a reference count in
object_property_add_link

 Yes.

 BTW, the patch to fix usb_del is on its way.

Cool, thanks!

Regards,

Anthony Liguori


 Paolo

Re: [Qemu-devel] [PATCH] qom: removal of link property need to release its target

2012-08-22 Thread Paolo Bonzini 
Il 22/08/2012 23:41, Anthony Liguori ha scritto:
 
 (1) should drop the floating reference and the reference held by the
 container.  That's what I meant by calling object_unparent in
 qmp_device_del.
 
 (2) should simply remove the device from the bus (further releasing a
 reference).
 
 (3) would happen automatically from (1) and (2) if they were called in
 that order.
 
 If the guest instantiates a remove on it's own, the device would be
 disconnected from the bus (functionally unplugged) but still in the
 container so it would *not* go away.
 
 I think this is desirable behavior.

It may be (I'm not sure it is desirable for HMP), but it's also
backwards-incompatible.  Right now, an unrequested guest-initiated
remove causes the device to disappear in info qtree too.  So, for
backwards-compatibility we need to keep using object_delete after
setting the parent bus to NULL.

WRT adding the unparent *also* in qmp_device_del, that prevents you from
later doing a surprise removal via the monitor, because you don't have
anymore a way to refer the device.  I'm also worried of what happens if
an object loses its canonical path in the middle of its life...

I'm not sure object_unparent should be extern, even.

Paolo

[Qemu-devel] [PATCH] qom: removal of link property need to release its target

2012-08-21 Thread Liu Ping Fan 
From: Liu Ping Fan pingf...@linux.vnet.ibm.com

Currently, link property's target is only managed by
object_set_link_property(). This will raise such issue that when
the property is finalized, its target has no opportunity to release.

Fix this issue by introduce object_finalize_link_property()

Signed-off-by: Liu Ping Fan pingf...@linux.vnet.ibm.com
---
 qom/object.c |   12 +++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index a552be2..76b3d34 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -957,6 +957,16 @@ static void object_set_link_property(Object *obj, Visitor 
*v, void *opaque,
 }
 }
 
+static void object_finalize_link_property(Object *obj, const char *name,
+   void *opaque)
+{
+Object **child = opaque;
+
+if (*child != NULL) {
+object_unref(*child);
+}
+}
+
 void object_property_add_link(Object *obj, const char *name,
   const char *type, Object **child,
   Error **errp)
@@ -968,7 +978,7 @@ void object_property_add_link(Object *obj, const char *name,
 object_property_add(obj, name, full_type,
 object_get_link_property,
 object_set_link_property,
-NULL, child, errp);
+object_finalize_link_property, child, errp);
 
 g_free(full_type);
 }
-- 
1.7.4.4