Re: [Qemu-devel] [PATCH v6 02/27] bootindex: add del_boot_device_path function

[Gonglei (Arei)]

> Subject: RE: [PATCH v6 02/27] bootindex: add del_boot_device_path function
> 
> Hi,
> 
> > > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path
> function
> > >
> > > On Wed, Sep 03, 2014 at 06:45:56AM +0000, Gonglei (Arei) wrote:
> > > [...]
> > > > > > 4. When we hotplug the virtio-net-pci device, only pass 
> > > > > > virtio-net-pci's
> > > pointer
> > > > > to
> > > > > > del_boot_device_path(). But virtio-net-pci != virtio-net-device, so 
> > > > > > I add
> a
> > > > > function
> > > > > > named is_same_fw_dev_path() to handle this situation.
> > > > >
> > > > > When hot-unplugging virtio-net-pci I'd expect we free both
> > > > > virtio-net-pci and virtio-net-device (and therefore call
> > > > > del_boot_device_path twice, once for each device).  Can you check
> that?
> > > > >
> > > > Yes, I can.
> > > >
> > > > The del_boot_device_path() is called only once, just for virtio-net-pci.
> > > > For its child, virtio-net-devcie's resource is cleaned by qbus->child
> > unrealizing
> > > > process, will not call device_finalize().
> > >
> > > Then we need to fix this to make sure there's a corresponding
> > > del_boot_device_path() call (with the same pointer) to every
> > > add_boot_device_path() call, instead of adding a hack to
> > > del_boot_device_path().
> > >
> > Good idea. We can add functions named $device_finalize_bootindex(), such
> as:
> >
> > static void virtio_net_set_bootindex(Object *obj, Visitor *v, void *opaque,
> >                                      const char *name, Error
> **errp)
> > {
> >     VirtIONet *n = VIRTIO_NET(obj);
> >
> >     set_bootindex(&n->nic_conf, v, name, errp);
> >     add_boot_device_path(n->nic_conf.bootindex,
> >                          DEVICE(obj), "/address@hidden");
> > }
> >
> > static void virtio_net_finalize_bootindex(Object *obj, const char *name,
> >                                           void *opaque)
> > {
> >     del_boot_device_path(DEVICE(obj));
> > }
> > ...
> 
> Oops, I found an issue that when we hot-unplug a virtio-net-pci device,
> the virtio_net_finalize_bootindex function will not be called too.
> Maybe this is a potential *bug* which will cause the virtio-net-device's 
> property
> resource leak.
> 
> Paolo, would you have a look at it? Thanks a lot!
> 
> Backtrace as below:
> 
> #0  object_unref (obj=0x7f207c640468) at qom/object.c:711
> #1  0x00007f207b9850da in bus_remove_child (bus=0x7f207c6403f0,
> child=0x7f207c640468) at hw/core/qdev.c:76
> #2  0x00007f207b987cbc in device_unparent (obj=0x7f207c640468) at
> hw/core/qdev.c:1013
> #3  0x00007f207ba9c5a3 in object_finalize_child_property
> (obj=0x7f207c63fa90, name=0x7f207c6027c0 "virtio-backend", opaque=
>     0x7f207c640468) at qom/object.c:1036
> #4  0x00007f207ba9b9a9 in object_property_del (obj=0x7f207c63fa90,
> name=0x7f207c6027c0 "virtio-backend", errp=0x0)
>     at qom/object.c:778
> #5  0x00007f207ba9a9ca in object_property_del_child (obj=0x7f207c63fa90,
> child=0x7f207c640468, errp=0x0) at qom/object.c:382
> #6  0x00007f207ba9aa83 in object_unparent (obj=0x7f207c640468) at
> qom/object.c:391
> #7  0x00007f207b986666 in bus_unparent (obj=0x7f207c6403f0) at
> hw/core/qdev.c:548
> #8  0x00007f207ba9c5a3 in object_finalize_child_property
> (obj=0x7f207c63fa90, name=0x7f207c5db7a0 "virtio-bus", opaque=
>     0x7f207c6403f0) at qom/object.c:1036
> #9  0x00007f207ba9b9a9 in object_property_del (obj=0x7f207c63fa90,
> name=0x7f207c5db7a0 "virtio-bus", errp=0x0) at qom/object.c:778
> #10 0x00007f207ba9a9ca in object_property_del_child (obj=0x7f207c63fa90,
> child=0x7f207c6403f0, errp=0x0) at qom/object.c:382
> #11 0x00007f207ba9aa83 in object_unparent (obj=0x7f207c6403f0) at
> qom/object.c:391
> #12 0x00007f207b987c8f in device_unparent (obj=0x7f207c63fa90) at
> hw/core/qdev.c:1010
> #13 0x00007f207ba9c5a3 in object_finalize_child_property
> (obj=0x7f207c4f2f90, name=0x7f207c604580 "nic1", opaque=0x7f207c63fa90)
>     at qom/object.c:1036
> #14 0x00007f207ba9b9a9 in object_property_del (obj=0x7f207c4f2f90,
> name=0x7f207c604580 "nic1", errp=0x0) at qom/object.c:778
> #15 0x00007f207ba9a9ca in object_property_del_child (obj=0x7f207c4f2f90,
> child=0x7f207c63fa90, errp=0x0) at qom/object.c:382
> #16 0x00007f207ba9aa83 in object_unparent (obj=0x7f207c63fa90) at
> qom/object.c:391
> #17 0x00007f207b94f7c7 in acpi_pcihp_eject_slot (s=0x7f207c5801e8, bsel=0,
> slots=8) at hw/acpi/pcihp.c:139
> #18 0x00007f207b94fe65 in pci_write (opaque=0x7f207c5801e8, addr=8,
> data=8, size=4) at hw/acpi/pcihp.c:277
> #19 0x00007f207b818a38 in memory_region_write_accessor
> (mr=0x7f207c580df8, addr=8, value=0x7f2075886b38, size=4, shift=0, mask=
>     4294967295) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:443
> #20 0x00007f207b818b74 in access_with_adjusted_size (addr=8,
> value=0x7f2075886b38, size=4, access_size_min=1, access_size_max=4,
>     access=0x7f207b8189ab <memory_region_write_accessor>,
> mr=0x7f207c580df8) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:480
> #21 0x00007f207b81c141 in memory_region_dispatch_write
> (mr=0x7f207c580df8, addr=8, data=8, size=4)
>     at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:1137
> #22 0x00007f207b81fcf0 in io_mem_write (mr=0x7f207c580df8, addr=8, val=8,
> size=4) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:1973
> #23 0x00007f207b7ca289 in address_space_rw (as=0x7f207bfeb1e0
> <address_space_io>, addr=44552, buf=0x7f207b719000 "\b", len=4,
>     is_write=true) at /mnt/sdb/gonglei/qemu.git/qemu/exec.c:2042
> #24 0x00007f207b815266 in kvm_handle_io (port=44552,
> data=0x7f207b719000, direction=1, size=4, count=1)
>     at /mnt/sdb/gonglei/qemu.git/qemu/kvm-all.c:1597
> #25 0x00007f207b8158a4 in kvm_cpu_exec (cpu=0x7f207c4f6610) at
> /mnt/sdb/gonglei/qemu.git/qemu/kvm-all.c:1748
> #26 0x00007f207b7fcd68 in qemu_kvm_cpu_thread_fn (arg=0x7f207c4f6610)
> at /mnt/sdb/gonglei/qemu.git/qemu/cpus.c:940
> #27 0x00007f2078a357f6 in start_thread () from /lib64/libpthread.so.0
> #28 0x00007f207879109d in clone () from /lib64/libc.so.6
> #29 0x0000000000000000 in ?? ()
> (gdb) p *obj
> $64 = {class = 0x7f207c4bc5f0, free = 0x0, properties = {tqh_first =
> 0x7f207c591fd0, tqh_last = 0x7f207c6027a8}, ref = 3, parent =
>     0x7f207c63fa90}
> (gdb) n
> 712         if (!obj) {
> (gdb)
> 715         g_assert(obj->ref > 0);
> (gdb)
> 718         if (atomic_fetch_dec(&obj->ref) == 1) {
> (gdb)
> 721     }
> (gdb) p *obj
> $65 = {class = 0x7f207c4bc5f0, free = 0x0, properties = {tqh_first =
> 0x7f207c591fd0, tqh_last = 0x7f207c6027a8}, ref = 2, parent =
>     0x7f207c63fa90}
> 
> Because of the virtio-net-device object's ref is 3, will not enter 
> object_finailze(),
> *leak resource*. Am I wrong?
> 

I've confirmed that this is a bug, and have posted a patch fix it. 

Please review, thanks!

[PATCH] virtio-pci: fix virtio-net child refcount in transports

Best regards,
-Gonglei