| Message ID | 20210719032043.25416-4-wangyanan55@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | machine: smp parsing fixes and improvement | expand |
On Mon, Jul 19, 2021 at 11:20:35AM +0800, Yanan Wang wrote: > We are currently using maxcpus to calculate the omitted sockets > but using cpus to calculate the omitted cores/threads. This makes > cmdlines like: > -smp cpus=8,maxcpus=16 > -smp cpus=8,cores=4,maxcpus=16 > -smp cpus=8,threads=2,maxcpus=16 > work fine but the ones like: > -smp cpus=8,sockets=2,maxcpus=16 > -smp cpus=8,sockets=2,cores=4,maxcpus=16 > -smp cpus=8,sockets=2,threads=2,maxcpus=16 > break the invalid cpu topology check. > > Since we require for the valid config that the sum of "sockets * cores > * dies * threads" should equal to the maxcpus, we should uniformly use > maxcpus to calculate their omitted values. > > Also the if-branch of "cpus == 0 || sockets == 0" was splited into two > branches of "cpus == 0" and "sockets == 0" so that we can clearly read > that we are parsing -smp cmdlines with a preference of cpus over sockets > over cores over threads. > > Note: change in this patch won't affect any existing working cmdlines > but improves consistency and allow more incomplete configs to be valid. We also remove rounding of cores and threads when the math doesn't come out right, which could possible start reporting a bad config as invalid which worked before. Or were you able to prove that that can't happen with your testing? > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > --- > hw/core/machine.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index ed6712e964..c9f15b15a5 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -768,24 +768,26 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > } > > /* compute missing values, prefer sockets over cores over threads */ > - if (cpus == 0 || sockets == 0) { > + maxcpus = maxcpus > 0 ? maxcpus : cpus; > + > + if (cpus == 0) { > + sockets = sockets > 0 ? sockets : 1; > cores = cores > 0 ? cores : 1; > threads = threads > 0 ? threads : 1; > - if (cpus == 0) { > - sockets = sockets > 0 ? sockets : 1; > - cpus = sockets * dies * cores * threads; > - } else { > - maxcpus = maxcpus > 0 ? maxcpus : cpus; > - sockets = maxcpus / (dies * cores * threads); > - } > + cpus = sockets * dies * cores * threads; > + maxcpus = maxcpus > 0 ? maxcpus : cpus; > + } else 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 = cpus / (sockets * dies * threads); > - cores = cores > 0 ? cores : 1; > + cores = maxcpus / (sockets * dies * threads); > } else if (threads == 0) { > - threads = cpus / (sockets * dies * cores); > - threads = threads > 0 ? threads : 1; > - } else if (sockets * dies * cores * threads < cpus) { > + threads = maxcpus / (sockets * dies * cores); > + } > + > + if (sockets * dies * cores * threads < cpus) { > g_autofree char *dies_msg = g_strdup_printf( > mc->smp_dies_supported ? " * dies (%u)" : "", dies); > error_setg(errp, "cpu topology: " > @@ -795,8 +797,6 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > return; > } > > - maxcpus = maxcpus > 0 ? maxcpus : cpus; > - > if (maxcpus < cpus) { > error_setg(errp, "maxcpus must be equal to or greater than smp"); > return; > -- > 2.19.1 > Reviewed-by: Andrew Jones <drjones@redhat.com>
On 2021/7/20 0:36, Andrew Jones wrote: > On Mon, Jul 19, 2021 at 11:20:35AM +0800, Yanan Wang wrote: >> We are currently using maxcpus to calculate the omitted sockets >> but using cpus to calculate the omitted cores/threads. This makes >> cmdlines like: >> -smp cpus=8,maxcpus=16 >> -smp cpus=8,cores=4,maxcpus=16 >> -smp cpus=8,threads=2,maxcpus=16 >> work fine but the ones like: >> -smp cpus=8,sockets=2,maxcpus=16 >> -smp cpus=8,sockets=2,cores=4,maxcpus=16 >> -smp cpus=8,sockets=2,threads=2,maxcpus=16 >> break the invalid cpu topology check. >> >> Since we require for the valid config that the sum of "sockets * cores >> * dies * threads" should equal to the maxcpus, we should uniformly use >> maxcpus to calculate their omitted values. >> >> Also the if-branch of "cpus == 0 || sockets == 0" was splited into two >> branches of "cpus == 0" and "sockets == 0" so that we can clearly read >> that we are parsing -smp cmdlines with a preference of cpus over sockets >> over cores over threads. >> >> Note: change in this patch won't affect any existing working cmdlines >> but improves consistency and allow more incomplete configs to be valid. > We also remove rounding of cores and threads when the math doesn't come > out right, which could possible start reporting a bad config as invalid > which worked before. Or were you able to prove that that can't happen with > your testing? I also had this concern, but I think that can't happen for sure. Take the if-branch of "cores == 0" as an example, Before this patch: We use cpus to calculate the missing cores and round it up to 1 if the result is zero, and at last set maxcpus to match cpus if it's omitted. So the parsing result must have met the two conditions: 1) sockets * cores * threads == maxcpus 2) sockets * cores * threads >= cpus After this patch: We start to use maxcpus to calculate the missing cores and also remove the rounding. For the same config mentioned above, it still works and the parsing result will also not change, because we will never get a fractional value of cores (maxcpus is multiple of (sockets * threads) ). Like the valid config "-smp 8,sockets=16,maxcpus=16", we will get "cpus=8,sockets=16,cores=1,threads=1,maxcpus=16" before, and still get the same result after. Please correct me if I missed something, thanks. >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >> --- >> hw/core/machine.c | 30 +++++++++++++++--------------- >> 1 file changed, 15 insertions(+), 15 deletions(-) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index ed6712e964..c9f15b15a5 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -768,24 +768,26 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) >> } >> >> /* compute missing values, prefer sockets over cores over threads */ >> - if (cpus == 0 || sockets == 0) { >> + maxcpus = maxcpus > 0 ? maxcpus : cpus; >> + >> + if (cpus == 0) { >> + sockets = sockets > 0 ? sockets : 1; >> cores = cores > 0 ? cores : 1; >> threads = threads > 0 ? threads : 1; >> - if (cpus == 0) { >> - sockets = sockets > 0 ? sockets : 1; >> - cpus = sockets * dies * cores * threads; >> - } else { >> - maxcpus = maxcpus > 0 ? maxcpus : cpus; >> - sockets = maxcpus / (dies * cores * threads); >> - } >> + cpus = sockets * dies * cores * threads; >> + maxcpus = maxcpus > 0 ? maxcpus : cpus; >> + } else 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 = cpus / (sockets * dies * threads); >> - cores = cores > 0 ? cores : 1; >> + cores = maxcpus / (sockets * dies * threads); >> } else if (threads == 0) { >> - threads = cpus / (sockets * dies * cores); >> - threads = threads > 0 ? threads : 1; >> - } else if (sockets * dies * cores * threads < cpus) { >> + threads = maxcpus / (sockets * dies * cores); >> + } >> + >> + if (sockets * dies * cores * threads < cpus) { >> g_autofree char *dies_msg = g_strdup_printf( >> mc->smp_dies_supported ? " * dies (%u)" : "", dies); >> error_setg(errp, "cpu topology: " >> @@ -795,8 +797,6 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) >> return; >> } >> >> - maxcpus = maxcpus > 0 ? maxcpus : cpus; >> - >> if (maxcpus < cpus) { >> error_setg(errp, "maxcpus must be equal to or greater than smp"); >> return; >> -- >> 2.19.1 >> > Reviewed-by: Andrew Jones <drjones@redhat.com> Thanks, Yanan .
diff --git a/hw/core/machine.c b/hw/core/machine.c index ed6712e964..c9f15b15a5 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -768,24 +768,26 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) } /* compute missing values, prefer sockets over cores over threads */ - if (cpus == 0 || sockets == 0) { + maxcpus = maxcpus > 0 ? maxcpus : cpus; + + if (cpus == 0) { + sockets = sockets > 0 ? sockets : 1; cores = cores > 0 ? cores : 1; threads = threads > 0 ? threads : 1; - if (cpus == 0) { - sockets = sockets > 0 ? sockets : 1; - cpus = sockets * dies * cores * threads; - } else { - maxcpus = maxcpus > 0 ? maxcpus : cpus; - sockets = maxcpus / (dies * cores * threads); - } + cpus = sockets * dies * cores * threads; + maxcpus = maxcpus > 0 ? maxcpus : cpus; + } else 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 = cpus / (sockets * dies * threads); - cores = cores > 0 ? cores : 1; + cores = maxcpus / (sockets * dies * threads); } else if (threads == 0) { - threads = cpus / (sockets * dies * cores); - threads = threads > 0 ? threads : 1; - } else if (sockets * dies * cores * threads < cpus) { + threads = maxcpus / (sockets * dies * cores); + } + + if (sockets * dies * cores * threads < cpus) { g_autofree char *dies_msg = g_strdup_printf( mc->smp_dies_supported ? " * dies (%u)" : "", dies); error_setg(errp, "cpu topology: " @@ -795,8 +797,6 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) return; } - maxcpus = maxcpus > 0 ? maxcpus : cpus; - if (maxcpus < cpus) { error_setg(errp, "maxcpus must be equal to or greater than smp"); return;
We are currently using maxcpus to calculate the omitted sockets but using cpus to calculate the omitted cores/threads. This makes cmdlines like: -smp cpus=8,maxcpus=16 -smp cpus=8,cores=4,maxcpus=16 -smp cpus=8,threads=2,maxcpus=16 work fine but the ones like: -smp cpus=8,sockets=2,maxcpus=16 -smp cpus=8,sockets=2,cores=4,maxcpus=16 -smp cpus=8,sockets=2,threads=2,maxcpus=16 break the invalid cpu topology check. Since we require for the valid config that the sum of "sockets * cores * dies * threads" should equal to the maxcpus, we should uniformly use maxcpus to calculate their omitted values. Also the if-branch of "cpus == 0 || sockets == 0" was splited into two branches of "cpus == 0" and "sockets == 0" so that we can clearly read that we are parsing -smp cmdlines with a preference of cpus over sockets over cores over threads. Note: change in this patch won't affect any existing working cmdlines but improves consistency and allow more incomplete configs to be valid. Signed-off-by: Yanan Wang <wangyanan55@huawei.com> --- hw/core/machine.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)