[Qemu-devel] [PATCH] vl: rework smp_parse

[Andrew Jones <drjones@redhat.com>]

smp_parse has a couple problems. First, it should use max_cpus,
not smp_cpus when calculating missing topology information.
Conversely, if maxcpus is not input, then the topology should
dictate max_cpus, as the topology may support more than the
input smp_cpus number. Second, smp_parse shouldn't silently
adjust the number of threads a user provides, which it currently
does in order to ensure the topology supports the number
of cpus provided. smp_parse should rather complain and exit
when input isn't consistent. This patch fixes those issues and
attempts to clarify the code a bit too.

I don't believe we need to consider compatibility with the old
behaviors. Specifying something like

  -smp 4,sockets=1,cores=1,threads=1

is wrong, even though it currently works (the number of threads
is silently adjusted to 4). And, specifying something like

  -smp 4,sockets=1,cores=4,threads=1,maxcpus=8

is also wrong, as there's no way to hotplug the additional 4 cpus
with this topology. So, any users doing these things should be
corrected. The new error message this patch provides should help
them do that.

Below are some examples with before/after results

// topology should support up to max_cpus
< -smp 4,sockets=2,maxcpus=8                    sockets=2 cores=2 threads=1 smp_cpus=4 max_cpus=8
> -smp 4,sockets=2,maxcpus=8                    sockets=2 cores=4 threads=1 smp_cpus=4 max_cpus=8

// topology supports more than smp_cpus, max_cpus should be higher
< -smp 4,sockets=4,cores=2                      sockets=4 cores=2 threads=1 smp_cpus=4 max_cpus=4
> -smp 4,sockets=4,cores=2                      sockets=4 cores=2 threads=1 smp_cpus=4 max_cpus=8

// shouldn't silently adjust threads
< -smp 4,sockets=1,cores=2,threads=1            sockets=1 cores=2 threads=2 smp_cpus=4 max_cpus=4
> -smp 4,sockets=1,cores=2,threads=1            "maxcpus must be equal to or greater than smp"
< -smp 4,sockets=2,cores=2,threads=4,maxcpus=8  sockets=2 cores=2 threads=1 smp_cpus=4 max_cpus=8
> -smp 4,sockets=2,cores=2,threads=4,maxcpus=8  "cpu topology: sockets (2) * cores (2) * threads (4) != max_cpus (8)"

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 vl.c | 59 ++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/vl.c b/vl.c
index f4a6e5e05bce2..23b21a5fbca50 100644
--- a/vl.c
+++ b/vl.c
@@ -1270,35 +1270,52 @@ static QemuOptsList qemu_smp_opts = {
 static void smp_parse(QemuOpts *opts)
 {
     if (opts) {
-
-        unsigned cpus    = qemu_opt_get_number(opts, "cpus", 0);
         unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
         unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
         unsigned threads = qemu_opt_get_number(opts, "threads", 0);
+        unsigned cpus;
+
+        smp_cpus = qemu_opt_get_number(opts, "cpus", 0);
+        max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
+
+        cpus = max_cpus ? max_cpus : smp_cpus;
 
         /* compute missing values, prefer sockets over cores over threads */
-        if (cpus == 0 || sockets == 0) {
-            sockets = sockets > 0 ? sockets : 1;
-            cores = cores > 0 ? cores : 1;
-            threads = threads > 0 ? threads : 1;
-            if (cpus == 0) {
-                cpus = cores * threads * sockets;
-            }
-        } else {
-            if (cores == 0) {
-                threads = threads > 0 ? threads : 1;
-                cores = cpus / (sockets * threads);
-            } else {
-                threads = cpus / (cores * sockets);
-            }
+        if (cpus == 0) {
+            cpus = sockets ? sockets : 1;
+            cpus = cpus * cores ? cpus * cores : cpus;
+            cpus = cpus * threads ? cpus * threads : cpus;
         }
 
-        max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
+        if (sockets == 0) {
+            sockets = 1;
+        }
 
-        smp_cpus = cpus;
-        smp_cores = cores > 0 ? cores : 1;
-        smp_threads = threads > 0 ? threads : 1;
+        if (cores == 0) {
+            threads = threads ? threads : 1;
+            cores = cpus / (sockets * threads);
+            cores = cores ? cores : 1;
+        }
+
+        if (threads == 0) {
+            threads = cpus / (sockets * cores);
+            threads = threads ? threads : 1;
+        }
+
+        if (max_cpus == 0) {
+            max_cpus = sockets * cores * threads;
+        }
 
+        if (sockets * cores * threads != max_cpus) {
+            fprintf(stderr, "cpu topology: "
+                    "sockets (%u) * cores (%u) * threads (%u) != max_cpus (%u)\n",
+                    sockets, cores, threads, max_cpus);
+            exit(1);
+        }
+
+        smp_cpus = smp_cpus ? smp_cpus : max_cpus;
+        smp_cores = cores;
+        smp_threads = threads;
     }
 
     if (max_cpus == 0) {
@@ -1309,11 +1326,11 @@ static void smp_parse(QemuOpts *opts)
         fprintf(stderr, "Unsupported number of maxcpus\n");
         exit(1);
     }
+
     if (max_cpus < smp_cpus) {
         fprintf(stderr, "maxcpus must be equal to or greater than smp\n");
         exit(1);
     }
-
 }
 
 static void realtime_init(void)
-- 
1.9.3