Re: [PATCH for-6.2 v2 02/11] machine: Make smp_parse generic enough for all arches

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Jul 19, 2021 at 11:20:34AM +0800, Yanan Wang wrote:
> Currently the only difference between smp_parse and pc_smp_parse
> is the support of multi-dies and the related error reporting code.
> With an arch compat variable "bool smp_dies_supported", we can
> easily make smp_parse generic enough for all arches and the PC
> specific one can be removed.
> 
> Making smp_parse() generic enough can reduce code duplication and
> ease the code maintenance, and also allows extending the topology
> with more arch specific members (e.g., clusters) in the future.
> 
> No functional change intended.
> 
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c   | 28 ++++++++++-------
>  hw/i386/pc.c        | 76 +--------------------------------------------
>  include/hw/boards.h |  1 +
>  3 files changed, 19 insertions(+), 86 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d73daa10f4..ed6712e964 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -743,6 +743,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
>  
>  static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>  {
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
>      unsigned cpus    = config->has_cpus ? config->cpus : 0;
>      unsigned sockets = config->has_sockets ? config->sockets : 0;
>      unsigned dies    = config->has_dies ? config->dies : 1;
> @@ -761,7 +762,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>          return;
>      }
>  
> -    if (dies > 1) {
> +    if (!mc->smp_dies_supported && dies > 1) {
>          error_setg(errp, "dies not supported by this machine's CPU topology");
>          return;
>      }
> @@ -772,23 +773,25 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>          threads = threads > 0 ? threads : 1;
>          if (cpus == 0) {
>              sockets = sockets > 0 ? sockets : 1;
> -            cpus = cores * threads * sockets;
> +            cpus = sockets * dies * cores * threads;
>          } else {
>              maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -            sockets = maxcpus / (cores * threads);
> +            sockets = maxcpus / (dies * cores * threads);
>          }
>      } else if (cores == 0) {
>          threads = threads > 0 ? threads : 1;
> -        cores = cpus / (sockets * threads);
> +        cores = cpus / (sockets * dies * threads);
>          cores = cores > 0 ? cores : 1;
>      } else if (threads == 0) {
> -        threads = cpus / (cores * sockets);
> +        threads = cpus / (sockets * dies * cores);
>          threads = threads > 0 ? threads : 1;
> -    } else if (sockets * cores * threads < cpus) {
> +    } else 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) * cores (%u) * threads (%u) < "
> +                   "sockets (%u)%s * cores (%u) * threads (%u) < "
>                     "smp_cpus (%u)",
> -                   sockets, cores, threads, cpus);
> +                   sockets, dies_msg, cores, threads, cpus);

Since we're allowing dies=1 (but not greater), I'm not convinced we
need the conditionally built error message, and could just include
"* dies" all the time.

If we do want it to be conditionally different though, I'd just
sugest calling error_setg twice. Although this duplicates stuff,
it'll be clearer to read which I think is a net win.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|