Re: [Qemu-devel] [PULL 47/58] qdev-monitor: Unref device when device_add fails

2013-11-19 Thread Amos Kong 
On Mon, Nov 18, 2013 at 03:35:20PM +0100, Andreas Färber wrote:
 Am 18.11.2013 13:29, schrieb Amos Kong:
  On Tue, Oct 08, 2013 at 07:44:45PM +0200, Andreas Färber wrote:
  From: Stefan Hajnoczi stefa...@redhat.com
 
  qdev_device_add() leaks the created device upon failure.  I suspect this
  problem crept in because qdev_free() unparents the device but does not
  drop a reference - confusing name.
 
  Cc: qemu-sta...@nongnu.org
  Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
  Signed-off-by: Andreas Färber afaer...@suse.de
  
  Hi Stefan,
  
  This commit caused a regression bug:
  
  hotplug more than 32 disks to vm, qemu crash
  
  ---
  
  [amos@amosk qemu]$ cat radd.sh 
  for i in `seq 3 9` a b c d e f 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 
  1f;do
  for j in `seq 1 7` 0;do
  /bin/cp /images/none.qcow2 /tmp/resize$i$j.qcow2
  
  echo drive_add $i.$j id=drv$i$j,file=/tmp/resize$i$j.qcow2,if=none
  echo drive_add $i.$j id=drv$i$j,file=/tmp/resize$i$j.qcow2,if=none | nc -U 
  /tmp/m
  
  echo device_add 
  virtio-blk-pci,id=dev$i$j,drive=drv$i$j,addr=0x$i.$j,multifunction=on
  echo device_add 
  virtio-blk-pci,id=dev$i$j,drive=drv$i$j,addr=0x$i.$j,multifunction=on | nc 
  -U /tmp/m
  done
  done
 
 Hi, thanks for catching this.
 
 Is this only with virtio-blk-pci or with any PCI card or only when
 drives are involved?

I can reproduce by hotplugging virtio-net-pci NIC

for i in `seq 3 9` a b c d e f 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f;do
for j in `seq 1 7` 0;do

echo netdev_add tap,id=dev$i$j | nc -U /tmp/m
echo device_add 
virtio-net-pci,netdev=dev$i$j,id=nic$i$j,addr=0x$i.$j,multifunction=on | nc -U 
/tmp/m

done
done


 Either way it would be really great if you could
 add such tests to Stefan's qtest using QMP, so that it can easily be run
 by everyone.

I will do it.
 
 The stacktrace below is not really telling to me. I wonder if we forget
 to clean up some MemoryRegion in the device that still has a back
 reference or whether the Memory API still references MemoryRegions that
 have been destroyed by the device or forgets the reference devices it
 still needs... Paolo?
 
 I had reviewed the call paths and believe the patch to be 100% good, so
 the fault will very likely be elsewhere.
 
 Regards,
 Andreas
 
  
  
  
  #0  0x558b7f95 in flatview_ref (view=0x0) at 
  /home/devel/qemu/memory.c:300
  #1  0x558b9689 in address_space_get_flatview (as=0x5645d660) at 
  /home/devel/qemu/memory.c:656
  #2  0x558ba416 in address_space_update_topology (as=0x5645d660) 
  at /home/devel/qemu/memory.c:760
  #3  0x558ba5cf in memory_region_transaction_commit () at 
  /home/devel/qemu/memory.c:799
  #4  0x558bcfcc in memory_region_set_enabled (mr=0x5647af08, 
  enabled=false) at /home/devel/qemu/memory.c:1503
  #5  0x5571a0af in do_pci_register_device (pci_dev=0x5647ac10, 
  bus=0x564132b0, name=0x56261100 virtio-blk-pci, devfn=26) at 
  hw/pci/pci.c:846
  #6  0x5571c6cc in pci_qdev_init (qdev=0x5647ac10) at 
  hw/pci/pci.c:1751
  #7  0x55694d70 in device_realize (dev=0x5647ac10, 
  err=0x7fffc8e8) at hw/core/qdev.c:178
  #8  0x556966fc in device_set_realized (obj=0x5647ac10, 
  value=true, err=0x7fffca60) at hw/core/qdev.c:699
  #9  0x557e7b57 in property_set_bool (obj=0x5647ac10, 
  v=0x5679a830, opaque=0x56461b10, name=0x559922ae realized, 
  errp=0x7fffca60)
  at qom/object.c:1315
  #10 0x557e665b in object_property_set (obj=0x5647ac10, 
  v=0x5679a830, name=0x559922ae realized, errp=0x7fffca60) at 
  qom/object.c:803
  #11 0x557e816e in object_property_set_qobject (obj=0x5647ac10, 
  value=0x56678880, name=0x559922ae realized, errp=0x7fffca60) 
  at qom/qom-qobject.c:24
  #12 0x557e6950 in object_property_set_bool (obj=0x5647ac10, 
  value=true, name=0x559922ae realized, errp=0x7fffca60) at 
  qom/object.c:866
  #13 0x55694ca7 in qdev_init (dev=0x5647ac10) at 
  hw/core/qdev.c:163
  #14 0x557c60ee in qdev_device_add (opts=0x56525370) at 
  qdev-monitor.c:543
  #15 0x557c6730 in do_device_add (mon=0x562fb760, 
  qdict=0x5645d440, ret_data=0x7fffcb80) at qdev-monitor.c:656
  #16 0x558c8892 in handle_user_command (mon=0x562fb760, 
  cmdline=0x563f0f60 device_add 
  virtio-blk-pci,id=dev32,drive=drv32,addr=0x3.2,multifunction=on)
  at /home/devel/qemu/monitor.c:4137
  #17 0x558ca10f in monitor_command_cb (mon=0x562fb760, 
  cmdline=0x563f0f60 device_add 
  virtio-blk-pci,id=dev32,drive=drv32,addr=0x3.2,multifunction=on, 
  opaque=0x0) at /home/devel/qemu/monitor.c:4757
  #18 0x557e9491 in readline_handle_byte (rs=0x563f0f60, ch=10) 
  at readline.c:373
  #19 0x558ca045 in monitor_read (opaque=0x562fb760, 
  buf=0x7fffccf0 \n\315\377\377\377\177, size=1) at

Re: [Qemu-devel] [PULL 47/58] qdev-monitor: Unref device when device_add fails

2013-11-19 Thread Paolo Bonzini 
Il 19/11/2013 09:31, Amos Kong ha scritto:
 I can reproduce by hotplugging virtio-net-pci NIC
 
 for i in `seq 3 9` a b c d e f 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f;do
 for j in `seq 1 7` 0;do
 
 echo netdev_add tap,id=dev$i$j | nc -U /tmp/m
 echo device_add 
 virtio-net-pci,netdev=dev$i$j,id=nic$i$j,addr=0x$i.$j,multifunction=on | nc 
 -U /tmp/m
 
 done
 done

Do you have the full command line?

Also, why/where is device_add failing?  Can you add a puts(driver); before the 
unrefs?

I tried something very similar to the above, with commands like

netdev_add bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=dev1f0
device_add virtio-net-pci,netdev=dev1f0,id=nic1f0,addr=0x1f.0,multifunction=on

and it hotplugged all 224 devices this way without any failure.

Did you try running with MALLOC_PERTURB_=42

Re: [Qemu-devel] [PULL 47/58] qdev-monitor: Unref device when device_add fails

2013-11-18 Thread Amos Kong 
On Tue, Oct 08, 2013 at 07:44:45PM +0200, Andreas Färber wrote:
 From: Stefan Hajnoczi stefa...@redhat.com
 
 qdev_device_add() leaks the created device upon failure.  I suspect this
 problem crept in because qdev_free() unparents the device but does not
 drop a reference - confusing name.
 
 Cc: qemu-sta...@nongnu.org
 Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
 Signed-off-by: Andreas Färber afaer...@suse.de

Hi Stefan,

This commit caused a regression bug:

hotplug more than 32 disks to vm, qemu crash

---

[amos@amosk qemu]$ cat radd.sh 
for i in `seq 3 9` a b c d e f 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f;do
for j in `seq 1 7` 0;do
/bin/cp /images/none.qcow2 /tmp/resize$i$j.qcow2

echo drive_add $i.$j id=drv$i$j,file=/tmp/resize$i$j.qcow2,if=none
echo drive_add $i.$j id=drv$i$j,file=/tmp/resize$i$j.qcow2,if=none | nc -U 
/tmp/m

echo device_add 
virtio-blk-pci,id=dev$i$j,drive=drv$i$j,addr=0x$i.$j,multifunction=on
echo device_add 
virtio-blk-pci,id=dev$i$j,drive=drv$i$j,addr=0x$i.$j,multifunction=on | nc -U 
/tmp/m
done
done



#0  0x558b7f95 in flatview_ref (view=0x0) at 
/home/devel/qemu/memory.c:300
#1  0x558b9689 in address_space_get_flatview (as=0x5645d660) at 
/home/devel/qemu/memory.c:656
#2  0x558ba416 in address_space_update_topology (as=0x5645d660) at 
/home/devel/qemu/memory.c:760
#3  0x558ba5cf in memory_region_transaction_commit () at 
/home/devel/qemu/memory.c:799
#4  0x558bcfcc in memory_region_set_enabled (mr=0x5647af08, 
enabled=false) at /home/devel/qemu/memory.c:1503
#5  0x5571a0af in do_pci_register_device (pci_dev=0x5647ac10, 
bus=0x564132b0, name=0x56261100 virtio-blk-pci, devfn=26) at 
hw/pci/pci.c:846
#6  0x5571c6cc in pci_qdev_init (qdev=0x5647ac10) at 
hw/pci/pci.c:1751
#7  0x55694d70 in device_realize (dev=0x5647ac10, 
err=0x7fffc8e8) at hw/core/qdev.c:178
#8  0x556966fc in device_set_realized (obj=0x5647ac10, value=true, 
err=0x7fffca60) at hw/core/qdev.c:699
#9  0x557e7b57 in property_set_bool (obj=0x5647ac10, 
v=0x5679a830, opaque=0x56461b10, name=0x559922ae realized, 
errp=0x7fffca60)
at qom/object.c:1315
#10 0x557e665b in object_property_set (obj=0x5647ac10, 
v=0x5679a830, name=0x559922ae realized, errp=0x7fffca60) at 
qom/object.c:803
#11 0x557e816e in object_property_set_qobject (obj=0x5647ac10, 
value=0x56678880, name=0x559922ae realized, errp=0x7fffca60) at 
qom/qom-qobject.c:24
#12 0x557e6950 in object_property_set_bool (obj=0x5647ac10, 
value=true, name=0x559922ae realized, errp=0x7fffca60) at 
qom/object.c:866
#13 0x55694ca7 in qdev_init (dev=0x5647ac10) at hw/core/qdev.c:163
#14 0x557c60ee in qdev_device_add (opts=0x56525370) at 
qdev-monitor.c:543
#15 0x557c6730 in do_device_add (mon=0x562fb760, 
qdict=0x5645d440, ret_data=0x7fffcb80) at qdev-monitor.c:656
#16 0x558c8892 in handle_user_command (mon=0x562fb760, 
cmdline=0x563f0f60 device_add 
virtio-blk-pci,id=dev32,drive=drv32,addr=0x3.2,multifunction=on)
at /home/devel/qemu/monitor.c:4137
#17 0x558ca10f in monitor_command_cb (mon=0x562fb760, 
cmdline=0x563f0f60 device_add 
virtio-blk-pci,id=dev32,drive=drv32,addr=0x3.2,multifunction=on, 
opaque=0x0) at /home/devel/qemu/monitor.c:4757
#18 0x557e9491 in readline_handle_byte (rs=0x563f0f60, ch=10) at 
readline.c:373
#19 0x558ca045 in monitor_read (opaque=0x562fb760, 
buf=0x7fffccf0 \n\315\377\377\377\177, size=1) at 
/home/devel/qemu/monitor.c:4743
#20 0x557c6cc8 in qemu_chr_be_write (s=0x56269040, 
buf=0x7fffccf0 \n\315\377\377\377\177, len=1) at qemu-char.c:165
#21 0x557cb026 in tcp_chr_read (chan=0x5645fe40, cond=G_IO_IN, 
opaque=0x56269040) at qemu-char.c:2487
#22 0x776ede06 in g_main_context_dispatch () from 
/lib64/libglib-2.0.so.0
#23 0x5578ef33 in glib_pollfds_poll () at main-loop.c:189
#24 0x5578f028 in os_host_main_loop_wait (timeout=77312299) at 
main-loop.c:234
#25 0x5578f100 in main_loop_wait (nonblocking=0) at main-loop.c:483
#26 0x5582e234 in main_loop () at vl.c:2014
#27 0x55835697 in main (argc=14, argv=0x7fffe298, 
envp=0x7fffe310) at vl.c:4362

 ---
  qdev-monitor.c | 2 ++
  1 file changed, 2 insertions(+)
 
 diff --git a/qdev-monitor.c b/qdev-monitor.c
 index b1ce26a..531b258 100644
 --- a/qdev-monitor.c
 +++ b/qdev-monitor.c
 @@ -518,6 +518,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
  }
  if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
  qdev_free(qdev);
 +object_unref(OBJECT(qdev));
  return NULL;
  }
  if (qdev-id) {
 @@ -531,6 +532,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
  g_free(name);
  }
  if (qdev_init(qdev)  0) {
 +

Re: [Qemu-devel] [PULL 47/58] qdev-monitor: Unref device when device_add fails

2013-11-18 Thread Andreas Färber 
Am 18.11.2013 13:29, schrieb Amos Kong:
 On Tue, Oct 08, 2013 at 07:44:45PM +0200, Andreas Färber wrote:
 From: Stefan Hajnoczi stefa...@redhat.com

 qdev_device_add() leaks the created device upon failure.  I suspect this
 problem crept in because qdev_free() unparents the device but does not
 drop a reference - confusing name.

 Cc: qemu-sta...@nongnu.org
 Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
 Signed-off-by: Andreas Färber afaer...@suse.de
 
 Hi Stefan,
 
 This commit caused a regression bug:
 
 hotplug more than 32 disks to vm, qemu crash
 
 ---
 
 [amos@amosk qemu]$ cat radd.sh 
 for i in `seq 3 9` a b c d e f 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f;do
 for j in `seq 1 7` 0;do
 /bin/cp /images/none.qcow2 /tmp/resize$i$j.qcow2
 
 echo drive_add $i.$j id=drv$i$j,file=/tmp/resize$i$j.qcow2,if=none
 echo drive_add $i.$j id=drv$i$j,file=/tmp/resize$i$j.qcow2,if=none | nc -U 
 /tmp/m
 
 echo device_add 
 virtio-blk-pci,id=dev$i$j,drive=drv$i$j,addr=0x$i.$j,multifunction=on
 echo device_add 
 virtio-blk-pci,id=dev$i$j,drive=drv$i$j,addr=0x$i.$j,multifunction=on | nc -U 
 /tmp/m
 done
 done

Hi, thanks for catching this.

Is this only with virtio-blk-pci or with any PCI card or only when
drives are involved? Either way it would be really great if you could
add such tests to Stefan's qtest using QMP, so that it can easily be run
by everyone.

The stacktrace below is not really telling to me. I wonder if we forget
to clean up some MemoryRegion in the device that still has a back
reference or whether the Memory API still references MemoryRegions that
have been destroyed by the device or forgets the reference devices it
still needs... Paolo?

I had reviewed the call paths and believe the patch to be 100% good, so
the fault will very likely be elsewhere.

Regards,
Andreas

 
 
 
 #0  0x558b7f95 in flatview_ref (view=0x0) at 
 /home/devel/qemu/memory.c:300
 #1  0x558b9689 in address_space_get_flatview (as=0x5645d660) at 
 /home/devel/qemu/memory.c:656
 #2  0x558ba416 in address_space_update_topology (as=0x5645d660) 
 at /home/devel/qemu/memory.c:760
 #3  0x558ba5cf in memory_region_transaction_commit () at 
 /home/devel/qemu/memory.c:799
 #4  0x558bcfcc in memory_region_set_enabled (mr=0x5647af08, 
 enabled=false) at /home/devel/qemu/memory.c:1503
 #5  0x5571a0af in do_pci_register_device (pci_dev=0x5647ac10, 
 bus=0x564132b0, name=0x56261100 virtio-blk-pci, devfn=26) at 
 hw/pci/pci.c:846
 #6  0x5571c6cc in pci_qdev_init (qdev=0x5647ac10) at 
 hw/pci/pci.c:1751
 #7  0x55694d70 in device_realize (dev=0x5647ac10, 
 err=0x7fffc8e8) at hw/core/qdev.c:178
 #8  0x556966fc in device_set_realized (obj=0x5647ac10, 
 value=true, err=0x7fffca60) at hw/core/qdev.c:699
 #9  0x557e7b57 in property_set_bool (obj=0x5647ac10, 
 v=0x5679a830, opaque=0x56461b10, name=0x559922ae realized, 
 errp=0x7fffca60)
 at qom/object.c:1315
 #10 0x557e665b in object_property_set (obj=0x5647ac10, 
 v=0x5679a830, name=0x559922ae realized, errp=0x7fffca60) at 
 qom/object.c:803
 #11 0x557e816e in object_property_set_qobject (obj=0x5647ac10, 
 value=0x56678880, name=0x559922ae realized, errp=0x7fffca60) at 
 qom/qom-qobject.c:24
 #12 0x557e6950 in object_property_set_bool (obj=0x5647ac10, 
 value=true, name=0x559922ae realized, errp=0x7fffca60) at 
 qom/object.c:866
 #13 0x55694ca7 in qdev_init (dev=0x5647ac10) at hw/core/qdev.c:163
 #14 0x557c60ee in qdev_device_add (opts=0x56525370) at 
 qdev-monitor.c:543
 #15 0x557c6730 in do_device_add (mon=0x562fb760, 
 qdict=0x5645d440, ret_data=0x7fffcb80) at qdev-monitor.c:656
 #16 0x558c8892 in handle_user_command (mon=0x562fb760, 
 cmdline=0x563f0f60 device_add 
 virtio-blk-pci,id=dev32,drive=drv32,addr=0x3.2,multifunction=on)
 at /home/devel/qemu/monitor.c:4137
 #17 0x558ca10f in monitor_command_cb (mon=0x562fb760, 
 cmdline=0x563f0f60 device_add 
 virtio-blk-pci,id=dev32,drive=drv32,addr=0x3.2,multifunction=on, 
 opaque=0x0) at /home/devel/qemu/monitor.c:4757
 #18 0x557e9491 in readline_handle_byte (rs=0x563f0f60, ch=10) at 
 readline.c:373
 #19 0x558ca045 in monitor_read (opaque=0x562fb760, 
 buf=0x7fffccf0 \n\315\377\377\377\177, size=1) at 
 /home/devel/qemu/monitor.c:4743
 #20 0x557c6cc8 in qemu_chr_be_write (s=0x56269040, 
 buf=0x7fffccf0 \n\315\377\377\377\177, len=1) at qemu-char.c:165
 #21 0x557cb026 in tcp_chr_read (chan=0x5645fe40, cond=G_IO_IN, 
 opaque=0x56269040) at qemu-char.c:2487
 #22 0x776ede06 in g_main_context_dispatch () from 
 /lib64/libglib-2.0.so.0
 #23 0x5578ef33 in glib_pollfds_poll () at main-loop.c:189
 #24 0x5578f028 in os_host_main_loop_wait (timeout=77312299) at

[Qemu-devel] [PULL 47/58] qdev-monitor: Unref device when device_add fails

2013-10-08 Thread Andreas Färber 
From: Stefan Hajnoczi stefa...@redhat.com

qdev_device_add() leaks the created device upon failure.  I suspect this
problem crept in because qdev_free() unparents the device but does not
drop a reference - confusing name.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
Signed-off-by: Andreas Färber afaer...@suse.de
---
 qdev-monitor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index b1ce26a..531b258 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -518,6 +518,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
 }
 if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
 qdev_free(qdev);
+object_unref(OBJECT(qdev));
 return NULL;
 }
 if (qdev-id) {
@@ -531,6 +532,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
 g_free(name);
 }
 if (qdev_init(qdev)  0) {
+object_unref(OBJECT(qdev));
 qerror_report(QERR_DEVICE_INIT_FAILED, driver);
 return NULL;
 }
-- 
1.8.1.4