[PATCH v2 0/5] spapr: Fix and cleanups for sPAPR CPU core

[PATCH v2 0/5] spapr: Fix and cleanups for sPAPR CPU core

[Greg Kurz]

While reading the code _again_ I spotted a memory leak and I realized
that the realize/unrealize paths are uselessly complex and not really
symmetrical.

This series fixes the leak and re-shuffles the code to make it cleaner.

Tested with 'make check', travis-ci and manual hotplug/unplug of CPU
cores. Also tested error paths by simulating failures when creating
interrupt presenters or when setting the vCPU id.

v2: - enforce symmetry between realize and unrealize
    - unrealize vCPUs with qdev_unrealize()
    - one loop to create/realize and to unrealize/delete vCPUs

---

Greg Kurz (5):
      spapr: Fix leak of CPU machine specific data
      spapr: Unrealize vCPUs with qdev_unrealize()
      spapr: Drop spapr_delete_vcpu() unused argument
      spapr: Make spapr_cpu_core_unrealize() idempotent
      spapr: Simplify spapr_cpu_core_realize() and spapr_cpu_core_unrealize()


 accel/tcg/user-exec-stub.c      |    4 ++
 hw/ppc/spapr_cpu_core.c         |   69 ++++++++++++++++++---------------------
 target/ppc/translate_init.c.inc |    2 +
 3 files changed, 37 insertions(+), 38 deletions(-)

--
Greg

[PATCH v2 1/5] spapr: Fix leak of CPU machine specific data

[Greg Kurz]

When a CPU core is being removed, the machine specific data of each
CPU thread object is leaked.

Fix this by calling the dedicated helper we have for that instead of
simply unparenting the CPU object. Call it from a separate loop in
spapr_cpu_core_unrealize() for symmetry with spapr_cpu_core_realize().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index b03620823adb..c55211214524 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -188,7 +188,6 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
     }
     spapr_irq_cpu_intc_destroy(SPAPR_MACHINE(qdev_get_machine()), cpu);
     cpu_remove_sync(CPU(cpu));
-    object_unparent(OBJECT(cpu));
 }
 
 /*
@@ -213,6 +212,15 @@ static void spapr_cpu_core_reset_handler(void *opaque)
     spapr_cpu_core_reset(opaque);
 }
 
+static void spapr_delete_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
+{
+    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+
+    cpu->machine_data = NULL;
+    g_free(spapr_cpu);
+    object_unparent(OBJECT(cpu));
+}
+
 static void spapr_cpu_core_unrealize(DeviceState *dev)
 {
     SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
@@ -224,6 +232,9 @@ static void spapr_cpu_core_unrealize(DeviceState *dev)
     for (i = 0; i < cc->nr_threads; i++) {
         spapr_unrealize_vcpu(sc->threads[i], sc);
     }
+    for (i = 0; i < cc->nr_threads; i++) {
+        spapr_delete_vcpu(sc->threads[i], sc);
+    }
     g_free(sc->threads);
 }
 
@@ -294,15 +305,6 @@ err:
     return NULL;
 }
 
-static void spapr_delete_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
-{
-    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
-
-    cpu->machine_data = NULL;
-    g_free(spapr_cpu);
-    object_unparent(OBJECT(cpu));
-}
-
 static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
 {
     /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user

[PATCH v2 2/5] spapr: Unrealize vCPUs with qdev_unrealize()

[Greg Kurz]

Since we introduced CPU hot-unplug in sPAPR, we don't unrealize the
vCPU objects explicitly. Instead, we let QOM handle that for us under
object_property_del_all() when the CPU core object is finalized. The
only thing we do is calling cpu_remove_sync() to tear the vCPU thread
down.

This happens to work but it is ugly because:
- we call qdev_realize() but the corresponding qdev_unrealize() is
  buried deep in the QOM code
- we call cpu_remove_sync() to undo qemu_init_vcpu() called by
  ppc_cpu_realize() in target/ppc/translate_init.c.inc
- the CPU init and teardown paths aren't really symmetrical

The latter didn't bite us so far but a future patch that greatly
simplifies the CPU core realize path needs it to avoid a crash
in QOM.

For all these reasons, have ppc_cpu_unrealize() to undo the changes
of ppc_cpu_realize() by calling cpu_remove_sync() at the right place,
and have the sPAPR CPU core code to call qdev_unrealize().

This requires to add a missing stub because translate_init.c.inc is
also compiled for user mode.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 accel/tcg/user-exec-stub.c      |    4 ++++
 hw/ppc/spapr_cpu_core.c         |    4 ++--
 target/ppc/translate_init.c.inc |    2 ++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
index f6d8c8fb6f2d..b876f5c1e45d 100644
--- a/accel/tcg/user-exec-stub.c
+++ b/accel/tcg/user-exec-stub.c
@@ -9,6 +9,10 @@ void cpu_resume(CPUState *cpu)
 {
 }
 
+void cpu_remove_sync(CPUState *cpu)
+{
+}
+
 void qemu_init_vcpu(CPUState *cpu)
 {
 }
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index c55211214524..e4aeb93c0299 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -187,7 +187,7 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
         vmstate_unregister(NULL, &vmstate_spapr_cpu_state, cpu->machine_data);
     }
     spapr_irq_cpu_intc_destroy(SPAPR_MACHINE(qdev_get_machine()), cpu);
-    cpu_remove_sync(CPU(cpu));
+    qdev_unrealize(DEVICE(cpu));
 }
 
 /*
@@ -255,7 +255,7 @@ static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
     kvmppc_set_papr(cpu);
 
     if (spapr_irq_cpu_intc_create(spapr, cpu, errp) < 0) {
-        cpu_remove_sync(CPU(cpu));
+        qdev_unrealize(DEVICE(cpu));
         return false;
     }
 
diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
index bb66526280ef..d2a8204d6011 100644
--- a/target/ppc/translate_init.c.inc
+++ b/target/ppc/translate_init.c.inc
@@ -10328,6 +10328,8 @@ static void ppc_cpu_unrealize(DeviceState *dev)
 
     pcc->parent_unrealize(dev);
 
+    cpu_remove_sync(CPU(cpu));
+
     for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
         if (cpu->opcodes[i] == &invalid_handler) {
             continue;

[PATCH v2 3/5] spapr: Drop spapr_delete_vcpu() unused argument

[Greg Kurz]

The 'sc' argument is unused. Drop it.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index e4aeb93c0299..45eb2121876e 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -212,7 +212,7 @@ static void spapr_cpu_core_reset_handler(void *opaque)
     spapr_cpu_core_reset(opaque);
 }
 
-static void spapr_delete_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
+static void spapr_delete_vcpu(PowerPCCPU *cpu)
 {
     SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
 
@@ -233,7 +233,7 @@ static void spapr_cpu_core_unrealize(DeviceState *dev)
         spapr_unrealize_vcpu(sc->threads[i], sc);
     }
     for (i = 0; i < cc->nr_threads; i++) {
-        spapr_delete_vcpu(sc->threads[i], sc);
+        spapr_delete_vcpu(sc->threads[i]);
     }
     g_free(sc->threads);
 }
@@ -345,7 +345,7 @@ err_unrealize:
     }
 err:
     while (--i >= 0) {
-        spapr_delete_vcpu(sc->threads[i], sc);
+        spapr_delete_vcpu(sc->threads[i]);
     }
     g_free(sc->threads);
 }

[PATCH v2 4/5] spapr: Make spapr_cpu_core_unrealize() idempotent

[Greg Kurz]

spapr_cpu_core_realize() has a rollback path which partially duplicates
the code of spapr_cpu_core_unrealize().

Let's make spapr_cpu_core_unrealize() idempotent and call it instead. This
requires to:
- move the registration and unregistration of the reset handler around
  but it is harmless,
- allocate the array of vCPUs with g_new0() to be able to filter out
  unused slots,
- make sure to only unrealize vCPUs that have been already realized.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c |   41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 45eb2121876e..317fb9934f58 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -227,15 +227,26 @@ static void spapr_cpu_core_unrealize(DeviceState *dev)
     CPUCore *cc = CPU_CORE(dev);
     int i;
 
-    qemu_unregister_reset(spapr_cpu_core_reset_handler, sc);
-
     for (i = 0; i < cc->nr_threads; i++) {
-        spapr_unrealize_vcpu(sc->threads[i], sc);
+        if (sc->threads[i]) {
+            /*
+             * Since this we can get here from the error path of
+             * spapr_cpu_core_realize(), make sure we only unrealize
+             * vCPUs that have already been realized.
+             */
+            if (object_property_get_bool(OBJECT(sc->threads[i]), "realized",
+                                         &error_abort)) {
+                spapr_unrealize_vcpu(sc->threads[i], sc);
+            }
+        }
     }
     for (i = 0; i < cc->nr_threads; i++) {
-        spapr_delete_vcpu(sc->threads[i]);
+        if (sc->threads[i]) {
+            spapr_delete_vcpu(sc->threads[i]);
+        }
     }
     g_free(sc->threads);
+    qemu_unregister_reset(spapr_cpu_core_reset_handler, sc);
 }
 
 static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
@@ -322,32 +333,22 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
+    qemu_register_reset(spapr_cpu_core_reset_handler, sc);
+    sc->threads = g_new0(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
         sc->threads[i] = spapr_create_vcpu(sc, i, errp);
         if (!sc->threads[i]) {
-            goto err;
+            spapr_cpu_core_unrealize(dev);
+            return;
         }
     }
 
     for (j = 0; j < cc->nr_threads; j++) {
         if (!spapr_realize_vcpu(sc->threads[j], spapr, sc, errp)) {
-            goto err_unrealize;
+            spapr_cpu_core_unrealize(dev);
+            return;
         }
     }
-
-    qemu_register_reset(spapr_cpu_core_reset_handler, sc);
-    return;
-
-err_unrealize:
-    while (--j >= 0) {
-        spapr_unrealize_vcpu(sc->threads[j], sc);
-    }
-err:
-    while (--i >= 0) {
-        spapr_delete_vcpu(sc->threads[i]);
-    }
-    g_free(sc->threads);
 }
 
 static Property spapr_cpu_core_properties[] = {

[PATCH v2 5/5] spapr: Simplify spapr_cpu_core_realize() and spapr_cpu_core_unrealize()

[Greg Kurz]

Now that the error path of spapr_cpu_core_realize() is just to call
idempotent spapr_cpu_core_unrealize() for rollback, no need to create
and realize the vCPUs in two separate loops.

Merge them and do them same in spapr_cpu_core_unrealize() for symmetry.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c |   16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 317fb9934f58..2f7dc3c23ded 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -238,10 +238,6 @@ static void spapr_cpu_core_unrealize(DeviceState *dev)
                                          &error_abort)) {
                 spapr_unrealize_vcpu(sc->threads[i], sc);
             }
-        }
-    }
-    for (i = 0; i < cc->nr_threads; i++) {
-        if (sc->threads[i]) {
             spapr_delete_vcpu(sc->threads[i]);
         }
     }
@@ -326,7 +322,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
                                                   TYPE_SPAPR_MACHINE);
     SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(OBJECT(dev));
-    int i, j;
+    int i;
 
     if (!spapr) {
         error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
@@ -337,14 +333,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     sc->threads = g_new0(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
         sc->threads[i] = spapr_create_vcpu(sc, i, errp);
-        if (!sc->threads[i]) {
-            spapr_cpu_core_unrealize(dev);
-            return;
-        }
-    }
-
-    for (j = 0; j < cc->nr_threads; j++) {
-        if (!spapr_realize_vcpu(sc->threads[j], spapr, sc, errp)) {
+        if (!sc->threads[i] ||
+            !spapr_realize_vcpu(sc->threads[i], spapr, sc, errp)) {
             spapr_cpu_core_unrealize(dev);
             return;
         }

Re: [PATCH v2 0/5] spapr: Fix and cleanups for sPAPR CPU core

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On Thu, Oct 15, 2020 at 11:18:18PM +0200, Greg Kurz wrote:
> While reading the code _again_ I spotted a memory leak and I realized
> that the realize/unrealize paths are uselessly complex and not really
> symmetrical.
> 
> This series fixes the leak and re-shuffles the code to make it cleaner.
> 
> Tested with 'make check', travis-ci and manual hotplug/unplug of CPU
> cores. Also tested error paths by simulating failures when creating
> interrupt presenters or when setting the vCPU id.
> 
> v2: - enforce symmetry between realize and unrealize
>     - unrealize vCPUs with qdev_unrealize()
>     - one loop to create/realize and to unrealize/delete vCPUs

Applied to ppc-for-5.2.

> 
> ---
> 
> Greg Kurz (5):
>       spapr: Fix leak of CPU machine specific data
>       spapr: Unrealize vCPUs with qdev_unrealize()
>       spapr: Drop spapr_delete_vcpu() unused argument
>       spapr: Make spapr_cpu_core_unrealize() idempotent
>       spapr: Simplify spapr_cpu_core_realize() and spapr_cpu_core_unrealize()
> 
> 
>  accel/tcg/user-exec-stub.c      |    4 ++
>  hw/ppc/spapr_cpu_core.c         |   69 ++++++++++++++++++---------------------
>  target/ppc/translate_init.c.inc |    2 +
>  3 files changed, 37 insertions(+), 38 deletions(-)
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]