diff mbox series

[1/2] hw/core/machine: Split out smp_parse as an inline API

Message ID 20211010103954.20644-2-wangyanan55@huawei.com
State New
Headers show
Series hw/core/machine: An an unit test for smp_parse | expand

Commit Message

wangyanan (Y) Oct. 10, 2021, 10:39 a.m. UTC
Functionally smp_parse() is only called once and in one place
i.e. machine_set_smp, the possible second place where it'll be
called should be some unit tests if any.

Actually we are going to introduce an unit test for the parser.
For necessary isolation of the tested code, split smp_parse out
into a separate header as an inline API.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 MAINTAINERS           |   1 +
 hw/core/machine.c     | 160 +-----------------------------------
 include/hw/core/smp.h | 185 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 187 insertions(+), 159 deletions(-)
 create mode 100644 include/hw/core/smp.h

Comments

Markus Armbruster Oct. 11, 2021, 5:26 a.m. UTC | #1
Yanan Wang <wangyanan55@huawei.com> writes:

> Functionally smp_parse() is only called once and in one place
> i.e. machine_set_smp, the possible second place where it'll be
> called should be some unit tests if any.
>
> Actually we are going to introduce an unit test for the parser.
> For necessary isolation of the tested code, split smp_parse out
> into a separate header as an inline API.

Why inline?

> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
wangyanan (Y) Oct. 11, 2021, 7:54 a.m. UTC | #2
Hi Markus,

On 2021/10/11 13:26, Markus Armbruster wrote:
> Yanan Wang <wangyanan55@huawei.com> writes:
>
>> Functionally smp_parse() is only called once and in one place
>> i.e. machine_set_smp, the possible second place where it'll be
>> called should be some unit tests if any.
>>
>> Actually we are going to introduce an unit test for the parser.
>> For necessary isolation of the tested code, split smp_parse out
>> into a separate header as an inline API.
> Why inline?
The motivation of the splitting is to isolate the tested smp_parse
from the other unrelated code in machine.c, so that we can solve
the build dependency problem for the unit test.

I once tried to split smp_parse out into a source file in [1] for the
test, but it looks more concise and convenient to make it as an
inline function in a header compared to [1]. Given that we only call
it in one place, it may not be harmful to keep it an inline.

Anyway, I not sure the method in this patch is most appropriate
and compliant. If it's just wrong I can change back to [1]. :)

[1] 
https://lore.kernel.org/qemu-devel/20210910073025.16480-16-wangyanan55@huawei.com/#t

Thanks,
Yanan
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> .
Markus Armbruster Oct. 12, 2021, 2:36 p.m. UTC | #3
"wangyanan (Y)" <wangyanan55@huawei.com> writes:

> Hi Markus,
>
> On 2021/10/11 13:26, Markus Armbruster wrote:
>> Yanan Wang <wangyanan55@huawei.com> writes:
>>
>>> Functionally smp_parse() is only called once and in one place
>>> i.e. machine_set_smp, the possible second place where it'll be
>>> called should be some unit tests if any.
>>>
>>> Actually we are going to introduce an unit test for the parser.
>>> For necessary isolation of the tested code, split smp_parse out
>>> into a separate header as an inline API.
>> Why inline?
> The motivation of the splitting is to isolate the tested smp_parse
> from the other unrelated code in machine.c, so that we can solve
> the build dependency problem for the unit test.
>
> I once tried to split smp_parse out into a source file in [1] for the
> test, but it looks more concise and convenient to make it as an
> inline function in a header compared to [1]. Given that we only call
> it in one place, it may not be harmful to keep it an inline.
>
> Anyway, I not sure the method in this patch is most appropriate
> and compliant. If it's just wrong I can change back to [1]. :)
>
> [1]
> https://lore.kernel.org/qemu-devel/20210910073025.16480-16-wangyanan55@huawei.com/#t

I'd prefer to keep it in .c, but I'm not the maintainer.
wangyanan (Y) Oct. 13, 2021, 1:37 a.m. UTC | #4
On 2021/10/12 22:36, Markus Armbruster wrote:
> "wangyanan (Y)" <wangyanan55@huawei.com> writes:
>
>> Hi Markus,
>>
>> On 2021/10/11 13:26, Markus Armbruster wrote:
>>> Yanan Wang <wangyanan55@huawei.com> writes:
>>>
>>>> Functionally smp_parse() is only called once and in one place
>>>> i.e. machine_set_smp, the possible second place where it'll be
>>>> called should be some unit tests if any.
>>>>
>>>> Actually we are going to introduce an unit test for the parser.
>>>> For necessary isolation of the tested code, split smp_parse out
>>>> into a separate header as an inline API.
>>> Why inline?
>> The motivation of the splitting is to isolate the tested smp_parse
>> from the other unrelated code in machine.c, so that we can solve
>> the build dependency problem for the unit test.
>>
>> I once tried to split smp_parse out into a source file in [1] for the
>> test, but it looks more concise and convenient to make it as an
>> inline function in a header compared to [1]. Given that we only call
>> it in one place, it may not be harmful to keep it an inline.
>>
>> Anyway, I not sure the method in this patch is most appropriate
>> and compliant. If it's just wrong I can change back to [1]. :)
>>
>> [1]
>> https://lore.kernel.org/qemu-devel/20210910073025.16480-16-wangyanan55@huawei.com/#t
> I'd prefer to keep it in .c, but I'm not the maintainer.
>
Ok, I will move it into a separate .c file in v2, which seems to meet
the standards more.

Thanks,
Yanan
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 50435b8d2f..dc9091c1d7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1629,6 +1629,7 @@  F: qapi/machine.json
 F: qapi/machine-target.json
 F: include/hw/boards.h
 F: include/hw/core/cpu.h
+F: include/hw/core/smp.h
 F: include/hw/cpu/cluster.h
 F: include/sysemu/numa.h
 T: git https://gitlab.com/ehabkost/qemu.git machine-next
diff --git a/hw/core/machine.c b/hw/core/machine.c
index b8d95eec32..3018230033 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -17,6 +17,7 @@ 
 #include "qemu/units.h"
 #include "hw/boards.h"
 #include "hw/loader.h"
+#include "hw/core/smp.h"
 #include "qapi/error.h"
 #include "qapi/qapi-visit-common.h"
 #include "qapi/qapi-visit-machine.h"
@@ -749,165 +750,6 @@  void machine_set_cpu_numa_node(MachineState *machine,
     }
 }
 
-/*
- * Report information of a machine's supported CPU topology hierarchy.
- * Topology members will be ordered from the largest to the smallest
- * in the string.
- */
-static char *cpu_hierarchy_to_string(MachineState *ms)
-{
-    MachineClass *mc = MACHINE_GET_CLASS(ms);
-    GString *s = g_string_new(NULL);
-
-    g_string_append_printf(s, "sockets (%u)", ms->smp.sockets);
-
-    if (mc->smp_props.dies_supported) {
-        g_string_append_printf(s, " * dies (%u)", ms->smp.dies);
-    }
-
-    g_string_append_printf(s, " * cores (%u)", ms->smp.cores);
-    g_string_append_printf(s, " * threads (%u)", ms->smp.threads);
-
-    return g_string_free(s, false);
-}
-
-/*
- * smp_parse - Generic function used to parse the given SMP configuration
- *
- * Any missing parameter in "cpus/maxcpus/sockets/cores/threads" will be
- * automatically computed based on the provided ones.
- *
- * In the calculation of omitted sockets/cores/threads: we prefer sockets
- * over cores over threads before 6.2, while preferring cores over sockets
- * over threads since 6.2.
- *
- * In the calculation of cpus/maxcpus: When both maxcpus and cpus are omitted,
- * maxcpus will be computed from the given parameters and cpus will be set
- * equal to maxcpus. When only one of maxcpus and cpus is given then the
- * omitted one will be set to its given counterpart's value. Both maxcpus and
- * cpus may be specified, but maxcpus must be equal to or greater than cpus.
- *
- * For compatibility, apart from the parameters that will be computed, newly
- * introduced topology members which are likely to be target specific should
- * be directly set as 1 if they are omitted (e.g. dies for PC since 4.1).
- */
-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 : 0;
-    unsigned cores   = config->has_cores ? config->cores : 0;
-    unsigned threads = config->has_threads ? config->threads : 0;
-    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
-
-    /*
-     * Specified CPU topology parameters must be greater than zero,
-     * explicit configuration like "cpus=0" is not allowed.
-     */
-    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)) {
-        warn_report("Deprecated CPU topology (considered invalid): "
-                    "CPU topology parameters must be greater than zero");
-    }
-
-    /*
-     * If not supported by the machine, a topology parameter must be
-     * omitted or specified equal to 1.
-     */
-    if (!mc->smp_props.dies_supported && dies > 1) {
-        error_setg(errp, "dies not supported by this machine's CPU topology");
-        return;
-    }
-
-    dies = dies > 0 ? dies : 1;
-
-    /* compute missing values based on the provided ones */
-    if (cpus == 0 && maxcpus == 0) {
-        sockets = sockets > 0 ? sockets : 1;
-        cores = cores > 0 ? cores : 1;
-        threads = threads > 0 ? threads : 1;
-    } else {
-        maxcpus = maxcpus > 0 ? maxcpus : cpus;
-
-        if (mc->smp_props.prefer_sockets) {
-            /* prefer sockets over cores before 6.2 */
-            if (sockets == 0) {
-                cores = cores > 0 ? cores : 1;
-                threads = threads > 0 ? threads : 1;
-                sockets = maxcpus / (dies * cores * threads);
-            } else if (cores == 0) {
-                threads = threads > 0 ? threads : 1;
-                cores = maxcpus / (sockets * dies * threads);
-            }
-        } else {
-            /* prefer cores over sockets since 6.2 */
-            if (cores == 0) {
-                sockets = sockets > 0 ? sockets : 1;
-                threads = threads > 0 ? threads : 1;
-                cores = maxcpus / (sockets * dies * threads);
-            } else if (sockets == 0) {
-                threads = threads > 0 ? threads : 1;
-                sockets = maxcpus / (dies * cores * threads);
-            }
-        }
-
-        /* try to calculate omitted threads at last */
-        if (threads == 0) {
-            threads = maxcpus / (sockets * dies * cores);
-        }
-    }
-
-    maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
-    cpus = cpus > 0 ? cpus : maxcpus;
-
-    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;
-
-    /* sanity-check of the computed topology */
-    if (sockets * dies * cores * threads != maxcpus) {
-        g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
-        error_setg(errp, "Invalid CPU topology: "
-                   "product of the hierarchy must match maxcpus: "
-                   "%s != maxcpus (%u)",
-                   topo_msg, maxcpus);
-        return;
-    }
-
-    if (maxcpus < cpus) {
-        g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
-        error_setg(errp, "Invalid CPU topology: "
-                   "maxcpus must be equal to or greater than smp: "
-                   "%s == maxcpus (%u) < smp_cpus (%u)",
-                   topo_msg, maxcpus, cpus);
-        return;
-    }
-
-    if (ms->smp.cpus < mc->min_cpus) {
-        error_setg(errp, "Invalid SMP CPUs %d. The min CPUs "
-                   "supported by machine '%s' is %d",
-                   ms->smp.cpus,
-                   mc->name, mc->min_cpus);
-        return;
-    }
-
-    if (ms->smp.max_cpus > mc->max_cpus) {
-        error_setg(errp, "Invalid SMP CPUs %d. The max CPUs "
-                   "supported by machine '%s' is %d",
-                   ms->smp.max_cpus,
-                   mc->name, mc->max_cpus);
-        return;
-    }
-}
-
 static void machine_get_smp(Object *obj, Visitor *v, const char *name,
                             void *opaque, Error **errp)
 {
diff --git a/include/hw/core/smp.h b/include/hw/core/smp.h
new file mode 100644
index 0000000000..fe892a5193
--- /dev/null
+++ b/include/hw/core/smp.h
@@ -0,0 +1,185 @@ 
+/*
+ * QEMU Machine core (related to SMP)
+ *
+ * Copyright (c) 2021 Huawei Technologies Co., Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License,
+ * or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef MACHINE_CORE_SMP_H
+#define MACHINE_CORE_SMP_H
+
+#include "hw/boards.h"
+#include "qapi/error.h"
+
+/*
+ * Report information of a machine's supported CPU topology hierarchy.
+ * Topology members will be ordered from the largest to the smallest
+ * in the string.
+ */
+static inline char *cpu_hierarchy_to_string(MachineState *ms)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    GString *s = g_string_new(NULL);
+
+    g_string_append_printf(s, "sockets (%u)", ms->smp.sockets);
+
+    if (mc->smp_props.dies_supported) {
+        g_string_append_printf(s, " * dies (%u)", ms->smp.dies);
+    }
+
+    g_string_append_printf(s, " * cores (%u)", ms->smp.cores);
+    g_string_append_printf(s, " * threads (%u)", ms->smp.threads);
+
+    return g_string_free(s, false);
+}
+
+/*
+ * smp_parse - Generic function used to parse the given SMP configuration
+ *
+ * Any missing parameter in "cpus/maxcpus/sockets/cores/threads" will be
+ * automatically computed based on the provided ones.
+ *
+ * In the calculation of omitted sockets/cores/threads: we prefer sockets
+ * over cores over threads before 6.2, while preferring cores over sockets
+ * over threads since 6.2.
+ *
+ * In the calculation of cpus/maxcpus: When both maxcpus and cpus are omitted,
+ * maxcpus will be computed from the given parameters and cpus will be set
+ * equal to maxcpus. When only one of maxcpus and cpus is given then the
+ * omitted one will be set to its given counterpart's value. Both maxcpus and
+ * cpus may be specified, but maxcpus must be equal to or greater than cpus.
+ *
+ * For compatibility, apart from the parameters that will be computed, newly
+ * introduced topology members which are likely to be target specific should
+ * be directly set as 1 if they are omitted (e.g. dies for PC since 4.1).
+ */
+static inline 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 : 0;
+    unsigned cores   = config->has_cores ? config->cores : 0;
+    unsigned threads = config->has_threads ? config->threads : 0;
+    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
+
+    /*
+     * Specified CPU topology parameters must be greater than zero,
+     * explicit configuration like "cpus=0" is not allowed.
+     */
+    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)) {
+        warn_report("Deprecated CPU topology (considered invalid): "
+                    "CPU topology parameters must be greater than zero");
+    }
+
+    /*
+     * If not supported by the machine, a topology parameter must be
+     * omitted or specified equal to 1.
+     */
+    if (!mc->smp_props.dies_supported && dies > 1) {
+        error_setg(errp, "dies not supported by this machine's CPU topology");
+        return;
+    }
+
+    dies = dies > 0 ? dies : 1;
+
+    /* compute missing values based on the provided ones */
+    if (cpus == 0 && maxcpus == 0) {
+        sockets = sockets > 0 ? sockets : 1;
+        cores = cores > 0 ? cores : 1;
+        threads = threads > 0 ? threads : 1;
+    } else {
+        maxcpus = maxcpus > 0 ? maxcpus : cpus;
+
+        if (mc->smp_props.prefer_sockets) {
+            /* prefer sockets over cores before 6.2 */
+            if (sockets == 0) {
+                cores = cores > 0 ? cores : 1;
+                threads = threads > 0 ? threads : 1;
+                sockets = maxcpus / (dies * cores * threads);
+            } else if (cores == 0) {
+                threads = threads > 0 ? threads : 1;
+                cores = maxcpus / (sockets * dies * threads);
+            }
+        } else {
+            /* prefer cores over sockets since 6.2 */
+            if (cores == 0) {
+                sockets = sockets > 0 ? sockets : 1;
+                threads = threads > 0 ? threads : 1;
+                cores = maxcpus / (sockets * dies * threads);
+            } else if (sockets == 0) {
+                threads = threads > 0 ? threads : 1;
+                sockets = maxcpus / (dies * cores * threads);
+            }
+        }
+
+        /* try to calculate omitted threads at last */
+        if (threads == 0) {
+            threads = maxcpus / (sockets * dies * cores);
+        }
+    }
+
+    maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
+    cpus = cpus > 0 ? cpus : maxcpus;
+
+    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;
+
+    /* sanity-check of the computed topology */
+    if (sockets * dies * cores * threads != maxcpus) {
+        g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
+        error_setg(errp, "Invalid CPU topology: "
+                   "product of the hierarchy must match maxcpus: "
+                   "%s != maxcpus (%u)",
+                   topo_msg, maxcpus);
+        return;
+    }
+
+    if (maxcpus < cpus) {
+        g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
+        error_setg(errp, "Invalid CPU topology: "
+                   "maxcpus must be equal to or greater than smp: "
+                   "%s == maxcpus (%u) < smp_cpus (%u)",
+                   topo_msg, maxcpus, cpus);
+        return;
+    }
+
+    if (ms->smp.cpus < mc->min_cpus) {
+        error_setg(errp, "Invalid SMP CPUs %d. The min CPUs "
+                   "supported by machine '%s' is %d",
+                   ms->smp.cpus,
+                   mc->name, mc->min_cpus);
+        return;
+    }
+
+    if (ms->smp.max_cpus > mc->max_cpus) {
+        error_setg(errp, "Invalid SMP CPUs %d. The max CPUs "
+                   "supported by machine '%s' is %d",
+                   ms->smp.max_cpus,
+                   mc->name, mc->max_cpus);
+        return;
+    }
+}
+
+#endif