qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH for-6.2 v2 10/11] machine: Split out the smp parsing code

From: wangyanan (Y)
Subject: Re: [PATCH for-6.2 v2 10/11] machine: Split out the smp parsing code
Date: Thu, 22 Jul 2021 22:29:24 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 
On 2021/7/22 21:07, Andrew Jones wrote:

On Thu, Jul 22, 2021 at 02:24:03PM +0800, wangyanan (Y) wrote:

On 2021/7/20 1:20, Andrew Jones wrote:

On Mon, Jul 19, 2021 at 11:20:42AM +0800, Yanan Wang wrote:

We are going to introduce an unit test for the parser smp_parse()
in hw/core/machine.c, but now machine.c is only built in softmmu.

In order to solve the build dependency on the smp parsing code and
avoid building unrelated stuff for the unit tests, move the related
code from machine.c into a new common file, i.e., machine-smp.c.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
   MAINTAINERS           |   1 +
   hw/core/machine-smp.c | 124 ++++++++++++++++++++++++++++++++++++++++++
   hw/core/machine.c     | 109 -------------------------------------
   hw/core/meson.build   |   1 +
   include/hw/boards.h   |   1 +
   5 files changed, 127 insertions(+), 109 deletions(-)
   create mode 100644 hw/core/machine-smp.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 9100f9a043..70633e3bf4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1626,6 +1626,7 @@ F: cpu.c
   F: hw/core/cpu.c
   F: hw/core/machine-qmp-cmds.c
   F: hw/core/machine.c
+F: hw/core/machine-smp.c

I just noticed that the spacing in this change might not be right.

Right, will fix it.

   F: hw/core/null-machine.c
   F: hw/core/numa.c
   F: hw/cpu/cluster.c
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
new file mode 100644
index 0000000000..6a00cfe44a
--- /dev/null
+++ b/hw/core/machine-smp.c
@@ -0,0 +1,124 @@
+/*
+ * QEMU Machine (related to SMP configuration)
+ *
+ * Copyright (C) 2014 Red Hat Inc
+ *
+ * Authors:
+ *   Marcel Apfelbaum <marcel.a@redhat.com>

This header was obviously copy+pasted without being updated.

Yes, the header was kept unchanged.

But actually I'm not completely sure which field should be updated. :)
Should I add "Copyright (C) 2021, Huawei, Inc." and also the authorship
"Yanan Wang <wangyanan55@huawei.com>" behind the existing ones
or just replace them?

I see what you were attempting to do now. You were deriving this new work
(a source file) from an existing work and you wanted to preserve the
original copyright and authorship. It's not so simple with these types of
projects though. In this case, smp_parse wasn't even part of the original
machine.c file (it came over with commit 6f479566a87d). I think it's
pretty common for these projects to just put whatever your preferred
(or your employer's preferred) copyright/authorship on new files. So, I'd
just replace the fields.

I see, will have some update.

Thanks,
Yanan

I'm interested in what others have to say about this though.

Thanks,
drew



+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/boards.h"
+#include "qapi/error.h"
+
+/*
+ * smp_parse - Generic function used to parse the given SMP configuration
+ *
+ * The topology parameters must be specified equal to or great than one
+ * or just omitted, explicit configuration like "cpus=0" is not allowed.
+ * The omitted parameters will be calculated based on the provided ones.
+ *
+ * maxcpus will default to the value of cpus if omitted and will be used
+ * to compute the missing sockets/cores/threads. cpus will be calculated
+ * from the computed parametrs if omitted.
+ *
+ * In calculation of omitted arch-netural sockets/cores/threads, we prefer
+ * sockets over cores over threads before 6.2, while prefer cores over
+ * sockets over threads since 6.2 on. The arch-specific dies will directly
+ * default to 1 if omitted.
+ */
+void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    unsigned cpus    = config->has_cpus ? config->cpus : 0;
+    unsigned sockets = config->has_sockets ? config->sockets : 0;
+    unsigned dies    = config->has_dies ? config->dies : 1;
+    unsigned cores   = config->has_cores ? config->cores : 0;
+    unsigned threads = config->has_threads ? config->threads : 0;
+    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
+
+    if ((config->has_cpus && config->cpus == 0) ||
+        (config->has_sockets && config->sockets == 0) ||
+        (config->has_dies && config->dies == 0) ||
+        (config->has_cores && config->cores == 0) ||
+        (config->has_threads && config->threads == 0) ||
+        (config->has_maxcpus && config->maxcpus == 0)) {
+        error_setg(errp, "parameters must be equal to or greater than one"
+                   "if provided");
+        return;
+    }
+
+    if (!mc->smp_dies_supported && dies > 1) {
+        error_setg(errp, "dies not supported by this machine's CPU topology");
+        return;
+    }
+
+    maxcpus = maxcpus > 0 ? maxcpus : cpus;
+
+    /* prefer sockets over cores over threads before 6.2 */
+    if (mc->smp_prefer_sockets) {
+        if (sockets == 0) {
+            cores = cores > 0 ? cores : 1;
+            threads = threads > 0 ? threads : 1;
+            sockets = maxcpus / (dies * cores * threads);
+            sockets = sockets > 0 ? sockets : 1;
+        } else if (cores == 0) {
+            threads = threads > 0 ? threads : 1;
+            cores = maxcpus / (sockets * dies * threads);
+            cores = cores > 0 ? cores : 1;
+        } else if (threads == 0) {
+            threads = maxcpus / (sockets * dies * cores);
+            threads = threads > 0 ? threads : 1;
+        }
+    /* prefer cores over sockets over threads since 6.2 */
+    } else {
+        if (cores == 0) {
+            sockets = sockets > 0 ? sockets : 1;
+            threads = threads > 0 ? threads : 1;
+            cores = maxcpus / (sockets * dies * threads);
+            cores = cores > 0 ? cores : 1;
+        } else if (sockets == 0) {
+            threads = threads > 0 ? threads : 1;
+            sockets = maxcpus / (dies * cores * threads);
+            sockets = sockets > 0 ? sockets : 1;
+        } else if (threads == 0) {
+            threads = maxcpus / (sockets * dies * cores);
+            threads = threads > 0 ? threads : 1;
+        }
+    }
+
+    /* use the computed parameters to calculate the omitted cpus */
+    cpus = cpus > 0 ? cpus : sockets * dies * cores * threads;
+    maxcpus = maxcpus > 0 ? maxcpus : cpus;
+
+    if (sockets * dies * cores * threads != maxcpus) {
+        g_autofree char *dies_msg = g_strdup_printf(
+            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
+        error_setg(errp, "Invalid CPU topology: "
+                   "sockets (%u)%s * cores (%u) * threads (%u) "
+                   "!= maxcpus (%u)",
+                   sockets, dies_msg, cores, threads,
+                   maxcpus);
+        return;
+    }
+
+    if (sockets * dies * cores * threads < cpus) {
+        g_autofree char *dies_msg = g_strdup_printf(
+            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
+        error_setg(errp, "Invalid CPU topology: "
+                   "sockets (%u)%s * cores (%u) * threads (%u) < "
+                   "smp_cpus (%u)",
+                   sockets, dies_msg, cores, threads, cpus);
+        return;
+    }
+
+    ms->smp.cpus = cpus;
+    ms->smp.sockets = sockets;
+    ms->smp.dies = dies;
+    ms->smp.cores = cores;
+    ms->smp.threads = threads;
+    ms->smp.max_cpus = maxcpus;
+}
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 9d24b67ef3..61be266b6c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -744,115 +744,6 @@ void machine_set_cpu_numa_node(MachineState *machine,
       }
   }
-/*
- * smp_parse - Generic function used to parse the given SMP configuration
- *
- * The topology parameters must be specified equal to or great than one
- * or just omitted, explicit configuration like "cpus=0" is not allowed.
- * The omitted parameters will be calculated based on the provided ones.
- *
- * maxcpus will default to the value of cpus if omitted and will be used
- * to compute the missing sockets/cores/threads. cpus will be calculated
- * from the computed parametrs if omitted.
- *
- * In calculation of omitted arch-netural sockets/cores/threads, we prefer
- * sockets over cores over threads before 6.2, while prefer cores over
- * sockets over threads since 6.2 on. The arch-specific dies will directly
- * default to 1 if omitted.
- */
-static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
-{
-    MachineClass *mc = MACHINE_GET_CLASS(ms);
-    unsigned cpus    = config->has_cpus ? config->cpus : 0;
-    unsigned sockets = config->has_sockets ? config->sockets : 0;
-    unsigned dies    = config->has_dies ? config->dies : 1;
-    unsigned cores   = config->has_cores ? config->cores : 0;
-    unsigned threads = config->has_threads ? config->threads : 0;
-    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
-
-    if ((config->has_cpus && config->cpus == 0) ||
-        (config->has_sockets && config->sockets == 0) ||
-        (config->has_dies && config->dies == 0) ||
-        (config->has_cores && config->cores == 0) ||
-        (config->has_threads && config->threads == 0) ||
-        (config->has_maxcpus && config->maxcpus == 0)) {
-        error_setg(errp, "parameters must be equal to or greater than one"
-                   "if provided");
-        return;
-    }
-
-    if (!mc->smp_dies_supported && dies > 1) {
-        error_setg(errp, "dies not supported by this machine's CPU topology");
-        return;
-    }
-
-    maxcpus = maxcpus > 0 ? maxcpus : cpus;
-
-    /* prefer sockets over cores over threads before 6.2 */
-    if (mc->smp_prefer_sockets) {
-        if (sockets == 0) {
-            cores = cores > 0 ? cores : 1;
-            threads = threads > 0 ? threads : 1;
-            sockets = maxcpus / (dies * cores * threads);
-            sockets = sockets > 0 ? sockets : 1;
-        } else if (cores == 0) {
-            threads = threads > 0 ? threads : 1;
-            cores = maxcpus / (sockets * dies * threads);
-            cores = cores > 0 ? cores : 1;
-        } else if (threads == 0) {
-            threads = maxcpus / (sockets * dies * cores);
-            threads = threads > 0 ? threads : 1;
-        }
-    /* prefer cores over sockets over threads since 6.2 */
-    } else {
-        if (cores == 0) {
-            sockets = sockets > 0 ? sockets : 1;
-            threads = threads > 0 ? threads : 1;
-            cores = maxcpus / (sockets * dies * threads);
-            cores = cores > 0 ? cores : 1;
-        } else if (sockets == 0) {
-            threads = threads > 0 ? threads : 1;
-            sockets = maxcpus / (dies * cores * threads);
-            sockets = sockets > 0 ? sockets : 1;
-        } else if (threads == 0) {
-            threads = maxcpus / (sockets * dies * cores);
-            threads = threads > 0 ? threads : 1;
-        }
-    }
-
-    /* use the computed parameters to calculate the omitted cpus */
-    cpus = cpus > 0 ? cpus : sockets * dies * cores * threads;
-    maxcpus = maxcpus > 0 ? maxcpus : cpus;
-
-    if (sockets * dies * cores * threads != maxcpus) {
-        g_autofree char *dies_msg = g_strdup_printf(
-            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
-        error_setg(errp, "Invalid CPU topology: "
-                   "sockets (%u)%s * cores (%u) * threads (%u) "
-                   "!= maxcpus (%u)",
-                   sockets, dies_msg, cores, threads,
-                   maxcpus);
-        return;
-    }
-
-    if (sockets * dies * cores * threads < cpus) {
-        g_autofree char *dies_msg = g_strdup_printf(
-            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
-        error_setg(errp, "Invalid CPU topology: "
-                   "sockets (%u)%s * cores (%u) * threads (%u) < "
-                   "smp_cpus (%u)",
-                   sockets, dies_msg, cores, threads, cpus);
-        return;
-    }
-
-    ms->smp.cpus = cpus;
-    ms->smp.sockets = sockets;
-    ms->smp.dies = dies;
-    ms->smp.cores = cores;
-    ms->smp.threads = threads;
-    ms->smp.max_cpus = maxcpus;
-}
-
   static void machine_get_smp(Object *obj, Visitor *v, const char *name,
                               void *opaque, Error **errp)
   {
diff --git a/hw/core/meson.build b/hw/core/meson.build
index 18f44fb7c2..6d727c7742 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -14,6 +14,7 @@ hwcore_files = files(
   )
   common_ss.add(files('cpu-common.c'))
+common_ss.add(files('machine-smp.c'))
   common_ss.add(when: 'CONFIG_FITLOADER', if_true: files('loader-fit.c'))
   common_ss.add(when: 'CONFIG_GENERIC_LOADER', if_true: 
files('generic-loader.c'))
   common_ss.add(when: ['CONFIG_GUEST_LOADER', fdt], if_true: 
files('guest-loader.c'))
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 12ab0f5968..071eec1e74 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -34,6 +34,7 @@ HotpluggableCPUList 
*machine_query_hotpluggable_cpus(MachineState *machine);
   void machine_set_cpu_numa_node(MachineState *machine,
                                  const CpuInstanceProperties *props,
                                  Error **errp);
+void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp);
   /**
    * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
--
2.19.1


Otherwise

Reviewed-by: Andrew Jones <drjones@redhat.com>


Thanks,
Yanan
.


.
reply via email to
[Prev in Thread] Current Thread [Next in Thread]