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

[Andrew Jones]

On Thu, Nov 06, 2014 at 05:09:35PM +0100, Andrew Jones wrote:
> smp_parse has a couple problems. First, it should use max_cpus,
> not smp_cpus when calculating missing topology information.
> Conversely, if maxcpus is not input, then the topology should
> dictate max_cpus, as the topology may support more than the
> input smp_cpus number. Second, smp_parse shouldn't silently
> adjust the number of threads a user provides, which it currently
> does in order to ensure the topology supports the number
> of cpus provided. smp_parse should rather complain and exit
> when input isn't consistent. This patch fixes those issues and
> attempts to clarify the code a bit too.
> 
> I don't believe we need to consider compatibility with the old
> behaviors. Specifying something like
> 
>   -smp 4,sockets=1,cores=1,threads=1
> 
> is wrong, even though it currently works (the number of threads
> is silently adjusted to 4). And, specifying something like
> 
>   -smp 4,sockets=1,cores=4,threads=1,maxcpus=8
> 
> is also wrong, as there's no way to hotplug the additional 4 cpus
> with this topology. So, any users doing these things should be
> corrected. The new error message this patch provides should help
> them do that.
> 
> Below are some examples with before/after results
> 
> // topology should support up to max_cpus
> < -smp 4,sockets=2,maxcpus=8                    sockets=2 cores=2 threads=1 
> smp_cpus=4 max_cpus=8
> > -smp 4,sockets=2,maxcpus=8                    sockets=2 cores=4 threads=1 
> > smp_cpus=4 max_cpus=8
> 
> // topology supports more than smp_cpus, max_cpus should be higher
> < -smp 4,sockets=4,cores=2                      sockets=4 cores=2 threads=1 
> smp_cpus=4 max_cpus=4
> > -smp 4,sockets=4,cores=2                      sockets=4 cores=2 threads=1 
> > smp_cpus=4 max_cpus=8
> 
> // shouldn't silently adjust threads
> < -smp 4,sockets=1,cores=2,threads=1            sockets=1 cores=2 threads=2 
> smp_cpus=4 max_cpus=4
> > -smp 4,sockets=1,cores=2,threads=1            "maxcpus must be equal to or 
> > greater than smp"
> < -smp 4,sockets=2,cores=2,threads=4,maxcpus=8  sockets=2 cores=2 threads=1 
> smp_cpus=4 max_cpus=8
> > -smp 4,sockets=2,cores=2,threads=4,maxcpus=8  "cpu topology: sockets (2) * 
> > cores (2) * threads (4) != max_cpus (8)"
> 
> Signed-off-by: Andrew Jones <address@hidden>
> ---
>  vl.c | 59 ++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 38 insertions(+), 21 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index f4a6e5e05bce2..23b21a5fbca50 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1270,35 +1270,52 @@ static QemuOptsList qemu_smp_opts = {
>  static void smp_parse(QemuOpts *opts)
>  {
>      if (opts) {
> -
> -        unsigned cpus    = qemu_opt_get_number(opts, "cpus", 0);
>          unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
>          unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
>          unsigned threads = qemu_opt_get_number(opts, "threads", 0);
> +        unsigned cpus;
> +
> +        smp_cpus = qemu_opt_get_number(opts, "cpus", 0);
> +        max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
> +
> +        cpus = max_cpus ? max_cpus : smp_cpus;
>  
>          /* compute missing values, prefer sockets over cores over threads */
> -        if (cpus == 0 || sockets == 0) {
> -            sockets = sockets > 0 ? sockets : 1;
> -            cores = cores > 0 ? cores : 1;
> -            threads = threads > 0 ? threads : 1;
> -            if (cpus == 0) {
> -                cpus = cores * threads * sockets;
> -            }
> -        } else {
> -            if (cores == 0) {
> -                threads = threads > 0 ? threads : 1;
> -                cores = cpus / (sockets * threads);
> -            } else {
> -                threads = cpus / (cores * sockets);
> -            }
> +        if (cpus == 0) {
> +            cpus = sockets ? sockets : 1;
> +            cpus = cpus * cores ? cpus * cores : cpus;
> +            cpus = cpus * threads ? cpus * threads : cpus;
>          }
>  
> -        max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
> +        if (sockets == 0) {
> +            sockets = 1;
> +        }
>  
> -        smp_cpus = cpus;
> -        smp_cores = cores > 0 ? cores : 1;
> -        smp_threads = threads > 0 ? threads : 1;
> +        if (cores == 0) {
> +            threads = threads ? threads : 1;
> +            cores = cpus / (sockets * threads);
> +            cores = cores ? cores : 1;
> +        }
> +
> +        if (threads == 0) {
> +            threads = cpus / (sockets * cores);
> +            threads = threads ? threads : 1;
> +        }
> +
> +        if (max_cpus == 0) {
> +            max_cpus = sockets * cores * threads;
> +        }
>  
> +        if (sockets * cores * threads != max_cpus) {
> +            fprintf(stderr, "cpu topology: "
> +                    "sockets (%u) * cores (%u) * threads (%u) != max_cpus 
> (%u)\n",
> +                    sockets, cores, threads, max_cpus);
> +            exit(1);
> +        }
> +
> +        smp_cpus = smp_cpus ? smp_cpus : max_cpus;
> +        smp_cores = cores;
> +        smp_threads = threads;
>      }
>  
>      if (max_cpus == 0) {
> @@ -1309,11 +1326,11 @@ static void smp_parse(QemuOpts *opts)
>          fprintf(stderr, "Unsupported number of maxcpus\n");
>          exit(1);
>      }
> +
>      if (max_cpus < smp_cpus) {
>          fprintf(stderr, "maxcpus must be equal to or greater than smp\n");
>          exit(1);
>      }
> -
>  }
>  
>  static void realtime_init(void)
> -- 
> 1.9.3
>

Dropping this patch in favor of series I'll post in a few seconds.

drew