Re: [Qemu-devel] [PULL 47/58] qdev-monitor: Unref device when device_add fails
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
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
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
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
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