Re: [PATCH for-6.2 v5 06/14] machine: Prefer cores over sockets in smp parsing since 6.2

[Pankaj Gupta]

> On 2021/8/14 0:30, Pankaj Gupta 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>
> >> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> >> Acked-by: Cornelia Huck <cohuck@redhat.com>
> >> Reviewed-by: Andrew Jones <drjones@redhat.com>
> >> ---
> >>   hw/arm/virt.c              |  1 +
> >>   hw/core/machine.c          | 35 ++++++++++++++++++++++++++---------
> >>   hw/i386/pc.c               | 35 ++++++++++++++++++++++++++---------
> >>   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            |  3 ++-
> >>   9 files changed, 60 insertions(+), 19 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;
> > Will this be set for virt_machine_6_2_?
> >
> Hi,
>
> We hope to start to assume cores over sockets on virt_6_2 machines
> and the later, and keep the old style of assuming sockets over cores
> on virt_6_1 machines and the older. Is there any concern here?

O.k. Thank you for clarifying.

Best regards,
Pankaj

>
> Thanks,
> Yanan
> >>   }
> >>   DEFINE_VIRT_MACHINE(6, 1)
> >>
> >> diff --git a/hw/core/machine.c b/hw/core/machine.c
> >> index bdce80df32..15b41c52e8 100644
> >> --- a/hw/core/machine.c
> >> +++ b/hw/core/machine.c
> >> @@ -748,6 +748,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 cores   = config->has_cores ? config->cores : 0;
> >> @@ -759,7 +760,7 @@ static void smp_parse(MachineState *ms, 
> >> SMPConfiguration *config, Error **errp)
> >>           return;
> >>       }
> >>
> >> -    /* compute missing values, prefer sockets over cores over threads */
> >> +    /* compute missing values based on the provided ones */
> >>       if (cpus == 0 && maxcpus == 0) {
> >>           sockets = sockets > 0 ? sockets : 1;
> >>           cores = cores > 0 ? cores : 1;
> >> @@ -767,14 +768,30 @@ static void smp_parse(MachineState *ms, 
> >> SMPConfiguration *config, Error **errp)
> >>       } else {
> >>           maxcpus = maxcpus > 0 ? maxcpus : cpus;
> >>
> >> -        if (sockets == 0) {
> >> -            cores = cores > 0 ? cores : 1;
> >> -            threads = threads > 0 ? threads : 1;
> >> -            sockets = maxcpus / (cores * threads);
> >> -        } else if (cores == 0) {
> >> -            threads = threads > 0 ? threads : 1;
> >> -            cores = maxcpus / (sockets * threads);
> >> -        } else if (threads == 0) {
> >> +        if (mc->smp_prefer_sockets) {
> >> +            /* prefer sockets over cores before 6.2 */
> >> +            if (sockets == 0) {
> >> +                cores = cores > 0 ? cores : 1;
> >> +                threads = threads > 0 ? threads : 1;
> >> +                sockets = maxcpus / (cores * threads);
> >> +            } else if (cores == 0) {
> >> +                threads = threads > 0 ? threads : 1;
> >> +                cores = maxcpus / (sockets * threads);
> >> +            }
> >> +        } else {
> >> +            /* prefer cores over sockets since 6.2 */
> >> +            if (cores == 0) {
> >> +                sockets = sockets > 0 ? sockets : 1;
> >> +                threads = threads > 0 ? threads : 1;
> >> +                cores = maxcpus / (sockets * threads);
> >> +            } else if (sockets == 0) {
> >> +                threads = threads > 0 ? threads : 1;
> >> +                sockets = maxcpus / (cores * threads);
> >> +            }
> >> +        }
> >> +
> >> +        /* try to calculate omitted threads at last */
> >> +        if (threads == 0) {
> >>               threads = maxcpus / (sockets * cores);
> >>           }
> >>       }
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index afd8b9c283..4b05ff7160 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -717,6 +717,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int 
> >> level)
> >>    */
> >>   static void pc_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 : 0;
> >> @@ -727,7 +728,7 @@ static void pc_smp_parse(MachineState *ms, 
> >> SMPConfiguration *config, Error **err
> >>       /* directly default dies to 1 if it's omitted */
> >>       dies = dies > 0 ? dies : 1;
> >>
> >> -    /* compute missing values, prefer sockets over cores over threads */
> >> +    /* compute missing values based on the provided ones */
> >>       if (cpus == 0 && maxcpus == 0) {
> >>           sockets = sockets > 0 ? sockets : 1;
> >>           cores = cores > 0 ? cores : 1;
> >> @@ -735,14 +736,30 @@ static void pc_smp_parse(MachineState *ms, 
> >> SMPConfiguration *config, Error **err
> >>       } else {
> >>           maxcpus = maxcpus > 0 ? maxcpus : cpus;
> >>
> >> -        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 = maxcpus / (sockets * dies * threads);
> >> -        } else if (threads == 0) {
> >> +        if (mc->smp_prefer_sockets) {
> >> +            /* prefer sockets over cores before 6.2 */
> >> +            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 = maxcpus / (sockets * dies * threads);
> >> +            }
> >> +        } else {
> >> +            /* prefer cores over sockets since 6.2 */
> >> +            if (cores == 0) {
> >> +                sockets = sockets > 0 ? sockets : 1;
> >> +                threads = threads > 0 ? threads : 1;
> >> +                cores = maxcpus / (sockets * dies * threads);
> >> +            } else if (sockets == 0) {
> >> +                threads = threads > 0 ? threads : 1;
> >> +                sockets = maxcpus / (dies * cores * threads);
> >> +            }
> >> +        }
> >> +
> >> +        /* try to calculate omitted threads at last */
> >> +        if (threads == 0) {
> >>               threads = maxcpus / (sockets * dies * cores);
> >>           }
> >>       }
> >> 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 463a5514f9..2ae039b74f 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 auto_enable_numa;
> >> +    bool smp_prefer_sockets;
> >>       const char *default_ram_id;
> >>
> >>       HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> >> diff --git a/qemu-options.hx b/qemu-options.hx
> >> index 06f819177e..451d2cd817 100644
> >> --- a/qemu-options.hx
> >> +++ b/qemu-options.hx
> >> @@ -238,7 +238,8 @@ SRST
> >>       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.
> >> +    liable to change. 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
> >>
> > .
>