Re: [PATCH for-6.2 v2 03/11] machine: Uniformly use maxcpus to calculate the omitted parameters

[Andrew Jones]

On Mon, Jul 19, 2021 at 11:20:35AM +0800, Yanan Wang wrote:
> We are currently using maxcpus to calculate the omitted sockets
> but using cpus to calculate the omitted cores/threads. This makes
> cmdlines like:
>   -smp cpus=8,maxcpus=16
>   -smp cpus=8,cores=4,maxcpus=16
>   -smp cpus=8,threads=2,maxcpus=16
> work fine but the ones like:
>   -smp cpus=8,sockets=2,maxcpus=16
>   -smp cpus=8,sockets=2,cores=4,maxcpus=16
>   -smp cpus=8,sockets=2,threads=2,maxcpus=16
> break the invalid cpu topology check.
> 
> Since we require for the valid config that the sum of "sockets * cores
> * dies * threads" should equal to the maxcpus, we should uniformly use
> maxcpus to calculate their omitted values.
> 
> Also the if-branch of "cpus == 0 || sockets == 0" was splited into two
> branches of "cpus == 0" and "sockets == 0" so that we can clearly read
> that we are parsing -smp cmdlines with a preference of cpus over sockets
> over cores over threads.
> 
> Note: change in this patch won't affect any existing working cmdlines
> but improves consistency and allow more incomplete configs to be valid.

We also remove rounding of cores and threads when the math doesn't come
out right, which could possible start reporting a bad config as invalid
which worked before. Or were you able to prove that that can't happen with
your testing?

> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index ed6712e964..c9f15b15a5 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -768,24 +768,26 @@ static void smp_parse(MachineState *ms, 
> SMPConfiguration *config, Error **errp)
>      }
>  
>      /* compute missing values, prefer sockets over cores over threads */
> -    if (cpus == 0 || sockets == 0) {
> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> +
> +    if (cpus == 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 = sockets * dies * cores * threads;
> -        } else {
> -            maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -            sockets = maxcpus / (dies * cores * threads);
> -        }
> +        cpus = sockets * dies * cores * threads;
> +        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 = cpus / (sockets * dies * threads);
> -        cores = cores > 0 ? cores : 1;
> +        cores = maxcpus / (sockets * dies * threads);
>      } else if (threads == 0) {
> -        threads = cpus / (sockets * dies * cores);
> -        threads = threads > 0 ? threads : 1;
> -    } else if (sockets * dies * cores * threads < cpus) {
> +        threads = maxcpus / (sockets * dies * cores);
> +    }
> +
> +    if (sockets * dies * cores * threads < cpus) {
>          g_autofree char *dies_msg = g_strdup_printf(
>              mc->smp_dies_supported ? " * dies (%u)" : "", dies);
>          error_setg(errp, "cpu topology: "
> @@ -795,8 +797,6 @@ static void smp_parse(MachineState *ms, SMPConfiguration 
> *config, Error **errp)
>          return;
>      }
>  
> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -
>      if (maxcpus < cpus) {
>          error_setg(errp, "maxcpus must be equal to or greater than smp");
>          return;
> -- 
> 2.19.1
>

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