| Message ID | 20210803080527.156556-7-wangyanan55@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | machine: smp parsing fixes and improvement | expand |
> 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. > > Reviewed-by: Andrew Jones <drjones@redhat.com> > Acked-by: Cornelia Huck <cohuck@redhat.com> > Acked-by: David Gibson <david@gibson.dropbear.id.au> > 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 | 36 ++++++++++++++++++++++++++---------- > hw/i386/pc.c | 36 ++++++++++++++++++++++++++---------- > 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(+), 21 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 458d9736e3..a8173a0f45 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -746,6 +746,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; > @@ -757,7 +758,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; > @@ -765,15 +766,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) { > - threads = maxcpus / (sockets * cores); > + if (mc->smp_prefer_sockets) { > + /* prefer sockets over cores over threads 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 if (threads == 0) { > + threads = maxcpus / (sockets * cores); > + } > + } else { > + /* prefer cores over sockets over threads 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); > + } else if (threads == 0) { > + threads = maxcpus / (sockets * cores); > + } I feel this code is repeated at multiple places. Also, (threads == 0) case at the end is common for all the cases, we can move it out of if-else? > } > } > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index afd8b9c283..5d7c3efc43 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,15 +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) { > - threads = maxcpus / (sockets * dies * cores); > + if (mc->smp_prefer_sockets) { > + /* prefer sockets over cores over threads 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 if (threads == 0) { > + threads = maxcpus / (sockets * dies * cores); > + } > + } else { > + /* prefer cores over sockets over threads 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); > + } else 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 >
On 2021/8/6 12:41, 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. >> >> Reviewed-by: Andrew Jones <drjones@redhat.com> >> Acked-by: Cornelia Huck <cohuck@redhat.com> >> Acked-by: David Gibson <david@gibson.dropbear.id.au> >> 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 | 36 ++++++++++++++++++++++++++---------- >> hw/i386/pc.c | 36 ++++++++++++++++++++++++++---------- >> 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(+), 21 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 458d9736e3..a8173a0f45 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -746,6 +746,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; >> @@ -757,7 +758,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; >> @@ -765,15 +766,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) { >> - threads = maxcpus / (sockets * cores); >> + if (mc->smp_prefer_sockets) { >> + /* prefer sockets over cores over threads 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 if (threads == 0) { >> + threads = maxcpus / (sockets * cores); >> + } >> + } else { >> + /* prefer cores over sockets over threads 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); >> + } else if (threads == 0) { >> + threads = maxcpus / (sockets * cores); >> + } > I feel this code is repeated at multiple places. Also, (threads == 0) case > at the end is common for all the cases, we can move it out of if-else? Hi Pankaj, Do you mean the code is repeated in the "if (mc->smp_prefer_sockets)" branch and "else" branch? The calculations of sockets and cores are different in the two branches, I think we have to keep them separately. I agree that (threads=0) case is common and we can try to move it out to a common place, I will refine this in next version. Thanks, Yanan >> } >> } >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index afd8b9c283..5d7c3efc43 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,15 +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) { >> - threads = maxcpus / (sockets * dies * cores); >> + if (mc->smp_prefer_sockets) { >> + /* prefer sockets over cores over threads 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 if (threads == 0) { >> + threads = maxcpus / (sockets * dies * cores); >> + } >> + } else { >> + /* prefer cores over sockets over threads 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); >> + } else 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 >> > .
> >> 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. > >> > >> Reviewed-by: Andrew Jones <drjones@redhat.com> > >> Acked-by: Cornelia Huck <cohuck@redhat.com> > >> Acked-by: David Gibson <david@gibson.dropbear.id.au> > >> 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 | 36 ++++++++++++++++++++++++++---------- > >> hw/i386/pc.c | 36 ++++++++++++++++++++++++++---------- > >> 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(+), 21 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 458d9736e3..a8173a0f45 100644 > >> --- a/hw/core/machine.c > >> +++ b/hw/core/machine.c > >> @@ -746,6 +746,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; > >> @@ -757,7 +758,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; > >> @@ -765,15 +766,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) { > >> - threads = maxcpus / (sockets * cores); > >> + if (mc->smp_prefer_sockets) { > >> + /* prefer sockets over cores over threads 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 if (threads == 0) { > >> + threads = maxcpus / (sockets * cores); > >> + } > >> + } else { > >> + /* prefer cores over sockets over threads 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); > >> + } else if (threads == 0) { > >> + threads = maxcpus / (sockets * cores); > >> + } > > I feel this code is repeated at multiple places. Also, (threads == 0) case > > at the end is common for all the cases, we can move it out of if-else? > Hi Pankaj, > > Do you mean the code is repeated in the "if (mc->smp_prefer_sockets)" > branch and "else" branch? The calculations of sockets and cores are > different in the two branches, I think we have to keep them separately. > > I agree that (threads=0) case is common and we can try to move it out > to a common place, I will refine this in next version. exactly. Thanks, Pankaj > > Thanks, > Yanan > >> } > >> } > >> > >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c > >> index afd8b9c283..5d7c3efc43 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,15 +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) { > >> - threads = maxcpus / (sockets * dies * cores); > >> + if (mc->smp_prefer_sockets) { > >> + /* prefer sockets over cores over threads 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 if (threads == 0) { > >> + threads = maxcpus / (sockets * dies * cores); > >> + } > >> + } else { > >> + /* prefer cores over sockets over threads 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); > >> + } else 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 > >> > > . >
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 458d9736e3..a8173a0f45 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -746,6 +746,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; @@ -757,7 +758,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; @@ -765,15 +766,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) { - threads = maxcpus / (sockets * cores); + if (mc->smp_prefer_sockets) { + /* prefer sockets over cores over threads 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 if (threads == 0) { + threads = maxcpus / (sockets * cores); + } + } else { + /* prefer cores over sockets over threads 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); + } else if (threads == 0) { + threads = maxcpus / (sockets * cores); + } } } diff --git a/hw/i386/pc.c b/hw/i386/pc.c index afd8b9c283..5d7c3efc43 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,15 +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) { - threads = maxcpus / (sockets * dies * cores); + if (mc->smp_prefer_sockets) { + /* prefer sockets over cores over threads 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 if (threads == 0) { + threads = maxcpus / (sockets * dies * cores); + } + } else { + /* prefer cores over sockets over threads 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); + } else 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,