> 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 <berra...@redhat.com> > >> Signed-off-by: Yanan Wang <wangyana...@huawei.com> > >> Acked-by: David Gibson <da...@gibson.dropbear.id.au> > >> Acked-by: Cornelia Huck <coh...@redhat.com> > >> Reviewed-by: Andrew Jones <drjo...@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 > >> > > . >