| Message ID | 1361559693-39396-2-git-send-email-jfrei@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 22.02.2013 20:01, schrieb Jens Freimann: > From: Christian Borntraeger <borntraeger@de.ibm.com> > > This patch fixes a crash when unplugging a virtio-ccw device. We no > longer need to do that in virtio-ccw since common code does now > proper handling. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com> > --- > hw/s390x/virtio-ccw.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index 231f81e..679afa6 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -865,7 +865,6 @@ static int virtio_ccw_busdev_unplug(DeviceState *dev) > > css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, 1, 0); > > - object_unparent(OBJECT(dev)); > qdev_free(dev); > return 0; > } qdev_free() does exactly object_unparent(), so in light of wanting to get away from qdev I would suggest to rather drop qdev_free() here. Paolo? Andreas
On 24/02/13 12:30, Andreas Färber wrote: > Am 22.02.2013 20:01, schrieb Jens Freimann: >> From: Christian Borntraeger <borntraeger@de.ibm.com> >> >> This patch fixes a crash when unplugging a virtio-ccw device. We no >> longer need to do that in virtio-ccw since common code does now >> proper handling. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com> >> --- >> hw/s390x/virtio-ccw.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c >> index 231f81e..679afa6 100644 >> --- a/hw/s390x/virtio-ccw.c >> +++ b/hw/s390x/virtio-ccw.c >> @@ -865,7 +865,6 @@ static int virtio_ccw_busdev_unplug(DeviceState *dev) >> >> css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, 1, 0); >> >> - object_unparent(OBJECT(dev)); >> qdev_free(dev); >> return 0; >> } > > qdev_free() does exactly object_unparent(), so in light of wanting to > get away from qdev I would suggest to rather drop qdev_free() here. Paolo? Agreed, this is identical from a functional perspective and I was asking that myself, but it seems that qdev_free is an interface. used by many busses: $ git grep qdev_free hw/acpi_piix4.c: qdev_free(qdev); hw/pci/pci-hotplug.c: qdev_free(&dev->qdev); hw/pci/pci_bridge.c: /* qbus_free() is called automatically by qdev_free() */ hw/pci/pcie.c: qdev_free(&pci_dev->qdev); hw/pci/shpc.c: qdev_free(&affected_dev->qdev); hw/qdev-core.h:void qdev_free(DeviceState *dev); hw/qdev-monitor.c: qdev_free(qdev); hw/qdev.c: qdev_free(dev); hw/qdev.c: qdev_free(dev); hw/qdev.c:void qdev_free(DeviceState *dev) hw/qdev.c: qdev_free(dev); hw/s390x/virtio-ccw.c: qdev_free(dev); hw/scsi-bus.c: qdev_free(&d->qdev); hw/scsi-bus.c: qdev_free(dev); hw/usb/bus.c: qdev_free(&port->dev->qdev); hw/usb/bus.c: qdev_free(&dev->qdev); hw/usb/dev-storage.c: qdev_free(&dev->qdev); hw/usb/host-linux.c: qdev_free(&dev->qdev); hw/virtio-bus.c: qdev_free(qdev); hw/xen_platform.c: qdev_free(&d->qdev); ...while object_unparent is still somewhat internal. $ git grep object_unparent hw/qdev.c: object_unparent(OBJECT(dev)); hw/qdev.c: object_unparent(OBJECT(bus)); include/qom/object.h:void object_unparent(Object *obj); qom/object.c:void object_unparent(Object *obj) Another thing is, that qdev_free looks now different, some days ago it also did an unref. As far as I can see the object_unparent in virtio-ccw was always the wrong thing to do. So for a potential backport this version of the patch might be better. Paolo, do you have any guidance? Christian
> Another thing is, that qdev_free looks now different, some days ago > it also did an unref. As far as I can see the object_unparent in > virtio-ccw was always the wrong thing to do. object_unparent is "almost" idempotent, i.e. idempotent as long as it does not cause the last reference to go away. So, doing an object_unparent before qdev_free was not wrong when qdev_free did an object_unref. I think qdev_free is better, unless we want to change all of them at the same time. Paolo > So for a potential backport > this version of the patch might be better. > Paolo, do you have any guidance? > > Christian > >
On 25/02/13 08:55, Paolo Bonzini wrote: > >> Another thing is, that qdev_free looks now different, some days ago >> it also did an unref. As far as I can see the object_unparent in >> virtio-ccw was always the wrong thing to do. > > object_unparent is "almost" idempotent, i.e. idempotent as long as it does > not cause the last reference to go away. So, doing an object_unparent > before qdev_free was not wrong when qdev_free did an object_unref. Hmm, the old sequence was object_unparent(OBJECT(dev)); qdev_free(dev) ---+ | V ... object_unparent(OBJECT(dev)); now the last reference is gone, object is freed object_unref(OBJECT(dev)); now the reference of a deleted object becomes -1 ... Isnt that a problem in itself that we modify a reference counter in an deleted object? > > I think qdev_free is better, unless we want to change all of them > at the same time. Ok, so we leave the patch as is. Christian
Am 25.02.2013 08:55, schrieb Paolo Bonzini: > >> Another thing is, that qdev_free looks now different, some days ago >> it also did an unref. As far as I can see the object_unparent in >> virtio-ccw was always the wrong thing to do. > > object_unparent is "almost" idempotent, i.e. idempotent as long as it does > not cause the last reference to go away. So, doing an object_unparent > before qdev_free was not wrong when qdev_free did an object_unref. > > I think qdev_free is better, unless we want to change all of them > at the same time. I did have a patch doing that but revoked it because I thought you said you wanted to do it differently as part of your series... If it stays it needs to be renamed device_* and probably not *_free either since that depends on reference count. Andreas
Il 25/02/2013 09:09, Christian Borntraeger ha scritto: > Hmm, the old sequence was > > object_unparent(OBJECT(dev)); > qdev_free(dev) ---+ > | > V > ... > object_unparent(OBJECT(dev)); now the last reference is gone, object is freed > object_unref(OBJECT(dev)); now the reference of a deleted object becomes -1 > ... > > Isnt that a problem in itself that we modify a reference counter in an deleted object? The second object_unparent should do nothing. So before you had: object_unparent(OBJECT(dev)); leaves refcount=1 qdev_free(dev) ---+ | V object_unparent(OBJECT(dev)); do nothing object_unref(OBJECT(dev)); refcount=0, object freed After the object_unref was removed you had: object_unparent(OBJECT(dev)); refcount=0, object freed qdev_free(dev) ---+ | V object_unparent(OBJECT(dev)); dangling pointer! Paolo
On 25/02/13 11:44, Paolo Bonzini wrote: > Il 25/02/2013 09:09, Christian Borntraeger ha scritto: >> Hmm, the old sequence was >> >> object_unparent(OBJECT(dev)); >> qdev_free(dev) ---+ >> | >> V >> ... >> object_unparent(OBJECT(dev)); now the last reference is gone, object is freed >> object_unref(OBJECT(dev)); now the reference of a deleted object becomes -1 >> ... >> >> Isnt that a problem in itself that we modify a reference counter in an deleted object? > > The second object_unparent should do nothing. So before you had: > > object_unparent(OBJECT(dev)); leaves refcount=1 > qdev_free(dev) ---+ > | > V > object_unparent(OBJECT(dev)); do nothing > object_unref(OBJECT(dev)); refcount=0, object freed > > After the object_unref was removed you had: > > object_unparent(OBJECT(dev)); refcount=0, object freed > qdev_free(dev) ---+ > | > V > object_unparent(OBJECT(dev)); dangling pointer! > Got it. Thanks
On 25.02.2013, at 12:10, Christian Borntraeger wrote: > On 25/02/13 11:44, Paolo Bonzini wrote: >> Il 25/02/2013 09:09, Christian Borntraeger ha scritto: >>> Hmm, the old sequence was >>> >>> object_unparent(OBJECT(dev)); >>> qdev_free(dev) ---+ >>> | >>> V >>> ... >>> object_unparent(OBJECT(dev)); now the last reference is gone, object is freed >>> object_unref(OBJECT(dev)); now the reference of a deleted object becomes -1 >>> ... >>> >>> Isnt that a problem in itself that we modify a reference counter in an deleted object? >> >> The second object_unparent should do nothing. So before you had: >> >> object_unparent(OBJECT(dev)); leaves refcount=1 >> qdev_free(dev) ---+ >> | >> V >> object_unparent(OBJECT(dev)); do nothing >> object_unref(OBJECT(dev)); refcount=0, object freed >> >> After the object_unref was removed you had: >> >> object_unparent(OBJECT(dev)); refcount=0, object freed >> qdev_free(dev) ---+ >> | >> V >> object_unparent(OBJECT(dev)); dangling pointer! >> > > > Got it. Thanks So is the patch valid? Alex
On Fri, 8 Mar 2013 21:11:13 +0100 Alexander Graf <agraf@suse.de> wrote: > > On 25.02.2013, at 12:10, Christian Borntraeger wrote: > > > On 25/02/13 11:44, Paolo Bonzini wrote: > >> Il 25/02/2013 09:09, Christian Borntraeger ha scritto: > >>> Hmm, the old sequence was > >>> > >>> object_unparent(OBJECT(dev)); > >>> qdev_free(dev) ---+ > >>> | > >>> V > >>> ... > >>> object_unparent(OBJECT(dev)); now the last reference is gone, object is freed > >>> object_unref(OBJECT(dev)); now the reference of a deleted object becomes -1 > >>> ... > >>> > >>> Isnt that a problem in itself that we modify a reference counter in an deleted object? > >> > >> The second object_unparent should do nothing. So before you had: > >> > >> object_unparent(OBJECT(dev)); leaves refcount=1 > >> qdev_free(dev) ---+ > >> | > >> V > >> object_unparent(OBJECT(dev)); do nothing > >> object_unref(OBJECT(dev)); refcount=0, object freed > >> > >> After the object_unref was removed you had: > >> > >> object_unparent(OBJECT(dev)); refcount=0, object freed > >> qdev_free(dev) ---+ > >> | > >> V > >> object_unparent(OBJECT(dev)); dangling pointer! > >> > > > > > > Got it. Thanks > > So is the patch valid? To my understanding, yes. > > > Alex >
Il 11/03/2013 13:04, Cornelia Huck ha scritto: > On Fri, 8 Mar 2013 21:11:13 +0100 > Alexander Graf <agraf@suse.de> wrote: > >> >> On 25.02.2013, at 12:10, Christian Borntraeger wrote: >> >>> On 25/02/13 11:44, Paolo Bonzini wrote: >>>> Il 25/02/2013 09:09, Christian Borntraeger ha scritto: >>>>> Hmm, the old sequence was >>>>> >>>>> object_unparent(OBJECT(dev)); >>>>> qdev_free(dev) ---+ >>>>> | >>>>> V >>>>> ... >>>>> object_unparent(OBJECT(dev)); now the last reference is gone, object is freed >>>>> object_unref(OBJECT(dev)); now the reference of a deleted object becomes -1 >>>>> ... >>>>> >>>>> Isnt that a problem in itself that we modify a reference counter in an deleted object? >>>> >>>> The second object_unparent should do nothing. So before you had: >>>> >>>> object_unparent(OBJECT(dev)); leaves refcount=1 >>>> qdev_free(dev) ---+ >>>> | >>>> V >>>> object_unparent(OBJECT(dev)); do nothing >>>> object_unref(OBJECT(dev)); refcount=0, object freed >>>> >>>> After the object_unref was removed you had: >>>> >>>> object_unparent(OBJECT(dev)); refcount=0, object freed >>>> qdev_free(dev) ---+ >>>> | >>>> V >>>> object_unparent(OBJECT(dev)); dangling pointer! >>>> >>> >>> >>> Got it. Thanks >> >> So is the patch valid? > > To my understanding, yes. Yes, except that the "fixed a crash" part in the commit message is probably no longer accurate. No big deal. :) Paolo
On 03/11/2013 01:16 PM, Paolo Bonzini wrote: > Il 11/03/2013 13:04, Cornelia Huck ha scritto: >> On Fri, 8 Mar 2013 21:11:13 +0100 >> Alexander Graf<agraf@suse.de> wrote: >> >>> On 25.02.2013, at 12:10, Christian Borntraeger wrote: >>> >>>> On 25/02/13 11:44, Paolo Bonzini wrote: >>>>> Il 25/02/2013 09:09, Christian Borntraeger ha scritto: >>>>>> Hmm, the old sequence was >>>>>> >>>>>> object_unparent(OBJECT(dev)); >>>>>> qdev_free(dev) ---+ >>>>>> | >>>>>> V >>>>>> ... >>>>>> object_unparent(OBJECT(dev)); now the last reference is gone, object is freed >>>>>> object_unref(OBJECT(dev)); now the reference of a deleted object becomes -1 >>>>>> ... >>>>>> >>>>>> Isnt that a problem in itself that we modify a reference counter in an deleted object? >>>>> The second object_unparent should do nothing. So before you had: >>>>> >>>>> object_unparent(OBJECT(dev)); leaves refcount=1 >>>>> qdev_free(dev) ---+ >>>>> | >>>>> V >>>>> object_unparent(OBJECT(dev)); do nothing >>>>> object_unref(OBJECT(dev)); refcount=0, object freed >>>>> >>>>> After the object_unref was removed you had: >>>>> >>>>> object_unparent(OBJECT(dev)); refcount=0, object freed >>>>> qdev_free(dev) ---+ >>>>> | >>>>> V >>>>> object_unparent(OBJECT(dev)); dangling pointer! >>>>> >>>> >>>> Got it. Thanks >>> So is the patch valid? >> To my understanding, yes. > Yes, except that the "fixed a crash" part in the commit message is > probably no longer accurate. No big deal. :) Ok, Connie could you please include it in your next pull then please? Alex
On Mon, 11 Mar 2013 13:22:45 +0100 Alexander Graf <agraf@suse.de> wrote: > On 03/11/2013 01:16 PM, Paolo Bonzini wrote: > > Il 11/03/2013 13:04, Cornelia Huck ha scritto: > >> On Fri, 8 Mar 2013 21:11:13 +0100 > >> Alexander Graf<agraf@suse.de> wrote: > >> > >>> On 25.02.2013, at 12:10, Christian Borntraeger wrote: > >>> > >>>> On 25/02/13 11:44, Paolo Bonzini wrote: > >>>>> Il 25/02/2013 09:09, Christian Borntraeger ha scritto: > >>>>>> Hmm, the old sequence was > >>>>>> > >>>>>> object_unparent(OBJECT(dev)); > >>>>>> qdev_free(dev) ---+ > >>>>>> | > >>>>>> V > >>>>>> ... > >>>>>> object_unparent(OBJECT(dev)); now the last reference is gone, object is freed > >>>>>> object_unref(OBJECT(dev)); now the reference of a deleted object becomes -1 > >>>>>> ... > >>>>>> > >>>>>> Isnt that a problem in itself that we modify a reference counter in an deleted object? > >>>>> The second object_unparent should do nothing. So before you had: > >>>>> > >>>>> object_unparent(OBJECT(dev)); leaves refcount=1 > >>>>> qdev_free(dev) ---+ > >>>>> | > >>>>> V > >>>>> object_unparent(OBJECT(dev)); do nothing > >>>>> object_unref(OBJECT(dev)); refcount=0, object freed > >>>>> > >>>>> After the object_unref was removed you had: > >>>>> > >>>>> object_unparent(OBJECT(dev)); refcount=0, object freed > >>>>> qdev_free(dev) ---+ > >>>>> | > >>>>> V > >>>>> object_unparent(OBJECT(dev)); dangling pointer! > >>>>> > >>>> > >>>> Got it. Thanks > >>> So is the patch valid? > >> To my understanding, yes. > > Yes, except that the "fixed a crash" part in the commit message is > > probably no longer accurate. No big deal. :) > > Ok, Connie could you please include it in your next pull then please? Sure, will do. > > > Alex >
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 231f81e..679afa6 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -865,7 +865,6 @@ static int virtio_ccw_busdev_unplug(DeviceState *dev) css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, 1, 0); - object_unparent(OBJECT(dev)); qdev_free(dev); return 0; }