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

[Andrew Jones]

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>
> ---
>  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.

parameters

Or how about something like this:

When both maxcpus and cpus are omitted maxcpus will be calculated from the
given parameters and cpus will be set equal to maxcpus. When only one of
maxcpus and cpus is given then the omitted one will be set to its given
counterpart's value. Both maxcpus and cpus may be specified, but cpus must
be less than or equal to maxcpus.

> + *
> + * In calculation of omitted arch-netural sockets/cores/threads, we prefer
> + * sockets over cores over threads before 6.2, while prefer cores over

while preferring

> + * sockets over threads since 6.2 on. The arch-specific dies will directly

s/on//

> + * 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) {

please move the comment into the if, so...

> +        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 */

...here we can put the comment in the else

> +    } 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.

Prior to 6.2 the preference was sockets over cores over threads. Since 6.2
the preference is cores over sockets over threads.

>  ERST
>  
>  DEF("numa", HAS_ARG, QEMU_OPTION_numa,
> -- 
> 2.19.1
>

Thanks,
drew