[PATCH v3 0/5] vCPU hotunplug related memory leaks

[PATCH v3 0/5] vCPU hotunplug related memory leaks

[Mark Kanda]

This series addresses a few vCPU hotunplug related leaks (found with Valgrind).

v3:
- patch 4: create cpu_address_space_destroy() to free cpu_ases (Phillipe)
- patch 5: create <kvm, hvf>_destroy_vcpu_thread() to free xsave_buf (Phillipe)

v2: Create AccelOpsClass::destroy_vcpu_thread() for vcpu thread related cleanup
(Philippe)

Mark Kanda (5):
  accel: Introduce AccelOpsClass::destroy_vcpu_thread()
  softmmu/cpus: Free cpu->thread in generic_destroy_vcpu_thread()
  softmmu/cpus: Free cpu->halt_cond in generic_destroy_vcpu_thread()
  cpu: Free cpu->cpu_ases in cpu_address_space_destroy()
  i386/cpu: Free env->xsave_buf in KVM and HVF destory_vcpu_thread
    routines

 accel/accel-common.c              |  7 +++++++
 accel/hvf/hvf-accel-ops.c         | 10 ++++++++++
 accel/kvm/kvm-accel-ops.c         | 10 ++++++++++
 accel/qtest/qtest.c               |  1 +
 accel/tcg/tcg-accel-ops.c         |  1 +
 accel/xen/xen-all.c               |  1 +
 cpu.c                             |  1 +
 include/exec/cpu-common.h         |  7 +++++++
 include/sysemu/accel-ops.h        |  3 +++
 softmmu/cpus.c                    |  3 +++
 softmmu/physmem.c                 |  5 +++++
 target/i386/hax/hax-accel-ops.c   |  1 +
 target/i386/nvmm/nvmm-accel-ops.c |  1 +
 target/i386/whpx/whpx-accel-ops.c |  1 +
 14 files changed, 52 insertions(+)

-- 
2.27.0

[PATCH v3 1/5] accel: Introduce AccelOpsClass::destroy_vcpu_thread()

[Mark Kanda]

Add destroy_vcpu_thread() to AccelOps as a method for vcpu thread cleanup.
This will be used in subsequent patches.

Suggested-by: Philippe Mathieu-Daudé  <f4bug@amsat.org>
Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/sysemu/accel-ops.h | 1 +
 softmmu/cpus.c             | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h
index 6013c9444c..e296b27b82 100644
--- a/include/sysemu/accel-ops.h
+++ b/include/sysemu/accel-ops.h
@@ -31,6 +31,7 @@ struct AccelOpsClass {
     bool (*cpus_are_resettable)(void);
 
     void (*create_vcpu_thread)(CPUState *cpu); /* MANDATORY NON-NULL */
+    void (*destroy_vcpu_thread)(CPUState *cpu);
     void (*kick_vcpu_thread)(CPUState *cpu);
     bool (*cpu_thread_is_idle)(CPUState *cpu);
 
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 7b75bb66d5..622f8b4608 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -609,6 +609,9 @@ void cpu_remove_sync(CPUState *cpu)
     qemu_mutex_unlock_iothread();
     qemu_thread_join(cpu->thread);
     qemu_mutex_lock_iothread();
+    if (cpus_accel->destroy_vcpu_thread) {
+        cpus_accel->destroy_vcpu_thread(cpu);
+    }
 }
 
 void cpus_register_accel(const AccelOpsClass *ops)
-- 
2.27.0

[PATCH v3 2/5] softmmu/cpus: Free cpu->thread in generic_destroy_vcpu_thread()

[Mark Kanda]

Free cpu->thread in a new AccelOpsClass::destroy_vcpu_thread() handler
generic_destroy_vcpu_thread().

vCPU hotunplug related leak reported by Valgrind:

==102631== 8 bytes in 1 blocks are definitely lost in loss record 1,037 of 8,555
==102631==    at 0x4C3ADBB: calloc (vg_replace_malloc.c:1117)
==102631==    by 0x69EE4CD: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5600.4)
==102631==    by 0x92443A: kvm_start_vcpu_thread (kvm-accel-ops.c:68)
==102631==    by 0x4505C2: qemu_init_vcpu (cpus.c:643)
==102631==    by 0x76B4D1: x86_cpu_realizefn (cpu.c:6520)
==102631==    by 0x9344A7: device_set_realized (qdev.c:531)
==102631==    by 0x93E329: property_set_bool (object.c:2273)
==102631==    by 0x93C2F8: object_property_set (object.c:1408)
==102631==    by 0x940796: object_property_set_qobject (qom-qobject.c:28)
==102631==    by 0x93C663: object_property_set_bool (object.c:1477)
==102631==    by 0x933D3B: qdev_realize (qdev.c:333)
==102631==    by 0x455EC4: qdev_device_add_from_qdict (qdev-monitor.c:713)

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
 accel/accel-common.c              | 6 ++++++
 accel/hvf/hvf-accel-ops.c         | 1 +
 accel/kvm/kvm-accel-ops.c         | 1 +
 accel/qtest/qtest.c               | 1 +
 accel/tcg/tcg-accel-ops.c         | 1 +
 accel/xen/xen-all.c               | 1 +
 include/sysemu/accel-ops.h        | 2 ++
 target/i386/hax/hax-accel-ops.c   | 1 +
 target/i386/nvmm/nvmm-accel-ops.c | 1 +
 target/i386/whpx/whpx-accel-ops.c | 1 +
 10 files changed, 16 insertions(+)

diff --git a/accel/accel-common.c b/accel/accel-common.c
index 7b8ec7e0f7..623df43cc3 100644
--- a/accel/accel-common.c
+++ b/accel/accel-common.c
@@ -28,6 +28,7 @@
 
 #include "cpu.h"
 #include "hw/core/accel-cpu.h"
+#include "sysemu/accel-ops.h"
 
 #ifndef CONFIG_USER_ONLY
 #include "accel-softmmu.h"
@@ -135,3 +136,8 @@ static void register_accel_types(void)
 }
 
 type_init(register_accel_types);
+
+void generic_destroy_vcpu_thread(CPUState *cpu)
+{
+    g_free(cpu->thread);
+}
diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
index 54457c76c2..b23a67881c 100644
--- a/accel/hvf/hvf-accel-ops.c
+++ b/accel/hvf/hvf-accel-ops.c
@@ -467,6 +467,7 @@ static void hvf_accel_ops_class_init(ObjectClass *oc, void *data)
     AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
     ops->create_vcpu_thread = hvf_start_vcpu_thread;
+    ops->destroy_vcpu_thread = generic_destroy_vcpu_thread;
     ops->kick_vcpu_thread = hvf_kick_vcpu_thread;
 
     ops->synchronize_post_reset = hvf_cpu_synchronize_post_reset;
diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c
index c4244a23c6..5a7a9ae79c 100644
--- a/accel/kvm/kvm-accel-ops.c
+++ b/accel/kvm/kvm-accel-ops.c
@@ -89,6 +89,7 @@ static void kvm_accel_ops_class_init(ObjectClass *oc, void *data)
     AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
     ops->create_vcpu_thread = kvm_start_vcpu_thread;
+    ops->destroy_vcpu_thread = generic_destroy_vcpu_thread;
     ops->cpu_thread_is_idle = kvm_vcpu_thread_is_idle;
     ops->cpus_are_resettable = kvm_cpus_are_resettable;
     ops->synchronize_post_reset = kvm_cpu_synchronize_post_reset;
diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
index f6056ac836..ba8573fc2c 100644
--- a/accel/qtest/qtest.c
+++ b/accel/qtest/qtest.c
@@ -51,6 +51,7 @@ static void qtest_accel_ops_class_init(ObjectClass *oc, void *data)
     AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
     ops->create_vcpu_thread = dummy_start_vcpu_thread;
+    ops->destroy_vcpu_thread = generic_destroy_vcpu_thread;
     ops->get_virtual_clock = qtest_get_virtual_clock;
 };
 
diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index ea7dcad674..527592c4d7 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -94,6 +94,7 @@ void tcg_handle_interrupt(CPUState *cpu, int mask)
 
 static void tcg_accel_ops_init(AccelOpsClass *ops)
 {
+    ops->destroy_vcpu_thread = generic_destroy_vcpu_thread;
     if (qemu_tcg_mttcg_enabled()) {
         ops->create_vcpu_thread = mttcg_start_vcpu_thread;
         ops->kick_vcpu_thread = mttcg_kick_vcpu_thread;
diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 69aa7d018b..0efda554cc 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -220,6 +220,7 @@ static void xen_accel_ops_class_init(ObjectClass *oc, void *data)
     AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
     ops->create_vcpu_thread = dummy_start_vcpu_thread;
+    ops->destroy_vcpu_thread = generic_destroy_vcpu_thread;
 }
 
 static const TypeInfo xen_accel_ops_type = {
diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h
index e296b27b82..fac7d6b34e 100644
--- a/include/sysemu/accel-ops.h
+++ b/include/sysemu/accel-ops.h
@@ -46,4 +46,6 @@ struct AccelOpsClass {
     int64_t (*get_elapsed_ticks)(void);
 };
 
+/* free vcpu thread structures */
+void generic_destroy_vcpu_thread(CPUState *cpu);
 #endif /* ACCEL_OPS_H */
diff --git a/target/i386/hax/hax-accel-ops.c b/target/i386/hax/hax-accel-ops.c
index 136630e9b2..b6ef246cbb 100644
--- a/target/i386/hax/hax-accel-ops.c
+++ b/target/i386/hax/hax-accel-ops.c
@@ -79,6 +79,7 @@ static void hax_accel_ops_class_init(ObjectClass *oc, void *data)
     AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
     ops->create_vcpu_thread = hax_start_vcpu_thread;
+    ops->destroy_vcpu_thread = generic_destroy_vcpu_thread;
     ops->kick_vcpu_thread = hax_kick_vcpu_thread;
 
     ops->synchronize_post_reset = hax_cpu_synchronize_post_reset;
diff --git a/target/i386/nvmm/nvmm-accel-ops.c b/target/i386/nvmm/nvmm-accel-ops.c
index f788f75289..ff9033127a 100644
--- a/target/i386/nvmm/nvmm-accel-ops.c
+++ b/target/i386/nvmm/nvmm-accel-ops.c
@@ -88,6 +88,7 @@ static void nvmm_accel_ops_class_init(ObjectClass *oc, void *data)
     AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
     ops->create_vcpu_thread = nvmm_start_vcpu_thread;
+    ops->destroy_vcpu_thread = generic_destroy_vcpu_thread;
     ops->kick_vcpu_thread = nvmm_kick_vcpu_thread;
 
     ops->synchronize_post_reset = nvmm_cpu_synchronize_post_reset;
diff --git a/target/i386/whpx/whpx-accel-ops.c b/target/i386/whpx/whpx-accel-ops.c
index 1d30e4e2ed..f4ec6b3518 100644
--- a/target/i386/whpx/whpx-accel-ops.c
+++ b/target/i386/whpx/whpx-accel-ops.c
@@ -93,6 +93,7 @@ static void whpx_accel_ops_class_init(ObjectClass *oc, void *data)
     AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
     ops->create_vcpu_thread = whpx_start_vcpu_thread;
+    ops->destroy_vcpu_thread = generic_destroy_vcpu_thread;
     ops->kick_vcpu_thread = whpx_kick_vcpu_thread;
     ops->cpu_thread_is_idle = whpx_vcpu_thread_is_idle;
 
-- 
2.27.0

[PATCH v3 3/5] softmmu/cpus: Free cpu->halt_cond in generic_destroy_vcpu_thread()

[Mark Kanda]

vCPU hotunplug related leak reported by Valgrind:

==102631== 56 bytes in 1 blocks are definitely lost in loss record 5,089 of 8,555
==102631==    at 0x4C3ADBB: calloc (vg_replace_malloc.c:1117)
==102631==    by 0x69EE4CD: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5600.4)
==102631==    by 0x924452: kvm_start_vcpu_thread (kvm-accel-ops.c:69)
==102631==    by 0x4505C2: qemu_init_vcpu (cpus.c:643)
==102631==    by 0x76B4D1: x86_cpu_realizefn (cpu.c:6520)
==102631==    by 0x9344A7: device_set_realized (qdev.c:531)
==102631==    by 0x93E329: property_set_bool (object.c:2273)
==102631==    by 0x93C2F8: object_property_set (object.c:1408)
==102631==    by 0x940796: object_property_set_qobject (qom-qobject.c:28)
==102631==    by 0x93C663: object_property_set_bool (object.c:1477)
==102631==    by 0x933D3B: qdev_realize (qdev.c:333)
==102631==    by 0x455EC4: qdev_device_add_from_qdict (qdev-monitor.c:713)

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
 accel/accel-common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/accel/accel-common.c b/accel/accel-common.c
index 623df43cc3..297d4e4ef1 100644
--- a/accel/accel-common.c
+++ b/accel/accel-common.c
@@ -140,4 +140,5 @@ type_init(register_accel_types);
 void generic_destroy_vcpu_thread(CPUState *cpu)
 {
     g_free(cpu->thread);
+    g_free(cpu->halt_cond);
 }
-- 
2.27.0

[PATCH v3 4/5] cpu: Free cpu->cpu_ases in cpu_address_space_destroy()

[Mark Kanda]

Create cpu_address_space_destroy() to free a CPU's cpu_ases list.

vCPU hotunplug related leak reported by Valgrind:

==132362== 216 bytes in 1 blocks are definitely lost in loss record 7,119 of 8,549
==132362==    at 0x4C3ADBB: calloc (vg_replace_malloc.c:1117)
==132362==    by 0x69EE4CD: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5600.4)
==132362==    by 0x7E34AF: cpu_address_space_init (physmem.c:751)
==132362==    by 0x45053E: qemu_init_vcpu (cpus.c:635)
==132362==    by 0x76B4A7: x86_cpu_realizefn (cpu.c:6520)
==132362==    by 0x9343ED: device_set_realized (qdev.c:531)
==132362==    by 0x93E26F: property_set_bool (object.c:2273)
==132362==    by 0x93C23E: object_property_set (object.c:1408)
==132362==    by 0x9406DC: object_property_set_qobject (qom-qobject.c:28)
==132362==    by 0x93C5A9: object_property_set_bool (object.c:1477)
==132362==    by 0x933C81: qdev_realize (qdev.c:333)
==132362==    by 0x455E9A: qdev_device_add_from_qdict (qdev-monitor.c:713)

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
 cpu.c                     | 1 +
 include/exec/cpu-common.h | 7 +++++++
 softmmu/physmem.c         | 5 +++++
 3 files changed, 13 insertions(+)

diff --git a/cpu.c b/cpu.c
index be1f8b074c..59352a1487 100644
--- a/cpu.c
+++ b/cpu.c
@@ -174,6 +174,7 @@ void cpu_exec_unrealizefn(CPUState *cpu)
         tcg_exec_unrealizefn(cpu);
     }
 
+    cpu_address_space_destroy(cpu);
     cpu_list_remove(cpu);
 }
 
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 50a7d2912e..b17ad61ae4 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -111,6 +111,13 @@ size_t qemu_ram_pagesize_largest(void);
  */
 void cpu_address_space_init(CPUState *cpu, int asidx,
                             const char *prefix, MemoryRegion *mr);
+/**
+ * cpu_address_space_destroy:
+ * @cpu: CPU for this address space
+ *
+ * Cleanup CPU's cpu_ases list.
+ */
+void cpu_address_space_destroy(CPUState *cpu);
 
 void cpu_physical_memory_rw(hwaddr addr, void *buf,
                             hwaddr len, bool is_write);
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 43ae70fbe2..aec61ca07a 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -762,6 +762,11 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
     }
 }
 
+void cpu_address_space_destroy(CPUState *cpu)
+{
+    g_free(cpu->cpu_ases);
+}
+
 AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
 {
     /* Return the AddressSpace corresponding to the specified index */
-- 
2.27.0

[PATCH v3 5/5] i386/cpu: Free env->xsave_buf in KVM and HVF destory_vcpu_thread routines

[Mark Kanda]

Create KVM and HVF specific destory_vcpu_thread() routines to free
env->xsave_buf.

vCPU hotunplug related leak reported by Valgrind:

==132362== 4,096 bytes in 1 blocks are definitely lost in loss record 8,440 of 8,549
==132362==    at 0x4C3B15F: memalign (vg_replace_malloc.c:1265)
==132362==    by 0x4C3B288: posix_memalign (vg_replace_malloc.c:1429)
==132362==    by 0xB41195: qemu_try_memalign (memalign.c:53)
==132362==    by 0xB41204: qemu_memalign (memalign.c:73)
==132362==    by 0x7131CB: kvm_init_xsave (kvm.c:1601)
==132362==    by 0x7148ED: kvm_arch_init_vcpu (kvm.c:2031)
==132362==    by 0x91D224: kvm_init_vcpu (kvm-all.c:516)
==132362==    by 0x9242C9: kvm_vcpu_thread_fn (kvm-accel-ops.c:40)
==132362==    by 0xB2EB26: qemu_thread_start (qemu-thread-posix.c:556)
==132362==    by 0x7EB2159: start_thread (in /usr/lib64/libpthread-2.28.so)
==132362==    by 0x9D45DD2: clone (in /usr/lib64/libc-2.28.so)

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
 accel/hvf/hvf-accel-ops.c | 11 ++++++++++-
 accel/kvm/kvm-accel-ops.c | 11 ++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
index b23a67881c..bc53890352 100644
--- a/accel/hvf/hvf-accel-ops.c
+++ b/accel/hvf/hvf-accel-ops.c
@@ -462,12 +462,21 @@ static void hvf_start_vcpu_thread(CPUState *cpu)
                        cpu, QEMU_THREAD_JOINABLE);
 }
 
+static void hvf_destroy_vcpu_thread(CPUState *cpu)
+{
+    X86CPU *x86_cpu = X86_CPU(cpu);
+    CPUX86State *env = &x86_cpu->env;
+
+    g_free(env->xsave_buf);
+    generic_destroy_vcpu_thread(cpu);
+}
+
 static void hvf_accel_ops_class_init(ObjectClass *oc, void *data)
 {
     AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
     ops->create_vcpu_thread = hvf_start_vcpu_thread;
-    ops->destroy_vcpu_thread = generic_destroy_vcpu_thread;
+    ops->destroy_vcpu_thread = hvf_destroy_vcpu_thread;
     ops->kick_vcpu_thread = hvf_kick_vcpu_thread;
 
     ops->synchronize_post_reset = hvf_cpu_synchronize_post_reset;
diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c
index 5a7a9ae79c..0345a30139 100644
--- a/accel/kvm/kvm-accel-ops.c
+++ b/accel/kvm/kvm-accel-ops.c
@@ -74,6 +74,15 @@ static void kvm_start_vcpu_thread(CPUState *cpu)
                        cpu, QEMU_THREAD_JOINABLE);
 }
 
+static void kvm_destroy_vcpu_thread(CPUState *cpu)
+{
+    X86CPU *x86_cpu = X86_CPU(cpu);
+    CPUX86State *env = &x86_cpu->env;
+
+    g_free(env->xsave_buf);
+    generic_destroy_vcpu_thread(cpu);
+}
+
 static bool kvm_vcpu_thread_is_idle(CPUState *cpu)
 {
     return !kvm_halt_in_kernel();
@@ -89,7 +98,7 @@ static void kvm_accel_ops_class_init(ObjectClass *oc, void *data)
     AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
     ops->create_vcpu_thread = kvm_start_vcpu_thread;
-    ops->destroy_vcpu_thread = generic_destroy_vcpu_thread;
+    ops->destroy_vcpu_thread = kvm_destroy_vcpu_thread;
     ops->cpu_thread_is_idle = kvm_vcpu_thread_is_idle;
     ops->cpus_are_resettable = kvm_cpus_are_resettable;
     ops->synchronize_post_reset = kvm_cpu_synchronize_post_reset;
-- 
2.27.0

Re: [PATCH v3 4/5] cpu: Free cpu->cpu_ases in cpu_address_space_destroy()

[Philippe Mathieu-Daudé]

On 21/3/22 15:14, Mark Kanda wrote:
> Create cpu_address_space_destroy() to free a CPU's cpu_ases list.
> 
> vCPU hotunplug related leak reported by Valgrind:
> 
> ==132362== 216 bytes in 1 blocks are definitely lost in loss record 7,119 of 8,549
> ==132362==    at 0x4C3ADBB: calloc (vg_replace_malloc.c:1117)
> ==132362==    by 0x69EE4CD: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5600.4)
> ==132362==    by 0x7E34AF: cpu_address_space_init (physmem.c:751)
> ==132362==    by 0x45053E: qemu_init_vcpu (cpus.c:635)
> ==132362==    by 0x76B4A7: x86_cpu_realizefn (cpu.c:6520)
> ==132362==    by 0x9343ED: device_set_realized (qdev.c:531)
> ==132362==    by 0x93E26F: property_set_bool (object.c:2273)
> ==132362==    by 0x93C23E: object_property_set (object.c:1408)
> ==132362==    by 0x9406DC: object_property_set_qobject (qom-qobject.c:28)
> ==132362==    by 0x93C5A9: object_property_set_bool (object.c:1477)
> ==132362==    by 0x933C81: qdev_realize (qdev.c:333)
> ==132362==    by 0x455E9A: qdev_device_add_from_qdict (qdev-monitor.c:713)
> 
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> ---
>   cpu.c                     | 1 +
>   include/exec/cpu-common.h | 7 +++++++
>   softmmu/physmem.c         | 5 +++++
>   3 files changed, 13 insertions(+)

Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v3 5/5] i386/cpu: Free env->xsave_buf in KVM and HVF destory_vcpu_thread routines

[Philippe Mathieu-Daudé]

On 21/3/22 15:14, Mark Kanda wrote:
> Create KVM and HVF specific destory_vcpu_thread() routines to free

Typo "destroy"

> env->xsave_buf.
> 
> vCPU hotunplug related leak reported by Valgrind:
> 
> ==132362== 4,096 bytes in 1 blocks are definitely lost in loss record 8,440 of 8,549
> ==132362==    at 0x4C3B15F: memalign (vg_replace_malloc.c:1265)
> ==132362==    by 0x4C3B288: posix_memalign (vg_replace_malloc.c:1429)
> ==132362==    by 0xB41195: qemu_try_memalign (memalign.c:53)
> ==132362==    by 0xB41204: qemu_memalign (memalign.c:73)
> ==132362==    by 0x7131CB: kvm_init_xsave (kvm.c:1601)
> ==132362==    by 0x7148ED: kvm_arch_init_vcpu (kvm.c:2031)
> ==132362==    by 0x91D224: kvm_init_vcpu (kvm-all.c:516)
> ==132362==    by 0x9242C9: kvm_vcpu_thread_fn (kvm-accel-ops.c:40)
> ==132362==    by 0xB2EB26: qemu_thread_start (qemu-thread-posix.c:556)
> ==132362==    by 0x7EB2159: start_thread (in /usr/lib64/libpthread-2.28.so)
> ==132362==    by 0x9D45DD2: clone (in /usr/lib64/libc-2.28.so)
> 
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> ---
>   accel/hvf/hvf-accel-ops.c | 11 ++++++++++-
>   accel/kvm/kvm-accel-ops.c | 11 ++++++++++-
>   2 files changed, 20 insertions(+), 2 deletions(-)

Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v3 2/5] softmmu/cpus: Free cpu->thread in generic_destroy_vcpu_thread()

[Philippe Mathieu-Daudé]

On 21/3/22 15:14, Mark Kanda wrote:
> Free cpu->thread in a new AccelOpsClass::destroy_vcpu_thread() handler
> generic_destroy_vcpu_thread().
> 
> vCPU hotunplug related leak reported by Valgrind:
> 
> ==102631== 8 bytes in 1 blocks are definitely lost in loss record 1,037 of 8,555
> ==102631==    at 0x4C3ADBB: calloc (vg_replace_malloc.c:1117)
> ==102631==    by 0x69EE4CD: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5600.4)
> ==102631==    by 0x92443A: kvm_start_vcpu_thread (kvm-accel-ops.c:68)
> ==102631==    by 0x4505C2: qemu_init_vcpu (cpus.c:643)
> ==102631==    by 0x76B4D1: x86_cpu_realizefn (cpu.c:6520)
> ==102631==    by 0x9344A7: device_set_realized (qdev.c:531)
> ==102631==    by 0x93E329: property_set_bool (object.c:2273)
> ==102631==    by 0x93C2F8: object_property_set (object.c:1408)
> ==102631==    by 0x940796: object_property_set_qobject (qom-qobject.c:28)
> ==102631==    by 0x93C663: object_property_set_bool (object.c:1477)
> ==102631==    by 0x933D3B: qdev_realize (qdev.c:333)
> ==102631==    by 0x455EC4: qdev_device_add_from_qdict (qdev-monitor.c:713)
> 
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> ---
>   accel/accel-common.c              | 6 ++++++
>   accel/hvf/hvf-accel-ops.c         | 1 +
>   accel/kvm/kvm-accel-ops.c         | 1 +
>   accel/qtest/qtest.c               | 1 +
>   accel/tcg/tcg-accel-ops.c         | 1 +
>   accel/xen/xen-all.c               | 1 +
>   include/sysemu/accel-ops.h        | 2 ++
>   target/i386/hax/hax-accel-ops.c   | 1 +
>   target/i386/nvmm/nvmm-accel-ops.c | 1 +
>   target/i386/whpx/whpx-accel-ops.c | 1 +
>   10 files changed, 16 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v3 4/5] cpu: Free cpu->cpu_ases in cpu_address_space_destroy()

[Philippe Mathieu-Daudé]

On 21/3/22 23:03, Philippe Mathieu-Daudé wrote:
> On 21/3/22 15:14, Mark Kanda wrote:
>> Create cpu_address_space_destroy() to free a CPU's cpu_ases list.
>>
>> vCPU hotunplug related leak reported by Valgrind:
>>
>> ==132362== 216 bytes in 1 blocks are definitely lost in loss record 
>> 7,119 of 8,549
>> ==132362==    at 0x4C3ADBB: calloc (vg_replace_malloc.c:1117)
>> ==132362==    by 0x69EE4CD: g_malloc0 (in 
>> /usr/lib64/libglib-2.0.so.0.5600.4)
>> ==132362==    by 0x7E34AF: cpu_address_space_init (physmem.c:751)
>> ==132362==    by 0x45053E: qemu_init_vcpu (cpus.c:635)
>> ==132362==    by 0x76B4A7: x86_cpu_realizefn (cpu.c:6520)
>> ==132362==    by 0x9343ED: device_set_realized (qdev.c:531)
>> ==132362==    by 0x93E26F: property_set_bool (object.c:2273)
>> ==132362==    by 0x93C23E: object_property_set (object.c:1408)
>> ==132362==    by 0x9406DC: object_property_set_qobject (qom-qobject.c:28)
>> ==132362==    by 0x93C5A9: object_property_set_bool (object.c:1477)
>> ==132362==    by 0x933C81: qdev_realize (qdev.c:333)
>> ==132362==    by 0x455E9A: qdev_device_add_from_qdict 
>> (qdev-monitor.c:713)
>>
>> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
>> ---
>>   cpu.c                     | 1 +
>>   include/exec/cpu-common.h | 7 +++++++
>>   softmmu/physmem.c         | 5 +++++
>>   3 files changed, 13 insertions(+)
> 
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Err I meant:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v3 5/5] i386/cpu: Free env->xsave_buf in KVM and HVF destory_vcpu_thread routines

[Philippe Mathieu-Daudé]

On 21/3/22 23:04, Philippe Mathieu-Daudé wrote:
> On 21/3/22 15:14, Mark Kanda wrote:
>> Create KVM and HVF specific destory_vcpu_thread() routines to free
> 
> Typo "destroy"
> 
>> env->xsave_buf.
>>
>> vCPU hotunplug related leak reported by Valgrind:
>>
>> ==132362== 4,096 bytes in 1 blocks are definitely lost in loss record 
>> 8,440 of 8,549
>> ==132362==    at 0x4C3B15F: memalign (vg_replace_malloc.c:1265)
>> ==132362==    by 0x4C3B288: posix_memalign (vg_replace_malloc.c:1429)
>> ==132362==    by 0xB41195: qemu_try_memalign (memalign.c:53)
>> ==132362==    by 0xB41204: qemu_memalign (memalign.c:73)
>> ==132362==    by 0x7131CB: kvm_init_xsave (kvm.c:1601)
>> ==132362==    by 0x7148ED: kvm_arch_init_vcpu (kvm.c:2031)
>> ==132362==    by 0x91D224: kvm_init_vcpu (kvm-all.c:516)
>> ==132362==    by 0x9242C9: kvm_vcpu_thread_fn (kvm-accel-ops.c:40)
>> ==132362==    by 0xB2EB26: qemu_thread_start (qemu-thread-posix.c:556)
>> ==132362==    by 0x7EB2159: start_thread (in 
>> /usr/lib64/libpthread-2.28.so)
>> ==132362==    by 0x9D45DD2: clone (in /usr/lib64/libc-2.28.so)
>>
>> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
>> ---
>>   accel/hvf/hvf-accel-ops.c | 11 ++++++++++-
>>   accel/kvm/kvm-accel-ops.c | 11 ++++++++++-
>>   2 files changed, 20 insertions(+), 2 deletions(-)
> 
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I meant:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v3 3/5] softmmu/cpus: Free cpu->halt_cond in generic_destroy_vcpu_thread()

[Philippe Mathieu-Daudé]

On 21/3/22 15:14, Mark Kanda wrote:
> vCPU hotunplug related leak reported by Valgrind:
> 
> ==102631== 56 bytes in 1 blocks are definitely lost in loss record 5,089 of 8,555
> ==102631==    at 0x4C3ADBB: calloc (vg_replace_malloc.c:1117)
> ==102631==    by 0x69EE4CD: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5600.4)
> ==102631==    by 0x924452: kvm_start_vcpu_thread (kvm-accel-ops.c:69)

Here we want to extract a common generic_init_vcpu_thread().

> ==102631==    by 0x4505C2: qemu_init_vcpu (cpus.c:643)
> ==102631==    by 0x76B4D1: x86_cpu_realizefn (cpu.c:6520)
> ==102631==    by 0x9344A7: device_set_realized (qdev.c:531)
> ==102631==    by 0x93E329: property_set_bool (object.c:2273)
> ==102631==    by 0x93C2F8: object_property_set (object.c:1408)
> ==102631==    by 0x940796: object_property_set_qobject (qom-qobject.c:28)
> ==102631==    by 0x93C663: object_property_set_bool (object.c:1477)
> ==102631==    by 0x933D3B: qdev_realize (qdev.c:333)
> ==102631==    by 0x455EC4: qdev_device_add_from_qdict (qdev-monitor.c:713)
> 
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> ---
>   accel/accel-common.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/accel/accel-common.c b/accel/accel-common.c
> index 623df43cc3..297d4e4ef1 100644
> --- a/accel/accel-common.c
> +++ b/accel/accel-common.c
> @@ -140,4 +140,5 @@ type_init(register_accel_types);
>   void generic_destroy_vcpu_thread(CPUState *cpu)
>   {
>       g_free(cpu->thread);
> +    g_free(cpu->halt_cond);
>   }

Re: [PATCH v3 5/5] i386/cpu: Free env->xsave_buf in KVM and HVF destory_vcpu_thread routines

[Philippe Mathieu-Daudé]

On 21/3/22 23:08, Philippe Mathieu-Daudé wrote:
> On 21/3/22 23:04, Philippe Mathieu-Daudé wrote:
>> On 21/3/22 15:14, Mark Kanda wrote:
>>> Create KVM and HVF specific destory_vcpu_thread() routines to free
>>
>> Typo "destroy"
>>
>>> env->xsave_buf.
>>>
>>> vCPU hotunplug related leak reported by Valgrind:
>>>
>>> ==132362== 4,096 bytes in 1 blocks are definitely lost in loss record 
>>> 8,440 of 8,549
>>> ==132362==    at 0x4C3B15F: memalign (vg_replace_malloc.c:1265)
>>> ==132362==    by 0x4C3B288: posix_memalign (vg_replace_malloc.c:1429)
>>> ==132362==    by 0xB41195: qemu_try_memalign (memalign.c:53)
>>> ==132362==    by 0xB41204: qemu_memalign (memalign.c:73)
>>> ==132362==    by 0x7131CB: kvm_init_xsave (kvm.c:1601)
>>> ==132362==    by 0x7148ED: kvm_arch_init_vcpu (kvm.c:2031)
>>> ==132362==    by 0x91D224: kvm_init_vcpu (kvm-all.c:516)
>>> ==132362==    by 0x9242C9: kvm_vcpu_thread_fn (kvm-accel-ops.c:40)
>>> ==132362==    by 0xB2EB26: qemu_thread_start (qemu-thread-posix.c:556)
>>> ==132362==    by 0x7EB2159: start_thread (in 
>>> /usr/lib64/libpthread-2.28.so)
>>> ==132362==    by 0x9D45DD2: clone (in /usr/lib64/libc-2.28.so)
>>>
>>> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
>>> ---
>>>   accel/hvf/hvf-accel-ops.c | 11 ++++++++++-
>>>   accel/kvm/kvm-accel-ops.c | 11 ++++++++++-
>>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> I meant:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Withdrawing these tags with HVF.

First xsave_buf is only allocated by KVM, second accel/ directory is
meant to be target agnostic, so x86-specific code there is not right.

Re: [PATCH v3 3/5] softmmu/cpus: Free cpu->halt_cond in generic_destroy_vcpu_thread()

[Mark Kanda]

Thanks Philippe,

On 3/21/2022 5:12 PM, Philippe Mathieu-Daudé wrote:
> On 21/3/22 15:14, Mark Kanda wrote:
>> vCPU hotunplug related leak reported by Valgrind:
>>
>> ==102631== 56 bytes in 1 blocks are definitely lost in loss record 5,089 of 
>> 8,555
>> ==102631==    at 0x4C3ADBB: calloc (vg_replace_malloc.c:1117)
>> ==102631==    by 0x69EE4CD: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5600.4)
>> ==102631==    by 0x924452: kvm_start_vcpu_thread (kvm-accel-ops.c:69)
>
> Here we want to extract a common generic_init_vcpu_thread().
>

How about I add extracting 'generic_init_vcpu_thread()' as a separate cleanup 
patch at the end? I'll also drop my xsave_buf patch due to your followup.

Thanks/regards,
-Mark

>> ==102631==    by 0x4505C2: qemu_init_vcpu (cpus.c:643)
>> ==102631==    by 0x76B4D1: x86_cpu_realizefn (cpu.c:6520)
>> ==102631==    by 0x9344A7: device_set_realized (qdev.c:531)
>> ==102631==    by 0x93E329: property_set_bool (object.c:2273)
>> ==102631==    by 0x93C2F8: object_property_set (object.c:1408)
>> ==102631==    by 0x940796: object_property_set_qobject (qom-qobject.c:28)
>> ==102631==    by 0x93C663: object_property_set_bool (object.c:1477)
>> ==102631==    by 0x933D3B: qdev_realize (qdev.c:333)
>> ==102631==    by 0x455EC4: qdev_device_add_from_qdict (qdev-monitor.c:713)
>>
>> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
>> ---
>>   accel/accel-common.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/accel/accel-common.c b/accel/accel-common.c
>> index 623df43cc3..297d4e4ef1 100644
>> --- a/accel/accel-common.c
>> +++ b/accel/accel-common.c
>> @@ -140,4 +140,5 @@ type_init(register_accel_types);
>>   void generic_destroy_vcpu_thread(CPUState *cpu)
>>   {
>>       g_free(cpu->thread);
>> +    g_free(cpu->halt_cond);
>>   }
>

Re: [PATCH v3 3/5] softmmu/cpus: Free cpu->halt_cond in generic_destroy_vcpu_thread()

[Philippe Mathieu-Daudé]

On 22/3/22 13:52, Mark Kanda wrote:
> Thanks Philippe,
> 
> On 3/21/2022 5:12 PM, Philippe Mathieu-Daudé wrote:
>> On 21/3/22 15:14, Mark Kanda wrote:
>>> vCPU hotunplug related leak reported by Valgrind:
>>>
>>> ==102631== 56 bytes in 1 blocks are definitely lost in loss record 
>>> 5,089 of 8,555
>>> ==102631==    at 0x4C3ADBB: calloc (vg_replace_malloc.c:1117)
>>> ==102631==    by 0x69EE4CD: g_malloc0 (in 
>>> /usr/lib64/libglib-2.0.so.0.5600.4)
>>> ==102631==    by 0x924452: kvm_start_vcpu_thread (kvm-accel-ops.c:69)
>>
>> Here we want to extract a common generic_init_vcpu_thread().
>>
> 
> How about I add extracting 'generic_init_vcpu_thread()' as a separate 
> cleanup patch at the end? I'll also drop my xsave_buf patch due to your 
> followup.

I plan to queue patch #4 for v7.0, but I'd rather have the first ones
reworked by extracting the common vcpu_thread_create() code (which
only differs in TCG/RR). I'll give it a try and send a respin later
today.

Re: [PATCH v3 2/5] softmmu/cpus: Free cpu->thread in generic_destroy_vcpu_thread()

[Paolo Bonzini]

On 3/21/22 15:14, Mark Kanda wrote:
> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> index ea7dcad674..527592c4d7 100644
> --- a/accel/tcg/tcg-accel-ops.c
> +++ b/accel/tcg/tcg-accel-ops.c
> @@ -94,6 +94,7 @@ void tcg_handle_interrupt(CPUState *cpu, int mask)
>   
>   static void tcg_accel_ops_init(AccelOpsClass *ops)
>   {
> +    ops->destroy_vcpu_thread = generic_destroy_vcpu_thread;
>       if (qemu_tcg_mttcg_enabled()) {
>           ops->create_vcpu_thread = mttcg_start_vcpu_thread;
>           ops->kick_vcpu_thread = mttcg_kick_vcpu_thread;

This should not be done for the icount case, where there is only one thread.

Paolo