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

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

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@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 <pingfank@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

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

[Paolo Bonzini]

Il 22/08/2012 05:02, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@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 <pingfank@linux.vnet.ibm.com>

Acked-by: Paolo Bonzini <pbonzini@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

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 22/08/2012 05:02, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <pingfank@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 <pingfank@linux.vnet.ibm.com>
>
> Acked-by: Paolo Bonzini <pbonzini@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

[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

[Anthony Liguori]

Hi Paolo,

Paolo Bonzini <pbonzini@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

[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

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

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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.

info qtree doesn't print out the composition tree, it prints the sysbus
tree.  Once the parent bus is set to NULL, it won't show on info qtree
anymore.

> 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 need to think more about this I guess..

What I'm after is an interface that consists of:

1) orphan device <- i don't care about this device anymore, as soon as
you can, get rid of it

2) request unplug <- ask the guest to remove the device

3) guest eject <- ejects a device from the guest

device_del would consist of "orphan device" followed by "request
unplug".

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.

Regards,

Anthony Liguori

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

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

[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

[liu ping fan]

On Thu, Aug 23, 2012 at 12:36 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Il 22/08/2012 05:02, Liu Ping Fan ha scritto:
>>> From: Liu Ping Fan <pingfank@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 <pingfank@linux.vnet.ibm.com>
>>
>> Acked-by: Paolo Bonzini <pbonzini@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

[Andreas Färber]

Am 22.08.2012 05:02, schrieb Liu Ping Fan:
> From: Liu Ping Fan <pingfank@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 <pingfank@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