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), "/ethernet-phy@0");
> }
>
> 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?
Best regards,
-Gonglei
> object_property_add(obj, "bootindex", "int",
> virtio_net_get_bootindex,
> virtio_net_set_bootindex,
> virtio_net_finalize_bootindex, dev, NULL);
>
> as the previous email, we lay add_boot_device_path() in
> $device_set_bootindex,
> and lay del_boot_device_path() in $device_finalize_bootindex is a good idea
> IMO.
>
> Do you like it? Thanks.
>
> Best regards,
> -Gonglei
