[Qemu-devel] [PATCH v4] vl.c deprecate incorrect CPUs topology

[Qemu-devel] [PATCH v4] vl.c deprecate incorrect CPUs topology

[Igor Mammedov]

-smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
so that total number of logical CPUs [sockets * cores * threads]
would be equal to [maxcpus], however historically we didn't have
such check in QEMU and it is possible to start VM with an invalid
topology.
Deprecate invalid options combination so we can make sure that
the topology VM started with is always correct in the future.
Users with an invalid sockets/cores/threads/maxcpus values should
fix their CLI to make sure that
   [sockets * cores * threads] == [maxcpus]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
  - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@redhat.com>)
v3:
  - more spelling fixes (Andrew Jones <drjones@redhat.com>)
  - place deprecation check after (sockets * cores * threads > max_cpus) check
    (Eduardo Habkost <ehabkost@redhat.com>)
v2:
  - spelling&&co fixes (Andrew Jones <drjones@redhat.com>)
---
 qemu-deprecated.texi | 11 +++++++++++
 vl.c                 |  7 +++++++
 2 files changed, 18 insertions(+)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 87212b6..094b4ca 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -159,6 +159,17 @@ The 'file' driver for drives is no longer appropriate for character or host
 devices and will only accept regular files (S_IFREG). The correct driver
 for these file types is 'host_cdrom' or 'host_device' as appropriate.
 
+@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)
+
+CPU topology properties should describe whole machine topology including
+possible CPUs, but historically it was possible to start QEMU with
+an incorrect topology where
+  sockets * cores * threads >= X && X < maxcpus
+which could lead to an incorrect topology enumeration by the guest.
+Support for invalid topologies will be removed, the user must ensure
+topologies described with -smp include all possible cpus, i.e.
+  sockets * cores * threads == maxcpus
+
 @section QEMU Machine Protocol (QMP) commands
 
 @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
diff --git a/vl.c b/vl.c
index 5ba06ad..7fd700e 100644
--- a/vl.c
+++ b/vl.c
@@ -1246,6 +1246,13 @@ static void smp_parse(QemuOpts *opts)
             exit(1);
         }
 
+        if (sockets * cores * threads != max_cpus) {
+            warn_report("Invalid CPU topology deprecated: "
+                        "sockets (%u) * cores (%u) * threads (%u) "
+                        "!= maxcpus (%u)",
+                        sockets, cores, threads, max_cpus);
+        }
+
         smp_cpus = cpus;
         smp_cores = cores;
         smp_threads = threads;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4] vl.c deprecate incorrect CPUs topology

[Andrew Jones]

On Tue, Aug 28, 2018 at 03:48:13PM +0200, Igor Mammedov wrote:
> -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> so that total number of logical CPUs [sockets * cores * threads]
> would be equal to [maxcpus], however historically we didn't have
> such check in QEMU and it is possible to start VM with an invalid
> topology.
> Deprecate invalid options combination so we can make sure that
> the topology VM started with is always correct in the future.
> Users with an invalid sockets/cores/threads/maxcpus values should
> fix their CLI to make sure that
>    [sockets * cores * threads] == [maxcpus]
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
>   - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@redhat.com>)
> v3:
>   - more spelling fixes (Andrew Jones <drjones@redhat.com>)
>   - place deprecation check after (sockets * cores * threads > max_cpus) check
>     (Eduardo Habkost <ehabkost@redhat.com>)
> v2:
>   - spelling&&co fixes (Andrew Jones <drjones@redhat.com>)
> ---
>  qemu-deprecated.texi | 11 +++++++++++
>  vl.c                 |  7 +++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 87212b6..094b4ca 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -159,6 +159,17 @@ The 'file' driver for drives is no longer appropriate for character or host
>  devices and will only accept regular files (S_IFREG). The correct driver
>  for these file types is 'host_cdrom' or 'host_device' as appropriate.
>  
> +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)
> +
> +CPU topology properties should describe whole machine topology including
> +possible CPUs, but historically it was possible to start QEMU with
> +an incorrect topology where
> +  sockets * cores * threads >= X && X < maxcpus
> +which could lead to an incorrect topology enumeration by the guest.
> +Support for invalid topologies will be removed, the user must ensure
> +topologies described with -smp include all possible cpus, i.e.
> +  sockets * cores * threads == maxcpus
> +
>  @section QEMU Machine Protocol (QMP) commands
>  
>  @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
> diff --git a/vl.c b/vl.c
> index 5ba06ad..7fd700e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1246,6 +1246,13 @@ static void smp_parse(QemuOpts *opts)
>              exit(1);
>          }
>  
> +        if (sockets * cores * threads != max_cpus) {
> +            warn_report("Invalid CPU topology deprecated: "
> +                        "sockets (%u) * cores (%u) * threads (%u) "
> +                        "!= maxcpus (%u)",
> +                        sockets, cores, threads, max_cpus);
> +        }
> +
>          smp_cpus = cpus;
>          smp_cores = cores;
>          smp_threads = threads;
> -- 
> 2.7.4
> 
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

[Qemu-devel] [PATCH] vl:c: make sure that sockets are calculated correctly in '-smp X' case

[Igor Mammedov]

commit
  (5cdc9b76e3 vl.c: Remove dead assignment)
removed sockets calculation when 'sockets' weren't provided on CLI
since there wasn't any users for it back then. Exiting checks
are neither reachable
   } else if (sockets * cores * threads < cpus) {
or nor triggable
   if (sockets * cores * threads > max_cpus)
so we weren't noticing wrong topology since then, since users
recalculate sockets adhoc on their own.

However with deprecation check it becomes noticable, for example
  -smp 2
will start printing warning:
  "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)"
calculating sockets if they weren't specified.

Fix it by returning back sockets calculation if it's omited on CLI.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 vl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 7fd700e..333d638 100644
--- a/vl.c
+++ b/vl.c
@@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
 
         /* 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) {
+                sockets = sockets > 0 ? sockets : 1;
                 cpus = cores * threads * sockets;
+            } else {
+                max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
+                sockets = !sockets ? max_cpus / (cores * threads) : sockets;
             }
         } else if (cores == 0) {
             threads = threads > 0 ? threads : 1;
-- 
2.7.4

Re: [Qemu-devel] [PATCH] vl:c: make sure that sockets are calculated correctly in '-smp X' case

[Eduardo Habkost]

On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote:
> commit
>   (5cdc9b76e3 vl.c: Remove dead assignment)
> removed sockets calculation when 'sockets' weren't provided on CLI
> since there wasn't any users for it back then. Exiting checks
> are neither reachable
>    } else if (sockets * cores * threads < cpus) {
> or nor triggable
>    if (sockets * cores * threads > max_cpus)
> so we weren't noticing wrong topology since then, since users
> recalculate sockets adhoc on their own.
> 
> However with deprecation check it becomes noticable, for example
>   -smp 2
> will start printing warning:
>   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)"
> calculating sockets if they weren't specified.
> 
> Fix it by returning back sockets calculation if it's omited on CLI.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  vl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index 7fd700e..333d638 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
>  
>          /* 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) {
> +                sockets = sockets > 0 ? sockets : 1;
>                  cpus = cores * threads * sockets;
> +            } else {
> +                max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);

We already have a "max_cpus = qemu_opt_get_number(...)" line in
this function[1]:

        /* 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) {
            [...]
        } else if (threads == 0) {
            [...]
        } else if (sockets * cores * threads < cpus) {
            [...]
        }

        max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */

        /* [2] */

        if (max_cpus < cpus) {
            error_report("maxcpus must be equal to or greater than smp");
            exit(1);
        }

Why not move the sockets == 0 check to [2]?


> +                sockets = !sockets ? max_cpus / (cores * threads) : sockets;

The two patches in this thread make QEMU print a warning on a
case that was never documented as invalid/deprecated: maxcpus now
needs to be a multiple of cores*threads.

However, the error message doesn't make it obvious:

  $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
  qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * cores (3) * threads (1) != maxcpus (7)

I think this would make more sense:

  $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
  qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) is not a multiple of cores (3) * threads (1)


>              }
>          } else if (cores == 0) {
>              threads = threads > 0 ? threads : 1;
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] vl:c: make sure that sockets are calculated correctly in '-smp X' case

[Igor Mammedov]

On Wed, 29 Aug 2018 14:33:01 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote:
> > commit
> >   (5cdc9b76e3 vl.c: Remove dead assignment)
> > removed sockets calculation when 'sockets' weren't provided on CLI
> > since there wasn't any users for it back then. Exiting checks
> > are neither reachable
> >    } else if (sockets * cores * threads < cpus) {
> > or nor triggable
> >    if (sockets * cores * threads > max_cpus)
> > so we weren't noticing wrong topology since then, since users
> > recalculate sockets adhoc on their own.
> > 
> > However with deprecation check it becomes noticable, for example
> >   -smp 2
> > will start printing warning:
> >   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)"
> > calculating sockets if they weren't specified.
> > 
> > Fix it by returning back sockets calculation if it's omited on CLI.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  vl.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 7fd700e..333d638 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
> >  
> >          /* 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) {
> > +                sockets = sockets > 0 ? sockets : 1;
> >                  cpus = cores * threads * sockets;
> > +            } else {
> > +                max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);  
> 
> We already have a "max_cpus = qemu_opt_get_number(...)" line in
> this function[1]:
> 
>         /* 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) {
>             [...]
>         } else if (threads == 0) {
>             [...]
>         } else if (sockets * cores * threads < cpus) {
>             [...]
>         }
> 
>         max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */
> 
>         /* [2] */
> 
>         if (max_cpus < cpus) {
>             error_report("maxcpus must be equal to or greater than smp");
>             exit(1);
>         }
> 
> Why not move the sockets == 0 check to [2]?
it won't work, since the rest 'else if' depends on sockets being non 0
which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2
equivalent conditions. It's more code but it might be easier to read

diff --git a/vl.c b/vl.c
index 7fd700e..6320bdc 100644
--- a/vl.c
+++ b/vl.c
@@ -1208,14 +1208,18 @@ static void smp_parse(QemuOpts *opts)
         unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
         unsigned threads = qemu_opt_get_number(opts, "threads", 0);
 
+        max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
         /* 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;
+            max_cpus = max_cpus > 0 ? max_cpus : cpus;
+        } else if (sockets == 0) {
+            cores = cores > 0 ? cores : 1;
+            threads = threads > 0 ? threads : 1;
+            sockets = max_cpus / (cores * threads);
         } else if (cores == 0) {
             threads = threads > 0 ? threads : 1;
             cores = cpus / (sockets * threads);
@@ -1231,8 +1235,6 @@ static void smp_parse(QemuOpts *opts)
             exit(1);
         }
 
-        max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
-
         if (max_cpus < cpus) {
             error_report("maxcpus must be equal to or greater than smp");
             exit(1);


> 
> 
> > +                sockets = !sockets ? max_cpus / (cores * threads) : sockets;  
> 
> The two patches in this thread make QEMU print a warning on a
> case that was never documented as invalid/deprecated: maxcpus now
> needs to be a multiple of cores*threads.
> 
> However, the error message doesn't make it obvious:
> 
>   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
>   qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * cores (3) * threads (1) != maxcpus (7)
to me it look pretty obvious, fix one side of equation to make sure that VM
starts in the future.

> I think this would make more sense:
> 
>   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
>   qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) is not a multiple of cores (3) * threads (1)
I think it will add not necessary warning/code, but if you really
think we need it, I can add it to 'vl.c deprecate incorrect CPUs
topology' on respin.

> 
> 
> >              }
> >          } else if (cores == 0) {
> >              threads = threads > 0 ? threads : 1;
> > -- 
> > 2.7.4
> >   
> 

Re: [Qemu-devel] [PATCH] vl:c: make sure that sockets are calculated correctly in '-smp X' case

[Andrew Jones]

On Thu, Aug 30, 2018 at 09:58:51AM +0200, Igor Mammedov wrote:
> On Wed, 29 Aug 2018 14:33:01 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote:
> > > commit
> > >   (5cdc9b76e3 vl.c: Remove dead assignment)
> > > removed sockets calculation when 'sockets' weren't provided on CLI
> > > since there wasn't any users for it back then. Exiting checks
> > > are neither reachable
> > >    } else if (sockets * cores * threads < cpus) {
> > > or nor triggable
> > >    if (sockets * cores * threads > max_cpus)
> > > so we weren't noticing wrong topology since then, since users
> > > recalculate sockets adhoc on their own.
> > > 
> > > However with deprecation check it becomes noticable, for example
> > >   -smp 2
> > > will start printing warning:
> > >   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)"
> > > calculating sockets if they weren't specified.
> > > 
> > > Fix it by returning back sockets calculation if it's omited on CLI.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  vl.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index 7fd700e..333d638 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
> > >  
> > >          /* 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) {
> > > +                sockets = sockets > 0 ? sockets : 1;
> > >                  cpus = cores * threads * sockets;
> > > +            } else {
> > > +                max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);  
> > 
> > We already have a "max_cpus = qemu_opt_get_number(...)" line in
> > this function[1]:
> > 
> >         /* 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) {
> >             [...]
> >         } else if (threads == 0) {
> >             [...]
> >         } else if (sockets * cores * threads < cpus) {
> >             [...]
> >         }
> > 
> >         max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */
> > 
> >         /* [2] */
> > 
> >         if (max_cpus < cpus) {
> >             error_report("maxcpus must be equal to or greater than smp");
> >             exit(1);
> >         }
> > 
> > Why not move the sockets == 0 check to [2]?
> it won't work, since the rest 'else if' depends on sockets being non 0
> which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2
> equivalent conditions. It's more code but it might be easier to read
> 
> diff --git a/vl.c b/vl.c
> index 7fd700e..6320bdc 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1208,14 +1208,18 @@ static void smp_parse(QemuOpts *opts)
>          unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
>          unsigned threads = qemu_opt_get_number(opts, "threads", 0);
>  
> +        max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
>          /* 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;
> +            max_cpus = max_cpus > 0 ? max_cpus : cpus;
> +        } else if (sockets == 0) {
> +            cores = cores > 0 ? cores : 1;
> +            threads = threads > 0 ? threads : 1;
> +            sockets = max_cpus / (cores * threads);
>          } else if (cores == 0) {
>              threads = threads > 0 ? threads : 1;
>              cores = cpus / (sockets * threads);
> @@ -1231,8 +1235,6 @@ static void smp_parse(QemuOpts *opts)
>              exit(1);
>          }
>  
> -        max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> -
>          if (max_cpus < cpus) {
>              error_report("maxcpus must be equal to or greater than smp");
>              exit(1);
>

I once made a cleanup similar to the above, but with more more line
removal. It doesn't look like I ever posted it, and I don't recall
how much I tested it, but here it is

diff --git a/vl.c b/vl.c
index 5ba06adf787b..ad4e389fde48 100644
--- a/vl.c
+++ b/vl.c
@@ -1208,30 +1208,22 @@ static void smp_parse(QemuOpts *opts)
         unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
         unsigned threads = qemu_opt_get_number(opts, "threads", 0);
 
+        max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
+
         /* compute missing values, prefer sockets over cores over threads */
-        if (cpus == 0 || sockets == 0) {
-            sockets = sockets > 0 ? sockets : 1;
+        if (sockets == 0) {
             cores = cores > 0 ? cores : 1;
             threads = threads > 0 ? threads : 1;
-            if (cpus == 0) {
-                cpus = cores * threads * sockets;
-            }
+            sockets = max_cpus > 0 ? max_cpus / (cores * threads) : 1;
         } else if (cores == 0) {
             threads = threads > 0 ? threads : 1;
-            cores = cpus / (sockets * threads);
-            cores = cores > 0 ? cores : 1;
+            cores = max_cpus > 0 ? max_cpus / (sockets * threads) : 1;
         } else if (threads == 0) {
-            threads = cpus / (cores * sockets);
-            threads = threads > 0 ? threads : 1;
-        } else if (sockets * cores * threads < cpus) {
-            error_report("cpu topology: "
-                         "sockets (%u) * cores (%u) * threads (%u) < "
-                         "smp_cpus (%u)",
-                         sockets, cores, threads, cpus);
-            exit(1);
+            threads = max_cpus > 0 ? max_cpus / (sockets * cores) : 1;
         }
 
-        max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
+        max_cpus = max_cpus ?: sockets * cores * threads;
+        cpus = cpus ?: max_cpus;
 
         if (max_cpus < cpus) {
             error_report("maxcpus must be equal to or greater than smp");


Thanks,
drew

> 
> > 
> > 
> > > +                sockets = !sockets ? max_cpus / (cores * threads) : sockets;  
> > 
> > The two patches in this thread make QEMU print a warning on a
> > case that was never documented as invalid/deprecated: maxcpus now
> > needs to be a multiple of cores*threads.
> > 
> > However, the error message doesn't make it obvious:
> > 
> >   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
> >   qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * cores (3) * threads (1) != maxcpus (7)
> to me it look pretty obvious, fix one side of equation to make sure that VM
> starts in the future.
> 
> > I think this would make more sense:
> > 
> >   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
> >   qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) is not a multiple of cores (3) * threads (1)
> I think it will add not necessary warning/code, but if you really
> think we need it, I can add it to 'vl.c deprecate incorrect CPUs
> topology' on respin.
> 
> > 
> > 
> > >              }
> > >          } else if (cores == 0) {
> > >              threads = threads > 0 ? threads : 1;
> > > -- 
> > > 2.7.4
> > >   
> > 
> 
> 

Re: [Qemu-devel] [PATCH] vl:c: make sure that sockets are calculated correctly in '-smp X' case

[Igor Mammedov]

On Thu, 30 Aug 2018 11:08:53 +0200
Andrew Jones <drjones@redhat.com> wrote:

> On Thu, Aug 30, 2018 at 09:58:51AM +0200, Igor Mammedov wrote:
> > On Wed, 29 Aug 2018 14:33:01 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote:  
> > > > commit
> > > >   (5cdc9b76e3 vl.c: Remove dead assignment)
> > > > removed sockets calculation when 'sockets' weren't provided on CLI
> > > > since there wasn't any users for it back then. Exiting checks
> > > > are neither reachable
> > > >    } else if (sockets * cores * threads < cpus) {
> > > > or nor triggable
> > > >    if (sockets * cores * threads > max_cpus)
> > > > so we weren't noticing wrong topology since then, since users
> > > > recalculate sockets adhoc on their own.
> > > > 
> > > > However with deprecation check it becomes noticable, for example
> > > >   -smp 2
> > > > will start printing warning:
> > > >   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)"
> > > > calculating sockets if they weren't specified.
> > > > 
> > > > Fix it by returning back sockets calculation if it's omited on CLI.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  vl.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/vl.c b/vl.c
> > > > index 7fd700e..333d638 100644
> > > > --- a/vl.c
> > > > +++ b/vl.c
> > > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
> > > >  
> > > >          /* 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) {
> > > > +                sockets = sockets > 0 ? sockets : 1;
> > > >                  cpus = cores * threads * sockets;
> > > > +            } else {
> > > > +                max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);    
> > > 
> > > We already have a "max_cpus = qemu_opt_get_number(...)" line in
> > > this function[1]:
> > > 
> > >         /* 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) {
> > >             [...]
> > >         } else if (threads == 0) {
> > >             [...]
> > >         } else if (sockets * cores * threads < cpus) {
> > >             [...]
> > >         }
> > > 
> > >         max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */
> > > 
> > >         /* [2] */
> > > 
> > >         if (max_cpus < cpus) {
> > >             error_report("maxcpus must be equal to or greater than smp");
> > >             exit(1);
> > >         }
> > > 
> > > Why not move the sockets == 0 check to [2]?  
> > it won't work, since the rest 'else if' depends on sockets being non 0
> > which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2
> > equivalent conditions. It's more code but it might be easier to read
> > 
> > diff --git a/vl.c b/vl.c
> > index 7fd700e..6320bdc 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1208,14 +1208,18 @@ static void smp_parse(QemuOpts *opts)
> >          unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
> >          unsigned threads = qemu_opt_get_number(opts, "threads", 0);
> >  
> > +        max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> >          /* 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;
> > +            max_cpus = max_cpus > 0 ? max_cpus : cpus;
> > +        } else if (sockets == 0) {
> > +            cores = cores > 0 ? cores : 1;
> > +            threads = threads > 0 ? threads : 1;
> > +            sockets = max_cpus / (cores * threads);
> >          } else if (cores == 0) {
> >              threads = threads > 0 ? threads : 1;
> >              cores = cpus / (sockets * threads);
> > @@ -1231,8 +1235,6 @@ static void smp_parse(QemuOpts *opts)
> >              exit(1);
> >          }
> >  
> > -        max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> > -
> >          if (max_cpus < cpus) {
> >              error_report("maxcpus must be equal to or greater than smp");
> >              exit(1);
> >  
> 
> I once made a cleanup similar to the above, but with more more line
> removal. It doesn't look like I ever posted it, and I don't recall
> how much I tested it, but here it is
my main motivation touching this code is to get in deprecation notice
with as little disruption as possible, and when it expires it should be
easier to cleanup as we won't have to worry about invalid then variants.


> diff --git a/vl.c b/vl.c
> index 5ba06adf787b..ad4e389fde48 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1208,30 +1208,22 @@ static void smp_parse(QemuOpts *opts)
>          unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
>          unsigned threads = qemu_opt_get_number(opts, "threads", 0);
>  
> +        max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> +
>          /* compute missing values, prefer sockets over cores over threads */
> -        if (cpus == 0 || sockets == 0) {
> -            sockets = sockets > 0 ? sockets : 1;
> +        if (sockets == 0) {
>              cores = cores > 0 ? cores : 1;
>              threads = threads > 0 ? threads : 1;
> -            if (cpus == 0) {
> -                cpus = cores * threads * sockets;
> -            }
> +            sockets = max_cpus > 0 ? max_cpus / (cores * threads) : 1;
>          } else if (cores == 0) {
>              threads = threads > 0 ? threads : 1;
> -            cores = cpus / (sockets * threads);
> -            cores = cores > 0 ? cores : 1;
> +            cores = max_cpus > 0 ? max_cpus / (sockets * threads) : 1;
Is it subject to the same issue why 'cores = cores > 0 ? cores : 1;' where added?
Also s/cpus/maxcpus/ is not equivalent, considering cores are not discarded like sockets

>          } else if (threads == 0) {
> -            threads = cpus / (cores * sockets);
> -            threads = threads > 0 ? threads : 1;
> -        } else if (sockets * cores * threads < cpus) {
> -            error_report("cpu topology: "
> -                         "sockets (%u) * cores (%u) * threads (%u) < "
> -                         "smp_cpus (%u)",
> -                         sockets, cores, threads, cpus);
> -            exit(1);
> +            threads = max_cpus > 0 ? max_cpus / (sockets * cores) : 1;
ditto

>          }
>  
> -        max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> +        max_cpus = max_cpus ?: sockets * cores * threads;
> +        cpus = cpus ?: max_cpus;
>  
>          if (max_cpus < cpus) {
>              error_report("maxcpus must be equal to or greater than smp");
> 
> 
> Thanks,
> drew
> 
> >   
> > > 
> > >   
> > > > +                sockets = !sockets ? max_cpus / (cores * threads) : sockets;    
> > > 
> > > The two patches in this thread make QEMU print a warning on a
> > > case that was never documented as invalid/deprecated: maxcpus now
> > > needs to be a multiple of cores*threads.
> > > 
> > > However, the error message doesn't make it obvious:
> > > 
> > >   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
> > >   qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * cores (3) * threads (1) != maxcpus (7)  
> > to me it look pretty obvious, fix one side of equation to make sure that VM
> > starts in the future.
> >   
> > > I think this would make more sense:
> > > 
> > >   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
> > >   qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) is not a multiple of cores (3) * threads (1)  
> > I think it will add not necessary warning/code, but if you really
> > think we need it, I can add it to 'vl.c deprecate incorrect CPUs
> > topology' on respin.
> >   
> > > 
> > >   
> > > >              }
> > > >          } else if (cores == 0) {
> > > >              threads = threads > 0 ? threads : 1;
> > > > -- 
> > > > 2.7.4
> > > >     
> > >   
> > 
> >   

Re: [Qemu-devel] [PATCH] vl:c: make sure that sockets are calculated correctly in '-smp X' case

[Eduardo Habkost]

On Thu, Aug 30, 2018 at 09:58:51AM +0200, Igor Mammedov wrote:
> On Wed, 29 Aug 2018 14:33:01 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote:
> > > commit
> > >   (5cdc9b76e3 vl.c: Remove dead assignment)
> > > removed sockets calculation when 'sockets' weren't provided on CLI
> > > since there wasn't any users for it back then. Exiting checks
> > > are neither reachable
> > >    } else if (sockets * cores * threads < cpus) {
> > > or nor triggable
> > >    if (sockets * cores * threads > max_cpus)
> > > so we weren't noticing wrong topology since then, since users
> > > recalculate sockets adhoc on their own.
> > > 
> > > However with deprecation check it becomes noticable, for example
> > >   -smp 2
> > > will start printing warning:
> > >   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)"
> > > calculating sockets if they weren't specified.
> > > 
> > > Fix it by returning back sockets calculation if it's omited on CLI.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  vl.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index 7fd700e..333d638 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
> > >  
> > >          /* 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) {
> > > +                sockets = sockets > 0 ? sockets : 1;
> > >                  cpus = cores * threads * sockets;
> > > +            } else {
> > > +                max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);  
> > 
> > We already have a "max_cpus = qemu_opt_get_number(...)" line in
> > this function[1]:
> > 
> >         /* 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) {
> >             [...]
> >         } else if (threads == 0) {
> >             [...]
> >         } else if (sockets * cores * threads < cpus) {
> >             [...]
> >         }
> > 
> >         max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */
> > 
> >         /* [2] */
> > 
> >         if (max_cpus < cpus) {
> >             error_report("maxcpus must be equal to or greater than smp");
> >             exit(1);
> >         }
> > 
> > Why not move the sockets == 0 check to [2]?
> it won't work, since the rest 'else if' depends on sockets being non 0
> which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2
> equivalent conditions. It's more code but it might be easier to read

I like your original patch better.  I wasn't thinking about the
"cpus == 0 || sockets == 0" check, but about this line:

   sockets = !sockets ? max_cpus / (cores * threads) : sockets;

Just moving this line to [2] would be enough, wouldn't it?

> 
[...]
> > 
> > 
> > > +                sockets = !sockets ? max_cpus / (cores * threads) : sockets;  
> > 
> > The two patches in this thread make QEMU print a warning on a
> > case that was never documented as invalid/deprecated: maxcpus now
> > needs to be a multiple of cores*threads.
> > 
> > However, the error message doesn't make it obvious:
> > 
> >   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
> >   qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * cores (3) * threads (1) != maxcpus (7)
> to me it look pretty obvious, fix one side of equation to make sure that VM
> starts in the future.
> 
> > I think this would make more sense:
> > 
> >   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
> >   qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) is not a multiple of cores (3) * threads (1)
> I think it will add not necessary warning/code, but if you really
> think we need it, I can add it to 'vl.c deprecate incorrect CPUs
> topology' on respin.

My main worry is about insufficient documentation: we need to
clarify that the s*c*t == maxcpus rule applies even if "sockets"
is omitted in the command-line, and that this indirectly requires
that maxcpus be a multiple of c*6.

That said, I won't mind if you keep the existing message in this
patch for code simplicity, as long as the documentation is
updated.

> 
> > 
> > 
> > >              }
> > >          } else if (cores == 0) {
> > >              threads = threads > 0 ? threads : 1;
> > > -- 
> > > 2.7.4
> > >   
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] vl:c: make sure that sockets are calculated correctly in '-smp X' case

[Igor Mammedov]

On Thu, 30 Aug 2018 11:08:00 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Aug 30, 2018 at 09:58:51AM +0200, Igor Mammedov wrote:
> > On Wed, 29 Aug 2018 14:33:01 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote:  
> > > > commit
> > > >   (5cdc9b76e3 vl.c: Remove dead assignment)
> > > > removed sockets calculation when 'sockets' weren't provided on CLI
> > > > since there wasn't any users for it back then. Exiting checks
> > > > are neither reachable
> > > >    } else if (sockets * cores * threads < cpus) {
> > > > or nor triggable
> > > >    if (sockets * cores * threads > max_cpus)
> > > > so we weren't noticing wrong topology since then, since users
> > > > recalculate sockets adhoc on their own.
> > > > 
> > > > However with deprecation check it becomes noticable, for example
> > > >   -smp 2
> > > > will start printing warning:
> > > >   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)"
> > > > calculating sockets if they weren't specified.
> > > > 
> > > > Fix it by returning back sockets calculation if it's omited on CLI.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  vl.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/vl.c b/vl.c
> > > > index 7fd700e..333d638 100644
> > > > --- a/vl.c
> > > > +++ b/vl.c
> > > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
> > > >  
> > > >          /* 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) {
> > > > +                sockets = sockets > 0 ? sockets : 1;
> > > >                  cpus = cores * threads * sockets;
> > > > +            } else {
> > > > +                max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);    
> > > 
> > > We already have a "max_cpus = qemu_opt_get_number(...)" line in
> > > this function[1]:
> > > 
> > >         /* 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) {
> > >             [...]
> > >         } else if (threads == 0) {
> > >             [...]
> > >         } else if (sockets * cores * threads < cpus) {
> > >             [...]
> > >         }
> > > 
> > >         max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */
> > > 
> > >         /* [2] */
> > > 
> > >         if (max_cpus < cpus) {
> > >             error_report("maxcpus must be equal to or greater than smp");
> > >             exit(1);
> > >         }
> > > 
> > > Why not move the sockets == 0 check to [2]?  
> > it won't work, since the rest 'else if' depends on sockets being non 0
> > which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2
> > equivalent conditions. It's more code but it might be easier to read  
> 
> I like your original patch better.  I wasn't thinking about the
> "cpus == 0 || sockets == 0" check, but about this line:
> 
>    sockets = !sockets ? max_cpus / (cores * threads) : sockets;
> 
> Just moving this line to [2] would be enough, wouldn't it?
it would be, but to me it looks more confusing, since one goes
into a branch on condition sockets == 0 but then does nothing with
socket. 
I'd rather have a duplicate qemu_opt_get_number(opts, "maxcpus", cpus)
at the time when we have all date to set it instead of deferring it
to the later time.

Once deprecation expires we will be able to clean it all up as Drew
suggested.


> [...]
> > > 
> > >   
> > > > +                sockets = !sockets ? max_cpus / (cores * threads) : sockets;    
> > > 
> > > The two patches in this thread make QEMU print a warning on a
> > > case that was never documented as invalid/deprecated: maxcpus now
> > > needs to be a multiple of cores*threads.
> > > 
> > > However, the error message doesn't make it obvious:
> > > 
> > >   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
> > >   qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * cores (3) * threads (1) != maxcpus (7)  
> > to me it look pretty obvious, fix one side of equation to make sure that VM
> > starts in the future.
> >   
> > > I think this would make more sense:
> > > 
> > >   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
> > >   qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) is not a multiple of cores (3) * threads (1)  
> > I think it will add not necessary warning/code, but if you really
> > think we need it, I can add it to 'vl.c deprecate incorrect CPUs
> > topology' on respin.  
> 
> My main worry is about insufficient documentation: we need to
> clarify that the s*c*t == maxcpus rule applies even if "sockets"
> is omitted in the command-line, and that this indirectly requires
> that maxcpus be a multiple of c*6.
It's not limited to c*t but to any combination, how about adding following:

"
Note: it's assumed that maxcpus value must be multiple of the topology           
options present on command line to avoid creating an invalid topology.           
If maxcpus isn't be multiple of present topology options then the condition      
(sockets * cores * threads == maxcpus) can't be satisfied and it will            
trigger deprecation warning which later will be converted to a error.            
If you get deprecation warning it's recommended to explicitly specify            
a correct topology to make warning go away and ensure that it will               
continue working in the future. 
"

> 
> That said, I won't mind if you keep the existing message in this
> patch for code simplicity, as long as the documentation is
> updated.
> 
> >   
> > > 
> > >   
> > > >              }
> > > >          } else if (cores == 0) {
> > > >              threads = threads > 0 ? threads : 1;
> > > > -- 
> > > > 2.7.4
> > > >     
> > >   
> >   
>