Re: [PATCH for-6.2 v2 04/11] machine: Use the computed parameters to calculate omitted cpus

[Andrew Jones]

On Mon, Jul 19, 2021 at 11:20:36AM +0800, Yanan Wang wrote:
> Currently we directly calculate the omitted cpus based on the already
> provided 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 use the computed paramters to calculate
> cpus when maxcpus is provided while cpus is omitted, which will make
> above configs start to work.
> 
> 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 | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c9f15b15a5..668f0a1553 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -767,26 +767,27 @@ static void smp_parse(MachineState *ms, 
> SMPConfiguration *config, Error **errp)
>          return;
>      }
>  
> -    /* compute missing values, prefer sockets over cores over threads */
>      maxcpus = maxcpus > 0 ? maxcpus : cpus;
>  
> -    if (cpus == 0) {
> -        sockets = sockets > 0 ? sockets : 1;
> -        cores = cores > 0 ? cores : 1;
> -        threads = threads > 0 ? threads : 1;
> -        cpus = sockets * dies * cores * threads;
> -        maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -    } else if (sockets == 0) {
> +    /* compute missing values, prefer sockets over cores over threads */
> +    if (sockets == 0) {
>          cores = cores > 0 ? cores : 1;
>          threads = threads > 0 ? threads : 1;
>          sockets = maxcpus / (dies * cores * threads);
> +        sockets = sockets > 0 ? sockets : 1;
>      } else if (cores == 0) {
>          threads = threads > 0 ? threads : 1;
>          cores = maxcpus / (sockets * dies * threads);
> +        cores = cores > 0 ? cores : 1;
>      } else if (threads == 0) {
>          threads = maxcpus / (sockets * dies * cores);
> +        threads = threads > 0 ? threads : 1;
>      }

I didn't think we wanted this rounding which this patch adds back into
cores and threads and now also sockets.

>  
> +    /* use the computed parameters to calculate the omitted cpus */
> +    cpus = cpus > 0 ? cpus : sockets * dies * cores * threads;
> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;

It doesn't really matter, but I think I'd rather write this like

 maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
 cpus = cpus > 0 ? cpus : maxcpus;

> +
>      if (sockets * dies * cores * threads < cpus) {
>          g_autofree char *dies_msg = g_strdup_printf(
>              mc->smp_dies_supported ? " * dies (%u)" : "", dies);
> -- 
> 2.19.1
> 

Thanks,
drew