Hi > -----Original Message----- > From: Igor Mammedov [mailto:imamm...@redhat.com] > Sent: Tuesday, June 12, 2018 9:40 PM > To: liujunjie (A) <liujunji...@huawei.com> > Cc: pbonz...@redhat.com; r...@twiddle.net; crosthwaite.pe...@gmail.com; > linzhecheng <linzhech...@huawei.com>; Huangweidong (C) > <weidong.hu...@huawei.com>; wangxin (U) > <wangxinxin.w...@huawei.com>; qemu-devel@nongnu.org; Gonglei (Arei) > <arei.gong...@huawei.com> > Subject: Re: [Qemu-devel] [PATCH v2] cpu hot-del: leak fix by free the > relevant > members > > On Fri, 8 Jun 2018 17:43:24 +0800 > liujunjie <liujunji...@huawei.com> wrote: > > > THese leaks are found by ASAN with CPU hot-add and hot-del actions, > > such as: > it would be better to split patch into several, 1 leak per patch > > > ==14127==ERROR: LeakSanitizer: detected memory leaks > > > > Direct leak of 4096 byte(s) in 1 object(s) allocated from: > > #0 0x7fc321cb6ec0 in posix_memalign > (/usr/local/gcc-6-4/lib64/libasan.so.3+0xc7ec0) > > #1 0xf756b9 in qemu_try_memalign util/oslib-posix.c:110 > > #2 0xf7575b in qemu_memalign util/oslib-posix.c:126 > > #3 0x7769cb in kvm_arch_init_vcpu > /root/qemu/target/i386/kvm.c:1103 > > #4 0x4cc4e6 in qemu_kvm_cpu_thread_fn /root/qemu/cpus.c:1201 > > #5 0x7fc31cb28dc4 in start_thread > > (/usr/lib64/libpthread.so.0+0x7dc4) > > > > Direct leak of 4096 byte(s) in 1 object(s) allocated from: > > #0 0x7fc321cb6560 in calloc > (/usr/local/gcc-6-4/lib64/libasan.so.3+0xc7560) > > #1 0x7fc31cf944d6 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x4f4d6) > > #2 0x4cc4e6 in qemu_kvm_cpu_thread_fn /root/qemu/cpus.c:1201 > > #3 0x7fc31cb28dc4 in start_thread > > (/usr/lib64/libpthread.so.0+0x7dc4) > > > > Direct leak of 184 byte(s) in 1 object(s) allocated from: > > #0 0x7fc321cb6560 in calloc > (/usr/local/gcc-6-4/lib64/libasan.so.3+0xc7560) > > #1 0x7fc31cf944d6 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x4f4d6) > > #2 0x4cda83 in qemu_init_vcpu /root/qemu/cpus.c:1993 > > #3 0x6aedbd in x86_cpu_realizefn /root/qemu/target/i386/cpu.c:4739 > > #4 0x8edfc8 in device_set_realized hw/core/qdev.c:826 > > #5 0xcff60c in property_set_bool qom/object.c:1928 > > #6 0xd09bd2 in object_property_set_qobject qom/qom-qobject.c:27 > > #7 0xd048e3 in object_property_set_bool qom/object.c:1188 > > #8 0x79ad83 in qdev_device_add /root/qemu/qdev-monitor.c:627 > > #9 0x79b50c in qmp_device_add /root/qemu/qdev-monitor.c:807 > > #10 0xf4ca5d in do_qmp_dispatch qapi/qmp-dispatch.c:119 > > #11 0xf4ce3c in qmp_dispatch qapi/qmp-dispatch.c:168 > > #12 0x4e2a5a in monitor_qmp_dispatch_one > /root/qemu/monitor.c:4088 > > #13 0x4e2baf in monitor_qmp_bh_dispatcher > /root/qemu/monitor.c:4146 > > #14 0xf67ad1 in aio_bh_poll util/async.c:118 > > #15 0xf724a3 in aio_dispatch util/aio-posix.c:436 > > #16 0xf67270 in aio_ctx_dispatch util/async.c:261 > > #17 0x7fc31cf8e999 in g_main_context_dispatch > > (/usr/lib64/libglib-2.0.so.0+0x49999) > > > > Direct leak of 64 byte(s) in 2 object(s) allocated from: > > #0 0x7fc321cb6560 in calloc > (/usr/local/gcc-6-4/lib64/libasan.so.3+0xc7560) > > #1 0x7fc31cf944d6 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x4f4d6) > > #2 0x4cda1f in qemu_init_vcpu /root/qemu/cpus.c:1997 > > #3 0x6aedbd in x86_cpu_realizefn /root/qemu/target/i386/cpu.c:4739 > > #4 0x8edfc8 in device_set_realized hw/core/qdev.c:826 > > #5 0xcff60c in property_set_bool qom/object.c:1928 > > #6 0xd09bd2 in object_property_set_qobject qom/qom-qobject.c:27 > > #7 0xd048e3 in object_property_set_bool qom/object.c:1188 > > #8 0x79ad83 in qdev_device_add /root/qemu/qdev-monitor.c:627 > > #9 0x79b50c in qmp_device_add /root/qemu/qdev-monitor.c:807 > > #10 0xf4ca5d in do_qmp_dispatch qapi/qmp-dispatch.c:119 > > #11 0xf4ce3c in qmp_dispatch qapi/qmp-dispatch.c:168 > > #12 0x4e2a5a in monitor_qmp_dispatch_one > /root/qemu/monitor.c:4088 > > #13 0x4e2baf in monitor_qmp_bh_dispatcher > /root/qemu/monitor.c:4146 > > #14 0xf67ad1 in aio_bh_poll util/async.c:118 > > #15 0xf724a3 in aio_dispatch util/aio-posix.c:436 > > #16 0xf67270 in aio_ctx_dispatch util/async.c:261 > > #17 0x7fc31cf8e999 in g_main_context_dispatch > > (/usr/lib64/libglib-2.0.so.0+0x49999) > back trace (including line numbers) is a moving target so it's only useful for > concrete copy. > I'd drop it and cite offending hunk in commit message instead. > > > > SUMMARY: AddressSanitizer: 8440 byte(s) leaked in 5 allocation(s). > > > > The relevant members in CPU Structure are freed to fix leak. > > Meanwhile, although the VMChangeStateEntry added in kvm_arch_init_vcpu > > does not be reportd by ASAN since it still in vm_change_state_head, it's not > longer used after hot-del, so free it, too. > > > > Signed-off-by: liujunjie <liujunji...@huawei.com> > > Signed-off-by: linzhecheng <linzhech...@huawei.com> > > --- > > accel/kvm/kvm-all.c | 3 +++ > > cpus.c | 6 ++++++ > > include/sysemu/kvm.h | 1 + > > target/i386/cpu.h | 2 ++ > > target/i386/kvm.c | 19 ++++++++++++++++++- > > 5 files changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index > > ffee68e..a0491dc 100644 > > --- a/accel/kvm/kvm-all.c > > +++ b/accel/kvm/kvm-all.c > > @@ -305,6 +305,9 @@ int kvm_destroy_vcpu(CPUState *cpu) > > vcpu->vcpu_id = kvm_arch_vcpu_id(cpu); > > vcpu->kvm_fd = cpu->kvm_fd; > > QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node); > > +#ifdef TARGET_I386 > > + kvm_arch_destroy_vcpu(cpu); > > +#endif > > err: > > return ret; > > } > > diff --git a/cpus.c b/cpus.c > > index d1f1629..cbe28d6 100644 > > --- a/cpus.c > > +++ b/cpus.c > > @@ -1842,6 +1842,12 @@ void cpu_remove_sync(CPUState *cpu) > > qemu_mutex_unlock_iothread(); > > qemu_thread_join(cpu->thread); > > qemu_mutex_lock_iothread(); > > + g_free(cpu->thread); > > + cpu->thread = NULL; > > + g_free(cpu->halt_cond); > > + cpu->halt_cond = NULL; > could you check if it's save to free thread/halt_cond when running in plain > TCG > mode > > > + g_free(cpu->cpu_ases); > > + cpu->cpu_ases = NULL; > consider TCG usecase, cpu_ases is registered with tcg_as_listener, is it > really > safe to free? > > > } > > > [...]
1. Since all these leaks are sit in two locations, one is in the common CPUState structure, the other is in the specific X86CPU structure. It is a good idea to split the whole patch into two patches. 2. I look back to the commit history and find the way to fix memory leak may be like commit ab105cc138, so I try to use the same way. And I do not know what "cite offending hunk in commit message" means. Can you explain more or is there an example? 3. I am not familiar with the plain TCG mode and can not find out whether is safe to free in this mode. I need some time to check it or is there anyone can figure it out?
