Re: [PATCH for-6.2 v2 07/11] machine: Prefer cores over sockets in smp parsing since 6.2
On 2021/7/20 1:13, Andrew Jones wrote: 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 Signed-off-by: Yanan Wang --- 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. Yes, this is much clearer. Now I understand why you suggested in patch #4 to change the format from cpus = cpus > 0 ? cpus : sockets * dies * cores * threads; maxcpus = maxcpus > 0 ? maxcpus : cpus; to maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads; cpus = cpus > 0 ? cpus : maxcpus; What you suggest seems more reasonable according to above doc, I will adjust the related part as you suggested. + * + * 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// Right. + * 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; +
Re: [PATCH for-6.2 v2 07/11] machine: Prefer cores over sockets in smp parsing since 6.2
On 2021/7/19 11:40, David Gibson wrote: 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 Signed-off-by: Yanan Wang ppc parts Acked-by: David Gibson Note that for the pseries machine types, being paravirtual, there is essentially no guest visible difference between "cores" and "sockets. I see. When the difference start to make some sense for the pseries guest, then I think high-count cores may also be preferred and we will already have the preference of cores at that time because of today's work. Thanks, Yanan . --- 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
Re: [PATCH for-6.2 v2 07/11] machine: Prefer cores over sockets in smp parsing since 6.2
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 > Signed-off-by: Yanan Wang > --- > 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) { >
Re: [PATCH for-6.2 v2 07/11] machine: Prefer cores over sockets in smp parsing since 6.2
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 > Signed-off-by: Yanan Wang ppc parts Acked-by: David Gibson 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; > +
[PATCH for-6.2 v2 07/11] machine: Prefer cores over sockets in smp parsing since 6.2
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 Signed-off-by: Yanan Wang --- 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