[Qemu-devel] [PATCH] vl: verify if combination of cpus, sockets, cores and threads is sane

[Qemu-devel] [PATCH] vl: verify if combination of cpus, sockets, cores and threads is sane

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 vl.c |   34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/vl.c b/vl.c
index 8d5d874..dc0b41a 100644
--- a/vl.c
+++ b/vl.c
@@ -1385,35 +1385,41 @@ 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);
 
         /* compute missing values, prefer sockets over cores over threads */
-        if (cpus == 0 || sockets == 0) {
+        if (cpus == 0) {
             sockets = sockets > 0 ? sockets : 1;
             cores = cores > 0 ? cores : 1;
             threads = threads > 0 ? threads : 1;
-            if (cpus == 0) {
-                cpus = cores * threads * sockets;
-            }
+            cpus = cores * threads * sockets;
+        } else if (sockets == 0) {
+            cores = cores > 0 ? cores : 1;
+            threads = threads > 0 ? threads : 1;
+            sockets = cpus / (cores * threads);
+        } else if (cores == 0) {
+            threads = threads > 0 ? threads : 1;
+            cores = cpus / (sockets * threads);
         } else {
-            if (cores == 0) {
-                threads = threads > 0 ? threads : 1;
-                cores = cpus / (sockets * threads);
-            } else {
-                threads = cpus / (cores * sockets);
-            }
+            threads = cpus / (sockets * cores);
         }
 
-        max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
+        if (cpus != sockets * cores * threads) {
+            fprintf(stderr, "Illegal CPU layout: %d cpus with %d sockets,"
+                            " %d cores per socket and %d threads per core"
+                            " (cpus != sockets * cores * threads)\n",
+                            cpus, sockets, cores, threads);
+            exit(1);
+        }
 
         smp_cpus = cpus;
-        smp_cores = cores > 0 ? cores : 1;
-        smp_threads = threads > 0 ? threads : 1;
+        smp_cores = cores;
+        smp_threads = threads;
 
+        max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
     }
 
     if (max_cpus == 0) {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] vl: verify if combination of cpus, sockets, cores and threads is sane

[Paolo Bonzini]

Il 21/11/2013 15:37, Peter Lieven ha scritto:
> -        max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
> +        if (cpus != sockets * cores * threads) {
> +            fprintf(stderr, "Illegal CPU layout: %d cpus with %d sockets,"
> +                            " %d cores per socket and %d threads per core"
> +                            " (cpus != sockets * cores * threads)\n",
> +                            cpus, sockets, cores, threads);
> +            exit(1);
> +        }

Should max_cpus be checked instead if non-zero?

I see where you come from, but I think the potential for this patch to
break some working configuration (for some definition of working) is too
high.  Can you split out the fixes to the "fill in the blanks" logic?

Paolo

Re: [Qemu-devel] [PATCH] vl: verify if combination of cpus, sockets, cores and threads is sane

[Peter Lieven]

On 22.11.2013 11:16, Paolo Bonzini wrote:
> Il 21/11/2013 15:37, Peter Lieven ha scritto:
>> -        max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
>> +        if (cpus != sockets * cores * threads) {
>> +            fprintf(stderr, "Illegal CPU layout: %d cpus with %d sockets,"
>> +                            " %d cores per socket and %d threads per core"
>> +                            " (cpus != sockets * cores * threads)\n",
>> +                            cpus, sockets, cores, threads);
>> +            exit(1);
>> +        }
> Should max_cpus be checked instead if non-zero?
>
> I see where you come from, but I think the potential for this patch to
> break some working configuration (for some definition of working) is too
> high.  Can you split out the fixes to the "fill in the blanks" logic?
I can, but the number of sockets is logal to the parse function.

What would you think is it okay to just send a warning about
the illegal config and drop the exit(1).

Peter

Re: [Qemu-devel] [PATCH] vl: verify if combination of cpus, sockets, cores and threads is sane

[Paolo Bonzini]

Il 22/11/2013 12:13, Peter Lieven ha scritto:
>> I see where you come from, but I think the potential for this patch to
>> break some working configuration (for some definition of working) is too
>> high.  Can you split out the fixes to the "fill in the blanks" logic?
> 
> I can, but the number of sockets is logal to the parse function.

Not sure why that matters, just make two patches instead of one.

> What would you think is it okay to just send a warning about
> the illegal config and drop the exit(1).

Yes, that would be okay.

Paolo

Re: [Qemu-devel] [PATCH] vl: verify if combination of cpus, sockets, cores and threads is sane

[Andreas Färber]

Am 22.11.2013 12:20, schrieb Paolo Bonzini:
> Il 22/11/2013 12:13, Peter Lieven ha scritto:
>>> I see where you come from, but I think the potential for this patch to
>>> break some working configuration (for some definition of working) is too
>>> high.  Can you split out the fixes to the "fill in the blanks" logic?
>>
>> I can, but the number of sockets is logal to the parse function.
> 
> Not sure why that matters, just make two patches instead of one.

...and please CC me and possibly Eduardo who refactored it last IIRC.

Andreas

>> What would you think is it okay to just send a warning about
>> the illegal config and drop the exit(1).
> 
> Yes, that would be okay.
> 
> Paolo

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg