Re: [Qemu-arm] [PATCH RFC 02/16] vl: smp: add checks for maxcpus based topologies

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3002 bytes --]

On Fri, Jun 10, 2016 at 07:40:13PM +0200, Andrew Jones wrote:
> smp_parse computes missing smp options. Unfortunately cores and
> threads are computed by dividing smp_cpus, instead of max_cpus.
> This is incorrect because the topology doesn't leave room for
> hotplug. More unfortunately, we can't change it easily, as doing
> so would impact existing command lines. This patch adds a warning
> when the topology doesn't add up, and then checks that the topology
> at least computes when sockets are recalculated. If not, then it
> does fail.
> 
> Adding the new failure is justified by the fact that we don't
> store the number of input sockets, and thus all consumers of
> cpu topology information recalculate it. If they choose to
> (correctly) calculate it based on maxcpus, then we need to
> guard them against building topologies which provide more cpu
> slots than are the maximum allowed cpus.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Hmm.. this makes sense to me.  Except that I've never been clear if
sockets= was supposed to match initial cpus or maxcpus.

> ---
>  vl.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/vl.c b/vl.c
> index 7b96e787922f9..8d482cb1bf020 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1227,6 +1227,7 @@ static void smp_parse(QemuOpts *opts)
>          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);
> +        bool sockets_input = sockets > 0;
>  
>          /* compute missing values, prefer sockets over cores over threads */
>          if (cpus == 0 || sockets == 0) {
> @@ -1269,6 +1270,24 @@ static void smp_parse(QemuOpts *opts)
>              exit(1);
>          }
>  
> +        if (sockets_input && sockets * cores * threads != max_cpus) {
> +            unsigned sockets_rounded = DIV_ROUND_UP(max_cpus, cores * threads);
> +
> +            error_report("warning: cpu topology: "
> +                         "sockets (%u) * cores (%u) * threads (%u) != "
> +                         "maxcpus (%u). Trying sockets=%u.",
> +                         sockets, cores, threads, max_cpus, sockets_rounded);
> +            sockets = sockets_rounded;
> +
> +            if (sockets * cores * threads > max_cpus) {
> +                error_report("cpu topology: "
> +                             "sockets (%u) * cores (%u) * threads (%u) > "
> +                             "maxcpus (%u)",
> +                             sockets, cores, threads, max_cpus);
> +                exit(1);
> +            }
> +        }
> +
>          smp_cpus = cpus;
>          smp_cores = cores;
>          smp_threads = threads;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]