Re: [PATCH for-6.2 v2 07/11] machine: Prefer cores over sockets in smp parsing since 6.2

[David Gibson]

On Mon, Jul 19, 2021 at 11:20:39AM +0800, Yanan Wang wrote:
> In the real SMP hardware topology world, it's much more likely that
> we have high cores-per-socket counts and few sockets totally. While
> the current preference of sockets over cores in smp parsing results
> in a virtual cpu topology with low cores-per-sockets counts and a
> large number of sockets, which is just contrary to the real world.
> 
> Given that it is better to make the virtual cpu topology be more
> reflective of the real world and also for the sake of compatibility,
> we start to prefer cores over sockets over threads in smp parsing
> since machine type 6.2 for different arches.
> 
> In this patch, a boolean "smp_prefer_sockets" is added, and we only
> enable the old preference on older machines and enable the new one
> since type 6.2 for all arches by using the machine compat mechanism.
> 
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>

ppc parts

Acked-by: David Gibson <david@gibson.dropbear.id.au>

Note that for the pseries machine types, being paravirtual, there is
essentially no guest visible difference between "cores" and "sockets.

> ---
>  hw/arm/virt.c              |  1 +
>  hw/core/machine.c          | 59 +++++++++++++++++++++++++++++---------
>  hw/i386/pc_piix.c          |  1 +
>  hw/i386/pc_q35.c           |  1 +
>  hw/ppc/spapr.c             |  1 +
>  hw/s390x/s390-virtio-ccw.c |  1 +
>  include/hw/boards.h        |  1 +
>  qemu-options.hx            |  4 ++-
>  8 files changed, 55 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 01165f7f53..7babea40dc 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2797,6 +2797,7 @@ static void virt_machine_6_1_options(MachineClass *mc)
>  {
>      virt_machine_6_2_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
> +    mc->smp_prefer_sockets = true;
>  }
>  DEFINE_VIRT_MACHINE(6, 1)
>  
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 63439c4a6d..c074425015 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -744,6 +744,22 @@ void machine_set_cpu_numa_node(MachineState *machine,
>      }
>  }
>  
> +/*
> + * smp_parse - Generic function used to parse the given SMP configuration
> + *
> + * The topology parameters must be specified equal to or great than one
> + * or just omitted, explicit configuration like "cpus=0" is not allowed.
> + * The omitted parameters will be calculated based on the provided ones.
> + *
> + * maxcpus will default to the value of cpus if omitted and will be used
> + * to compute the missing sockets/cores/threads. cpus will be calculated
> + * from the computed parametrs if omitted.
> + *
> + * In calculation of omitted arch-netural sockets/cores/threads, we prefer
> + * sockets over cores over threads before 6.2, while prefer cores over
> + * sockets over threads since 6.2 on. The arch-specific dies will directly
> + * default to 1 if omitted.
> + */
>  static void smp_parse(MachineState *ms, SMPConfiguration *config, Error 
> **errp)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(ms);
> @@ -772,19 +788,36 @@ static void smp_parse(MachineState *ms, 
> SMPConfiguration *config, Error **errp)
>  
>      maxcpus = maxcpus > 0 ? maxcpus : cpus;
>  
> -    /* 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;
> +    /* prefer sockets over cores over threads before 6.2 */
> +    if (mc->smp_prefer_sockets) {
> +        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;
> +        }
> +    /* prefer cores over sockets over threads since 6.2 */
> +    } else {
> +        if (cores == 0) {
> +            sockets = sockets > 0 ? sockets : 1;
> +            threads = threads > 0 ? threads : 1;
> +            cores = maxcpus / (sockets * dies * threads);
> +            cores = cores > 0 ? cores : 1;
> +        } else if (sockets == 0) {
> +            threads = threads > 0 ? threads : 1;
> +            sockets = maxcpus / (dies * cores * threads);
> +            sockets = sockets > 0 ? sockets : 1;
> +        } else if (threads == 0) {
> +            threads = maxcpus / (sockets * dies * cores);
> +            threads = threads > 0 ? threads : 1;
> +        }
>      }
>  
>      /* use the computed parameters to calculate the omitted cpus */
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index fd5c2277f2..9b811fc6ca 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -432,6 +432,7 @@ static void pc_i440fx_6_1_machine_options(MachineClass *m)
>      m->is_default = false;
>      compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
>      compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
> +    m->smp_prefer_sockets = true;
>  }
>  
>  DEFINE_I440FX_MACHINE(v6_1, "pc-i440fx-6.1", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index b45903b15e..88efb7fde4 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -372,6 +372,7 @@ static void pc_q35_6_1_machine_options(MachineClass *m)
>      m->alias = NULL;
>      compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
>      compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
> +    m->smp_prefer_sockets = true;
>  }
>  
>  DEFINE_Q35_MACHINE(v6_1, "pc-q35-6.1", NULL,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d39fd4e644..a481fade51 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4702,6 +4702,7 @@ static void 
> spapr_machine_6_1_class_options(MachineClass *mc)
>  {
>      spapr_machine_6_2_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
> +    mc->smp_prefer_sockets = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(6_1, "6.1", false);
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4d25278cf2..b40e647883 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -809,6 +809,7 @@ static void ccw_machine_6_1_class_options(MachineClass 
> *mc)
>  {
>      ccw_machine_6_2_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
> +    mc->smp_prefer_sockets = true;
>  }
>  DEFINE_CCW_MACHINE(6_1, "6.1", false);
>  
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2832f0f8aa..8df885c9d2 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -247,6 +247,7 @@ struct MachineClass {
>      bool nvdimm_supported;
>      bool numa_mem_supported;
>      bool smp_dies_supported;
> +    bool smp_prefer_sockets;
>      bool auto_enable_numa;
>      const char *default_ram_id;
>  
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 0c9ddc0274..6ef57e838c 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -227,7 +227,9 @@ SRST
>      from those which are given. Historically preference was given to the
>      coarsest topology parameters when computing missing values (ie sockets
>      preferred over cores, which were preferred over threads), however, this
> -    behaviour is considered liable to change.
> +    behaviour is considered liable to change. The historical preference of
> +    sockets over cores over threads works before 6.2, and a new preference
> +    of cores over sockets over threads starts to work since 6.2 on.
>  ERST
>  
>  DEF("numa", HAS_ARG, QEMU_OPTION_numa,

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature