| Message ID | 1345604562-27289-1-git-send-email-qemulist@gmail.com |
|---|---|
| State | New |
| Headers | show |
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); > } >
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); >> } >>
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); > }
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
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
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
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
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); >>> } >>>
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
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); }