On 2021/7/20 0:53, Andrew Jones wrote:
On Mon, Jul 19, 2021 at 11:20:37AM +0800, Yanan Wang wrote:
We totally have two requirements for a valid SMP configuration:
s/totally//
the sum of "sockets * dies * cores * threads" must represent all
the product
the possible cpus, i.e., max_cpus, and must include the initial
present cpus, i.e., smp_cpus.
We only need to ensure "sockets * dies * cores * threads == maxcpus"
at first and then ensure "sockets * dies * cores * threads >= cpus".
Or, "maxcpus >= cpus"
With a reasonable order of the sanity-check, we can simplify the
error reporting code.
Signed-off-by: Yanan Wang <wangyana...@huawei.com>
---
hw/core/machine.c | 25 ++++++++++---------------
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 668f0a1553..8b4d07d3fc 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -788,21 +788,6 @@ static void smp_parse(MachineState *ms, SMPConfiguration
*config, Error **errp)
cpus = cpus > 0 ? cpus : sockets * dies * cores * threads;
maxcpus = maxcpus > 0 ? maxcpus : cpus;
- if (sockets * dies * cores * threads < cpus) {
- g_autofree char *dies_msg = g_strdup_printf(
- mc->smp_dies_supported ? " * dies (%u)" : "", dies);
- error_setg(errp, "cpu topology: "
- "sockets (%u)%s * cores (%u) * threads (%u) < "
- "smp_cpus (%u)",
- sockets, dies_msg, cores, threads, cpus);
- return;
- }
-
- if (maxcpus < cpus) {
- error_setg(errp, "maxcpus must be equal to or greater than smp");
- return;
- }
This may be redundant when determining a valid config, but by checking it
separately we can provide a more useful error message.
Yes, this message is more useful. Can we also report the exact values of the
parameters within this error message ? How about the following:
if (sockets * cores * threads != maxcpus) {
error_setg("product of the topology must be equal to maxcpus"
"sockets (%u) * cores (%u) * threads (%u)"
"!= maxcpus (%u)",
sockets, cores, threads, maxcpus);
return;
}
if (maxcpus < cpus) {
error_setg("maxcpus must be equal to or greater than smp:"
"sockets (%u) * cores (%u) * threads (%u)"
"== maxcpus (%u) < smp_cpus (%u)",
sockets, cores, threads, maxcpus, cpus);
return;
}
Thanks,
Yanan
.
-
if (sockets * dies * cores * threads != maxcpus) {
g_autofree char *dies_msg = g_strdup_printf(
mc->smp_dies_supported ? " * dies (%u)" : "", dies);
@@ -814,6 +799,16 @@ static void smp_parse(MachineState *ms, SMPConfiguration
*config, Error **errp)
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;
--
2.19.1
I'm not sure we need this patch.
Thanks,
drew
.
