Patchwork [1/3] virtio-ccw: remove qdev_unparent in unplug routing

login
register
mail settings
Submitter Jens Freimann
Date Feb. 22, 2013, 7:01 p.m.
Message ID <1361559693-39396-2-git-send-email-jfrei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/222614/
State New
Headers show

Comments

Jens Freimann - Feb. 22, 2013, 7:01 p.m.
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(-)
Andreas Färber - Feb. 24, 2013, 11:30 a.m.
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
Christian Borntraeger - Feb. 24, 2013, 2:38 p.m.
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
Paolo Bonzini - Feb. 25, 2013, 7:55 a.m.
> 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
> 
>
Christian Borntraeger - Feb. 25, 2013, 8:09 a.m.
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
Andreas Färber - Feb. 25, 2013, 8:38 a.m.
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
Paolo Bonzini - Feb. 25, 2013, 10:44 a.m.
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
Christian Borntraeger - Feb. 25, 2013, 11:10 a.m.
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
Alexander Graf - March 8, 2013, 8:11 p.m.
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
Cornelia Huck - March 11, 2013, 12:04 p.m.
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
>
Paolo Bonzini - March 11, 2013, 12:16 p.m.
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
Alexander Graf - March 11, 2013, 12:22 p.m.
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
Cornelia Huck - March 11, 2013, 12:32 p.m.
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
>

Patch

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;
 }