[Qemu-devel] [PATCH 0/3] vl: smp_parse sanity checks

[Qemu-devel] [PATCH 0/3] vl: smp_parse sanity checks

[Andrew Jones]

See individual patches.

Andrew Jones (3):
  vl: fix max_cpus check
  vl: sanity check cpu topology
  vl: warn on topology <-> maxcpus mismatch

 vl.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/3] vl: fix max_cpus check

[Andrew Jones]

We should confirm max_cpus, which is >= smp_cpus, is
<= the machine's true max_cpus, not just smp_cpus.

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

diff --git a/vl.c b/vl.c
index f4a6e5e05bce2..9d9855092ab4a 100644
--- a/vl.c
+++ b/vl.c
@@ -3903,9 +3903,9 @@ int main(int argc, char **argv, char **envp)
     smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
 
     machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */
-    if (smp_cpus > machine_class->max_cpus) {
+    if (max_cpus > machine_class->max_cpus) {
         fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
-                "supported by machine `%s' (%d)\n", smp_cpus,
+                "supported by machine `%s' (%d)\n", max_cpus,
                 machine_class->name, machine_class->max_cpus);
         exit(1);
     }
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/3] vl: fix max_cpus check

[Eduardo Habkost]

On Fri, Nov 07, 2014 at 05:04:38PM +0100, Andrew Jones wrote:
> We should confirm max_cpus, which is >= smp_cpus, is
> <= the machine's true max_cpus, not just smp_cpus.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I was sure I had fixed this before. But it looks like my patches were silently
ignored, and I forgot to ping about them.

See:

  Date: Thu, 5 Jun 2014 19:18:42 -0300
  From: Eduardo Habkost <ehabkost@redhat.com>
  Message-ID: <20140605221842.GI15000@otherpad.lan.raisama.net>
  Subject: [Qemu-devel] [PATCH] vl.c: Check -smp option ranges before setting int globals

and:

  Date: Wed, 14 May 2014 16:18:42 -0300
  From: Eduardo Habkost <ehabkost@redhat.com>
  Message-Id: <1400095122-24736-1-git-send-email-ehabkost@redhat.com>
  Subject: [Qemu-devel] [PATCH v2 RESEND] vl.c: Unify MAX_CPUMASK_BITS and machine->max_cpus checks



> ---
>  vl.c | 6 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index f4a6e5e05bce2..9d9855092ab4a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3903,9 +3903,9 @@ int main(int argc, char **argv, char **envp)
>      smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
>  
>      machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */
> -    if (smp_cpus > machine_class->max_cpus) {
> +    if (max_cpus > machine_class->max_cpus) {
>          fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
> -                "supported by machine `%s' (%d)\n", smp_cpus,
> +                "supported by machine `%s' (%d)\n", max_cpus,
>                  machine_class->name, machine_class->max_cpus);
>          exit(1);
>      }
> -- 
> 1.9.3
> 

-- 
Eduardo

[Qemu-devel] [PATCH 2/3] vl: sanity check cpu topology

[Andrew Jones]

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>
---
 vl.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index 9d9855092ab4a..c62fe29aa8075 100644
--- a/vl.c
+++ b/vl.c
@@ -1288,16 +1288,35 @@ 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)) {
+                    fprintf(stderr, "cpu topology: warning: "
+                            "Calculation results in fractional cores number. "
+                            "Adjusting.\n");
+                    cores += 1;
+                }
+            } else if (threads == 0) {
+                threads = cpus / (sockets * cores);
+                if (cpus % (sockets * cores)) {
+                    fprintf(stderr, "cpu topology: warning: "
+                            "Calculation results in fractional threads number. "
+                            "Adjusting.\n");
+                    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;
 
     }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/3] vl: sanity check cpu topology

[Eduardo Habkost]

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.

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.

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 */
    } 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);
        }
    }


> ---
>  vl.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 9d9855092ab4a..c62fe29aa8075 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1288,16 +1288,35 @@ 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)) {
> +                    fprintf(stderr, "cpu topology: warning: "
> +                            "Calculation results in fractional cores number. "
> +                            "Adjusting.\n");
> +                    cores += 1;
> +                }
> +            } else if (threads == 0) {
> +                threads = cpus / (sockets * cores);
> +                if (cpus % (sockets * cores)) {
> +                    fprintf(stderr, "cpu topology: warning: "
> +                            "Calculation results in fractional threads number. "
> +                            "Adjusting.\n");
> +                    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;
>  
>      }
>  
> -- 
> 1.9.3
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/3] vl: sanity check cpu topology

[Andrew Jones]

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(-)

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;
 
     }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/3] vl: sanity check cpu topology

[Eduardo Habkost]

On Tue, Nov 11, 2014 at 03:37:11PM +0100, Andrew Jones wrote:
> 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.

True. When I described (1) I was seeing (3) as a subset of it, as
threads is silently changed only if core and sockets were also provided
and cores*sockets*threads != smp_cpus.

So, yes, I want to fix (1) and (3) on 2.2. I am not sure about (2).

> 
> > 
> > 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...

A warning would be interesting for (2), maybe. I just would like to have
it addressed in a separate patch, so we can fix (3) and (1) in 2.2
before we decide about (2).

> 
> > 
> > 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).

Yes, I was talking about the existing code that's inside the "else"
branch (and would change threads silently even if it was already set),
that would fix (3) too.

> 
> >     } 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.

I don't think that's accurate: you are not aborting only when the
complete CPU topology has been input, but also when QEMU automatic
calculation is incorrect due to truncation.

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

You are fixing a (subset of) 2 while fixing 1 and 3, again. Can we
address (2) in a separate patch?

Untested patch that fixes only (1) & (3) below:

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
diff --git a/vl.c b/vl.c
index f4a6e5e..536c7d3 100644
--- a/vl.c
+++ b/vl.c
@@ -1284,13 +1284,17 @@ static void smp_parse(QemuOpts *opts)
             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);
-            }
+        } else if (cores == 0) {
+            threads = threads > 0 ? threads : 1;
+            cores = cpus / (sockets * threads);
+        } else if (threads ==0) {
+            threads = cpus / (cores * sockets);
+        } else 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);


>              }
>          }
>  
> +        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;
>  
>      }
>  
> -- 
> 1.9.3
> 


-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/3] vl: sanity check cpu topology

[Andrew Jones]

On Tue, Nov 11, 2014 at 01:48:00PM -0200, Eduardo Habkost wrote:
> On Tue, Nov 11, 2014 at 03:37:11PM +0100, Andrew Jones wrote:
> > 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.
> 
> True. When I described (1) I was seeing (3) as a subset of it, as
> threads is silently changed only if core and sockets were also provided
> and cores*sockets*threads != smp_cpus.
> 
> So, yes, I want to fix (1) and (3) on 2.2. I am not sure about (2).
> 
> > 
> > > 
> > > 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...
> 
> A warning would be interesting for (2), maybe. I just would like to have
> it addressed in a separate patch, so we can fix (3) and (1) in 2.2
> before we decide about (2).
> 
> > 
> > > 
> > > 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).
> 
> Yes, I was talking about the existing code that's inside the "else"
> branch (and would change threads silently even if it was already set),
> that would fix (3) too.
> 
> > 
> > >     } 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.
> 
> I don't think that's accurate: you are not aborting only when the
> complete CPU topology has been input, but also when QEMU automatic
> calculation is incorrect due to truncation.

OK, let's modify the commit message, but keep the patch. The
truncation problem we'd abort for is still directly due to user
input. We wouldn't get a fractional cores or threads number if the
input was appropriate.

Anyway, if we don't need to worry about sockets*cores*threads < cpus
for the truncation case, then why worry about it for the full input
case? Well, other than informing the user that the input she's been
using all this time with the silent threads adjustment was wrong,
which is why I suspect you want it. However, that argument should
hold for the truncation case too, i.e. informing a user that e.g.
5,sockets=2,threads=1 has always been wrong.

drew

Re: [Qemu-devel] [PATCH 2/3] vl: sanity check cpu topology

[Eduardo Habkost]

On Tue, Nov 11, 2014 at 06:04:18PM +0100, Andrew Jones wrote:
> On Tue, Nov 11, 2014 at 01:48:00PM -0200, Eduardo Habkost wrote:
> > On Tue, Nov 11, 2014 at 03:37:11PM +0100, Andrew Jones wrote:
> > > 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.
> > 
> > True. When I described (1) I was seeing (3) as a subset of it, as
> > threads is silently changed only if core and sockets were also provided
> > and cores*sockets*threads != smp_cpus.
> > 
> > So, yes, I want to fix (1) and (3) on 2.2. I am not sure about (2).
> > 
> > > 
> > > > 
> > > > 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...
> > 
> > A warning would be interesting for (2), maybe. I just would like to have
> > it addressed in a separate patch, so we can fix (3) and (1) in 2.2
> > before we decide about (2).
> > 
> > > 
> > > > 
> > > > 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).
> > 
> > Yes, I was talking about the existing code that's inside the "else"
> > branch (and would change threads silently even if it was already set),
> > that would fix (3) too.
> > 
> > > 
> > > >     } 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.
> > 
> > I don't think that's accurate: you are not aborting only when the
> > complete CPU topology has been input, but also when QEMU automatic
> > calculation is incorrect due to truncation.
> 
> OK, let's modify the commit message, but keep the patch. The
> truncation problem we'd abort for is still directly due to user
> input. We wouldn't get a fractional cores or threads number if the
> input was appropriate.

You are probably right that it is also due to an user mistake. But I
still believe we should address both in different patches because then
we can take different decisions about 2.2 inclusion for each of them. I
would like to be able to revert or drop the fix for (2) without losing
the fix for (1) and (3).

What about this: I will prepare a new series, but replacing patch 2/3
with a sequence consisting of my patch (addressing (1) & (3)) + your v2
patch (addressing (2)). Is that OK?

> 
> Anyway, if we don't need to worry about sockets*cores*threads < cpus
> for the truncation case, then why worry about it for the full input
> case? Well, other than informing the user that the input she's been
> using all this time with the silent threads adjustment was wrong,
> which is why I suspect you want it. However, that argument should
> hold for the truncation case too, i.e. informing a user that e.g.
> 5,sockets=2,threads=1 has always been wrong.

I believe we do need to worry about both. The difference is that in one
case we may be aborting because of a QEMU bug, and in another case due
to an obvious user mistake.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/3] vl: sanity check cpu topology

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/3] vl: sanity check cpu topology

[Andrew Jones]

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

[Qemu-devel] [PATCH 3/3] vl: warn on topology <-> maxcpus mismatch

[Andrew Jones]

Start guiding users towards making sure their topology supports the
maximum number of cpus they wish to support. A future patch series
will enforce this for new machine types.

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

diff --git a/vl.c b/vl.c
index c62fe29aa8075..72ffbffd858c5 100644
--- a/vl.c
+++ b/vl.c
@@ -1313,6 +1313,13 @@ static void smp_parse(QemuOpts *opts)
         }
 
         max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
+        max_cpus = max_cpus ?: cpus;
+
+        if (sockets * cores * threads != max_cpus) {
+            fprintf(stderr, "cpu topology: warning: "
+                    "sockets (%u) * cores (%u) * threads (%u) != max_cpus (%u)\n",
+                    sockets, cores, threads, max_cpus);
+        }
 
         smp_cpus = cpus;
         smp_cores = cores;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 0/3] vl: smp_parse sanity checks

[Paolo Bonzini]

On 07/11/2014 17:04, Andrew Jones wrote:
> See individual patches.
> 
> Andrew Jones (3):
>   vl: fix max_cpus check
>   vl: sanity check cpu topology
>   vl: warn on topology <-> maxcpus mismatch
> 
>  vl.c | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 

Looks good, but Eduardo should be the authority on this stuff.

Adding Peter Lieven too, because he looked at bad -smp configurations
too in the past.

Paolo

Re: [Qemu-devel] [PATCH 0/3] vl: smp_parse sanity checks

[Eduardo Habkost]

On Tue, Nov 11, 2014 at 12:15:26PM +0100, Paolo Bonzini wrote:
> 
> 
> On 07/11/2014 17:04, Andrew Jones wrote:
> > See individual patches.
> > 
> > Andrew Jones (3):
> >   vl: fix max_cpus check
> >   vl: sanity check cpu topology
> >   vl: warn on topology <-> maxcpus mismatch
> > 
> >  vl.c | 38 ++++++++++++++++++++++++++++++++------
> >  1 file changed, 32 insertions(+), 6 deletions(-)
> > 
> 
> Looks good, but Eduardo should be the authority on this stuff.

Patch 1/3 is an obvious fix that should go to 2.2.

Patch 2/3 is fixing two diferent bugs. One fix is making QEMU abort when
I think it really should, but the second fix may cause a silent ABI
change.

Patch 3/3 is just an warning, but it is changing the meaning of the
"sockets" option (from "sockets with online CPUs" to "all available
sockets (even the empty ones)"), so I think it makes sense only after we
actually change its meaning in 2.3.

-- 
Eduardo