diff mbox

[2/3] vl: sanity check cpu topology

Message ID 20141111143709.GA14394@hawk.usersys.redhat.com
State New
Headers show

Commit Message

Andrew Jones Nov. 11, 2014, 2:37 p.m. UTC 
On Tue, Nov 11, 2014 at 10:41:00AM -0200, Eduardo Habkost wrote:
> On Fri, Nov 07, 2014 at 05:04:39PM +0100, Andrew Jones wrote:
> > smp_parse allows partial or complete cpu topology to be given.
> > In either case there may be inconsistencies in the input which
> > are currently not sounding any alarms. In some cases the input
> > is even being silently corrected. We shouldn't do this. Add
> > warnings when input isn't adding up right, and even abort when
> > the complete cpu topology has been input, but isn't correct.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> So, we are fixing bugs and changing behavior on two different cases
> here:
> 
> 1) when all options are provided and they aren't enough for smp_cpus;
> 2) when one option was missing, but the existing calculation was
>    incorrect because of division truncation.

  3) when threads were provided, but incorrect, we silently changed
     it. I thought you wanted to fix this one right now too.

> 
> I don't think we need to keep compatibility on (1) because the user is
> obviously providing an invalid configuration. That's why I suggested we
> implemented it in 2.2. And it is safer because we won't be silently
> changing behavior: QEMU is going to abort and the mistake will be easily
> detected.
> 
> But (2) is fixing a QEMU bug, not user error. The user may be unaware of
> the bug, and will get a silent ABI change once upgrading to a newer
> QEMU.

We can keep it rounding down, unless the result is zero, as the current
code does. How about keeping the new warning? Nah, let's drop it. Who
actually cares about warnings anyway...

> 
> I suggest fixing only (1) by now and keeping the behavior for (2) on
> QEMU 2.2. Something like:
> 
>     if (sockets == 0) {
>         /* keep existing code for sockets == 0 */
>     } else if (cores == 0) {
>         /* keep existing code for cores == 0 */
>     } else if (threads == 0) {
>         /* keep existing code for threads == 0 */

This doesn't exist with current code. Adding an 'if (threads == 0)'
case is fix (3).

>     } else {
>         /* new code: */
>         if (sockets * cores * threads < cpus) {
>             fprintf(stderr, "cpu topology: error: "
>                     "sockets (%u) * cores (%u) * threads (%u) < smp_cpus (%u)\n",
>                     sockets, cores, threads, cpus);
>             exit(1);
>         }
>     }
> 
> 

Below is a v2 I can post if it looks good to you.

From: Andrew Jones <drjones@redhat.com>
Date: Fri, 7 Nov 2014 15:45:07 +0100
Subject: [PATCH v2] vl: sanity check cpu topology

smp_parse allows partial or complete cpu topology to be given.
In either case there may be inconsistencies in the input which
are currently not sounding any alarms. In some cases the input
is even being silently corrected. Stop silently adjusting input
and abort when the complete cpu topology has been input, but
isn't correct.

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

Comments

Eduardo Habkost Nov. 11, 2014, 6:31 p.m. UTC | #1 
On Tue, Nov 11, 2014 at 03:37:11PM +0100, Andrew Jones wrote:
[...]
> Below is a v2 I can post if it looks good to you.
> 
> From: Andrew Jones <drjones@redhat.com>
> Date: Fri, 7 Nov 2014 15:45:07 +0100
> Subject: [PATCH v2] vl: sanity check cpu topology
> 
> smp_parse allows partial or complete cpu topology to be given.
> In either case there may be inconsistencies in the input which
> are currently not sounding any alarms. In some cases the input
> is even being silently corrected. Stop silently adjusting input
> and abort when the complete cpu topology has been input, but
> isn't correct.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>

After applying this patch:

  $ ./install/bin/qemu-system-x86_64 -smp 12
  cpu topology: error: sockets (1) * cores (1) * threads (1) < smp_cpus (12)

That is why I wanted to address the most obvious (and less risky) issues first
(aborting only if all options were explicitly set), and touch automatic
calculation later.
Andrew Jones Nov. 12, 2014, 9:14 a.m. UTC | #2 
On Tue, Nov 11, 2014 at 04:31:24PM -0200, Eduardo Habkost wrote:
> On Tue, Nov 11, 2014 at 03:37:11PM +0100, Andrew Jones wrote:
> [...]
> > Below is a v2 I can post if it looks good to you.
> > 
> > From: Andrew Jones <drjones@redhat.com>
> > Date: Fri, 7 Nov 2014 15:45:07 +0100
> > Subject: [PATCH v2] vl: sanity check cpu topology
> > 
> > smp_parse allows partial or complete cpu topology to be given.
> > In either case there may be inconsistencies in the input which
> > are currently not sounding any alarms. In some cases the input
> > is even being silently corrected. Stop silently adjusting input
> > and abort when the complete cpu topology has been input, but
> > isn't correct.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> After applying this patch:
> 
>   $ ./install/bin/qemu-system-x86_64 -smp 12
>   cpu topology: error: sockets (1) * cores (1) * threads (1) < smp_cpus (12)
> 
> That is why I wanted to address the most obvious (and less risky) issues first
> (aborting only if all options were explicitly set), and touch automatic
> calculation later.
>

Oh right. I fixed that once, but then lost the change when trying to
produce these half fixes.

drew
diff mbox

Patch

diff --git a/vl.c b/vl.c
index 9d9855092ab4a..e686fd21e266f 100644
--- a/vl.c
+++ b/vl.c
@@ -1288,16 +1288,39 @@  static void smp_parse(QemuOpts *opts)
             if (cores == 0) {
                 threads = threads > 0 ? threads : 1;
                 cores = cpus / (sockets * threads);
-            } else {
-                threads = cpus / (cores * sockets);
+                if (cpus % (sockets * threads)) {
+                    /* The calculation resulted in a fractional number, so we
+                     * need to adjust it. The below adjustment is wrong, it
+                     * should be '+= 1', but we need to keep it this way for
+                     * compatibility.
+                     */
+                    cores = cores > 0 ? cores : 1;
+                }
+            } else if (threads == 0) {
+                threads = cpus / (sockets * cores);
+                if (cpus % (sockets * cores)) {
+                    /* The calculation resulted in a fractional number, so we
+                     * need to adjust it. The below adjustment is wrong, it
+                     * should be '+= 1', but we need to keep it this way for
+                     * compatibility.
+                     */
+                    threads = threads > 0 ? threads : 1;
+                }
             }
         }
 
+        if (sockets * cores * threads < cpus) {
+            fprintf(stderr, "cpu topology: error: "
+                    "sockets (%u) * cores (%u) * threads (%u) < smp_cpus (%u)\n",
+                    sockets, cores, threads, cpus);
+            exit(1);
+        }
+
         max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
 
         smp_cpus = cpus;
-        smp_cores = cores > 0 ? cores : 1;
-        smp_threads = threads > 0 ? threads : 1;
+        smp_cores = cores;
+        smp_threads = threads;
 
     }