Re: [PATCH for-6.2 v3 03/11] machine: Set the value of cpus to match maxcpus if it's omitted

[Andrew Jones]

On Wed, Jul 28, 2021 at 11:48:40AM +0800, Yanan Wang wrote:
> Currently we directly calculate the omitted cpus based on the given
> incomplete collection of parameters. This makes some cmdlines like:
>   -smp maxcpus=16
>   -smp sockets=2,maxcpus=16
>   -smp sockets=2,dies=2,maxcpus=16
>   -smp sockets=2,cores=4,maxcpus=16
> not work. We should probably set the value of cpus to match maxcpus
> if it's omitted, which will make above configs start to work.
> 
> So the calculation logic of cpus/maxcpus after this patch will be:
> When both maxcpus and cpus are omitted, maxcpus will be calculated
> from the given parameters and cpus will be set equal to maxcpus.
> When only one of maxcpus and cpus is given then the omitted one
> will be set to its counterpart's value. Both maxcpus and cpus may
> be specified, but maxcpus must be equal to or greater than cpus.
> 
> Note: change in this patch won't affect any existing working cmdlines
> but allows more incomplete configs to be valid.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 29 ++++++++++++++++-------------
>  hw/i386/pc.c      | 29 ++++++++++++++++-------------
>  qemu-options.hx   | 11 ++++++++---
>  3 files changed, 40 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 69979c93dd..958e6e7107 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -755,25 +755,28 @@ static void smp_parse(MachineState *ms, 
> SMPConfiguration *config, Error **errp)
>      }
>  
>      /* compute missing values, prefer sockets over cores over threads */
> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -
> -    if (cpus == 0) {
> +    if (cpus == 0 && maxcpus == 0) {
>          sockets = sockets > 0 ? sockets : 1;
>          cores = cores > 0 ? cores : 1;
>          threads = threads > 0 ? threads : 1;
> -        cpus = sockets * cores * threads;
> +    } else {
>          maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -    } else if (sockets == 0) {
> -        cores = cores > 0 ? cores : 1;
> -        threads = threads > 0 ? threads : 1;
> -        sockets = maxcpus / (cores * threads);
> -    } else if (cores == 0) {
> -        threads = threads > 0 ? threads : 1;
> -        cores = maxcpus / (sockets * threads);
> -    } else if (threads == 0) {
> -        threads = maxcpus / (sockets * cores);
> +
> +        if (sockets == 0) {
> +            cores = cores > 0 ? cores : 1;
> +            threads = threads > 0 ? threads : 1;
> +            sockets = maxcpus / (cores * threads);
> +        } else if (cores == 0) {
> +            threads = threads > 0 ? threads : 1;
> +            cores = maxcpus / (sockets * threads);
> +        } else if (threads == 0) {
> +            threads = maxcpus / (sockets * cores);
> +        }
>      }
>  
> +    maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
> +    cpus = cpus > 0 ? cpus : maxcpus;
> +
>      if (sockets * cores * threads < cpus) {
>          error_setg(errp, "cpu topology: "
>                     "sockets (%u) * cores (%u) * threads (%u) < "
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a9ff9ef52c..9ad7ae5254 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -725,25 +725,28 @@ static void pc_smp_parse(MachineState *ms, 
> SMPConfiguration *config, Error **err
>      dies = dies > 0 ? dies : 1;
>  
>      /* compute missing values, prefer sockets over cores over threads */
> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -
> -    if (cpus == 0) {
> +    if (cpus == 0 && maxcpus == 0) {
>          sockets = sockets > 0 ? sockets : 1;
>          cores = cores > 0 ? cores : 1;
>          threads = threads > 0 ? threads : 1;
> -        cpus = sockets * dies * cores * threads;
> +    } else {
>          maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -    } else if (sockets == 0) {
> -        cores = cores > 0 ? cores : 1;
> -        threads = threads > 0 ? threads : 1;
> -        sockets = maxcpus / (dies * cores * threads);
> -    } else if (cores == 0) {
> -        threads = threads > 0 ? threads : 1;
> -        cores = maxcpus / (sockets * dies * threads);
> -    } else if (threads == 0) {
> -        threads = maxcpus / (sockets * dies * cores);
> +
> +        if (sockets == 0) {
> +            cores = cores > 0 ? cores : 1;
> +            threads = threads > 0 ? threads : 1;
> +            sockets = maxcpus / (dies * cores * threads);
> +        } else if (cores == 0) {
> +            threads = threads > 0 ? threads : 1;
> +            cores = maxcpus / (sockets * dies * threads);
> +        } else if (threads == 0) {
> +            threads = maxcpus / (sockets * dies * cores);
> +        }
>      }
>  
> +    maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
> +    cpus = cpus > 0 ? cpus : maxcpus;
> +
>      if (sockets * dies * cores * threads < cpus) {
>          error_setg(errp, "cpu topology: "
>                     "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
> diff --git a/qemu-options.hx b/qemu-options.hx
> index e077aa80bf..0912236b4b 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -210,9 +210,14 @@ SRST
>      Simulate a SMP system with '\ ``n``\ ' CPUs initially present on
>      the machine type board. On boards supporting CPU hotplug, the optional
>      '\ ``maxcpus``\ ' parameter can be set to enable further CPUs to be
> -    added at runtime. If omitted the maximum number of CPUs will be
> -    set to match the initial CPU count. Both parameters are subject to
> -    an upper limit that is determined by the specific machine type chosen.
> +    added at runtime. When both parameters are omitted, the maximum number
> +    of CPUs will be calculated from the provided topology members and the
> +    initial CPU count will match the maximum number. When only one of them
> +    is given then the omitted one will be set to its counterpart's value.
> +    Both parameters may be specified, but the maximum number of CPUs must
> +    equal to or greater than the initial CPU count. Both parameters are

be equal to

> +    subject to an upper limit that is determined by the specific machine
> +    type chosen.
>  
>      To control reporting of CPU topology information, the number of sockets,
>      dies per socket, cores per die, and threads per core can be specified.
> -- 
> 2.19.1
>

Otherwise

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