Re: [PATCH for-6.2 v2 05/11] machine: Improve the error reporting of smp parsing

[Andrew Jones]

On Mon, Jul 19, 2021 at 11:20:37AM +0800, Yanan Wang wrote:
> We totally have two requirements for a valid SMP configuration:

s/totally//

> the sum of "sockets * dies * cores * threads" must represent all

the product

> the possible cpus, i.e., max_cpus, and must include the initial
> present cpus, i.e., smp_cpus.
> 
> We only need to ensure "sockets * dies * cores * threads == maxcpus"
> at first and then ensure "sockets * dies * cores * threads >= cpus".

Or, "maxcpus >= cpus"

> With a reasonable order of the sanity-check, we can simplify the
> error reporting code.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 668f0a1553..8b4d07d3fc 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -788,21 +788,6 @@ static void smp_parse(MachineState *ms, SMPConfiguration 
> *config, Error **errp)
>      cpus = cpus > 0 ? cpus : sockets * dies * cores * threads;
>      maxcpus = maxcpus > 0 ? maxcpus : cpus;
>  
> -    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: "
> -                   "sockets (%u)%s * cores (%u) * threads (%u) < "
> -                   "smp_cpus (%u)",
> -                   sockets, dies_msg, cores, threads, cpus);
> -        return;
> -    }
> -
> -    if (maxcpus < cpus) {
> -        error_setg(errp, "maxcpus must be equal to or greater than smp");
> -        return;
> -    }

This may be redundant when determining a valid config, but by checking it
separately we can provide a more useful error message.

> -
>      if (sockets * dies * cores * threads != maxcpus) {
>          g_autofree char *dies_msg = g_strdup_printf(
>              mc->smp_dies_supported ? " * dies (%u)" : "", dies);
> @@ -814,6 +799,16 @@ static void smp_parse(MachineState *ms, SMPConfiguration 
> *config, Error **errp)
>          return;
>      }
>  
> +    if (sockets * dies * cores * threads < cpus) {
> +        g_autofree char *dies_msg = g_strdup_printf(
> +            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
> +        error_setg(errp, "Invalid CPU topology: "
> +                   "sockets (%u)%s * cores (%u) * threads (%u) < "
> +                   "smp_cpus (%u)",
> +                   sockets, dies_msg, cores, threads, cpus);
> +        return;
> +    }
> +
>      ms->smp.cpus = cpus;
>      ms->smp.sockets = sockets;
>      ms->smp.dies = dies;
> -- 
> 2.19.1
>

I'm not sure we need this patch.

Thanks,
drew