On 2021/7/20 0:36, Andrew Jones wrote:
On Mon, Jul 19, 2021 at 11:20:35AM +0800, Yanan Wang wrote:
We are currently using maxcpus to calculate the omitted sockets
but using cpus to calculate the omitted cores/threads. This makes
cmdlines like:
-smp cpus=8,maxcpus=16
-smp cpus=8,cores=4,maxcpus=16
-smp cpus=8,threads=2,maxcpus=16
work fine but the ones like:
-smp cpus=8,sockets=2,maxcpus=16
-smp cpus=8,sockets=2,cores=4,maxcpus=16
-smp cpus=8,sockets=2,threads=2,maxcpus=16
break the invalid cpu topology check.
Since we require for the valid config that the sum of "sockets * cores
* dies * threads" should equal to the maxcpus, we should uniformly use
maxcpus to calculate their omitted values.
Also the if-branch of "cpus == 0 || sockets == 0" was splited into two
branches of "cpus == 0" and "sockets == 0" so that we can clearly read
that we are parsing -smp cmdlines with a preference of cpus over sockets
over cores over threads.
Note: change in this patch won't affect any existing working cmdlines
but improves consistency and allow more incomplete configs to be valid.
We also remove rounding of cores and threads when the math doesn't come
out right, which could possible start reporting a bad config as invalid
which worked before. Or were you able to prove that that can't happen with
your testing?
I also had this concern, but I think that can't happen for sure.
Take the if-branch of "cores == 0" as an example,
Before this patch:
We use cpus to calculate the missing cores and round it up to 1 if the
result is zero, and at last set maxcpus to match cpus if it's omitted.
So the parsing result must have met the two conditions:
1) sockets * cores * threads == maxcpus
2) sockets * cores * threads >= cpus
After this patch:
We start to use maxcpus to calculate the missing cores and also remove
the rounding. For the same config mentioned above, it still works and the
parsing result will also not change, because we will never get a fractional
value of cores (maxcpus is multiple of (sockets * threads) ).
Like the valid config "-smp 8,sockets=16,maxcpus=16", we will get
"cpus=8,sockets=16,cores=1,threads=1,maxcpus=16" before, and
still get the same result after.
Please correct me if I missed something, thanks.
Signed-off-by: Yanan Wang <wangyana...@huawei.com>
---
hw/core/machine.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index ed6712e964..c9f15b15a5 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -768,24 +768,26 @@ static void smp_parse(MachineState *ms, SMPConfiguration
*config, Error **errp)
}
/* compute missing values, prefer sockets over cores over threads */
- if (cpus == 0 || sockets == 0) {
+ maxcpus = maxcpus > 0 ? maxcpus : cpus;
+
+ if (cpus == 0) {
+ sockets = sockets > 0 ? sockets : 1;
cores = cores > 0 ? cores : 1;
threads = threads > 0 ? threads : 1;
- if (cpus == 0) {
- sockets = sockets > 0 ? sockets : 1;
- cpus = sockets * dies * cores * threads;
- } else {
- maxcpus = maxcpus > 0 ? maxcpus : cpus;
- sockets = maxcpus / (dies * cores * threads);
- }
+ cpus = sockets * dies * cores * threads;
+ maxcpus = maxcpus > 0 ? maxcpus : cpus;
+ } else 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 = cpus / (sockets * dies * threads);
- cores = cores > 0 ? cores : 1;
+ cores = maxcpus / (sockets * dies * threads);
} else if (threads == 0) {
- threads = cpus / (sockets * dies * cores);
- threads = threads > 0 ? threads : 1;
- } else if (sockets * dies * cores * threads < cpus) {
+ threads = maxcpus / (sockets * dies * cores);
+ }
+
+ 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: "
@@ -795,8 +797,6 @@ static void smp_parse(MachineState *ms, SMPConfiguration
*config, Error **errp)
return;
}
- maxcpus = maxcpus > 0 ? maxcpus : cpus;
-
if (maxcpus < cpus) {
error_setg(errp, "maxcpus must be equal to or greater than smp");
return;
--
2.19.1
Reviewed-by: Andrew Jones <drjo...@redhat.com>
Thanks,
Yanan
.
