| Message ID | 20210702100739.13672-2-wangyanan55@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | machine: smp parsing fixes and improvement | expand |
On Fri, Jul 02, 2021 at 06:07:34PM +0800, Yanan Wang wrote: > It is currently allowed to explicitly specified the topology parameters > as 0 in the -smp cmdlines, such as -smp cpus=8,maxcpus=0,sockets=0. And > for the values of cpus/sockets/cores/threads, we always determine that > they are ommited if either set to 0 in the cmdline(e.g. sockets=0) or > just not explicitly specified, then we compute the ommited values. > > We probably should also treat "maxcpus=0" as ommited and then set the > value to match smp cpus. This makes cmdlines like "-smp 8, maxcpus=0" > start to work as "-smp 8,maxcpus=8,sockets=8,cores=1,threads=1". > > Note that this patch won't affect any existing working cmdlines, but > will allow configuration like "-smp cpus=n,maxcpus=0" to be valid. Personally, I'd rather see -smp cpus=n,sockets=0 become an error than to "fix" -smp cpus=n,maxcpus=0. Thanks, drew > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > --- > hw/core/machine.c | 14 ++++++++------ > hw/i386/pc.c | 14 ++++++++------ > 2 files changed, 16 insertions(+), 12 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index ffc076ae84..f17bbe3275 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -745,6 +745,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > unsigned sockets = config->has_sockets ? config->sockets : 0; > unsigned cores = config->has_cores ? config->cores : 0; > unsigned threads = config->has_threads ? config->threads : 0; > + unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; > > if (config->has_dies && config->dies != 0 && config->dies != 1) { > error_setg(errp, "dies not supported by this machine's CPU topology"); > @@ -758,8 +759,8 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > sockets = sockets > 0 ? sockets : 1; > cpus = cores * threads * sockets; > } else { > - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; > - sockets = ms->smp.max_cpus / (cores * threads); > + maxcpus = maxcpus > 0 ? maxcpus : cpus; > + sockets = maxcpus / (cores * threads); > } > } else if (cores == 0) { > threads = threads > 0 ? threads : 1; > @@ -776,19 +777,19 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > return; > } > > - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; > + maxcpus = maxcpus > 0 ? maxcpus : cpus; > > - if (ms->smp.max_cpus < cpus) { > + if (maxcpus < cpus) { > error_setg(errp, "maxcpus must be equal to or greater than smp"); > return; > } > > - if (sockets * cores * threads != ms->smp.max_cpus) { > + if (sockets * cores * threads != maxcpus) { > error_setg(errp, "Invalid CPU topology: " > "sockets (%u) * cores (%u) * threads (%u) " > "!= maxcpus (%u)", > sockets, cores, threads, > - ms->smp.max_cpus); > + maxcpus); > return; > } > > @@ -796,6 +797,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > ms->smp.cores = cores; > ms->smp.threads = threads; > ms->smp.sockets = sockets; > + ms->smp.max_cpus = maxcpus; > } > > static void machine_class_init(ObjectClass *oc, void *data) > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 8e1220db72..a9b22fdc01 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -717,6 +717,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > unsigned dies = config->has_dies ? config->dies : 1; > unsigned cores = config->has_cores ? config->cores : 0; > unsigned threads = config->has_threads ? config->threads : 0; > + unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; > > /* compute missing values, prefer sockets over cores over threads */ > if (cpus == 0 || sockets == 0) { > @@ -726,8 +727,8 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > sockets = sockets > 0 ? sockets : 1; > cpus = cores * threads * dies * sockets; > } else { > - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; > - sockets = ms->smp.max_cpus / (cores * threads * dies); > + maxcpus = maxcpus > 0 ? maxcpus : cpus; > + sockets = maxcpus / (dies * cores * threads); > } > } else if (cores == 0) { > threads = threads > 0 ? threads : 1; > @@ -744,19 +745,19 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > return; > } > > - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; > + maxcpus = maxcpus > 0 ? maxcpus : cpus; > > - if (ms->smp.max_cpus < cpus) { > + if (maxcpus < cpus) { > error_setg(errp, "maxcpus must be equal to or greater than smp"); > return; > } > > - if (sockets * dies * cores * threads != ms->smp.max_cpus) { > + if (sockets * dies * cores * threads != maxcpus) { > error_setg(errp, "Invalid CPU topology deprecated: " > "sockets (%u) * dies (%u) * cores (%u) * threads (%u) " > "!= maxcpus (%u)", > sockets, dies, cores, threads, > - ms->smp.max_cpus); > + maxcpus); > return; > } > > @@ -765,6 +766,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > ms->smp.threads = threads; > ms->smp.sockets = sockets; > ms->smp.dies = dies; > + ms->smp.max_cpus = maxcpus; > } > > static > -- > 2.19.1 >
On Mon, Jul 12, 2021 at 04:57:37PM +0200, Andrew Jones wrote: > On Fri, Jul 02, 2021 at 06:07:34PM +0800, Yanan Wang wrote: > > It is currently allowed to explicitly specified the topology parameters > > as 0 in the -smp cmdlines, such as -smp cpus=8,maxcpus=0,sockets=0. And > > for the values of cpus/sockets/cores/threads, we always determine that > > they are ommited if either set to 0 in the cmdline(e.g. sockets=0) or > > just not explicitly specified, then we compute the ommited values. > > > > We probably should also treat "maxcpus=0" as ommited and then set the > > value to match smp cpus. This makes cmdlines like "-smp 8, maxcpus=0" > > start to work as "-smp 8,maxcpus=8,sockets=8,cores=1,threads=1". > > > > Note that this patch won't affect any existing working cmdlines, but > > will allow configuration like "-smp cpus=n,maxcpus=0" to be valid. > > Personally, I'd rather see -smp cpus=n,sockets=0 become an error than to > "fix" -smp cpus=n,maxcpus=0. Yes, a value of 0 is not really something a user should ever be explicitly setting. It is purely an internal default that is used by QEMU to identify when it needs to auto-fill a value. A user should give a non-zero value, or omit the parameter entirely Regards, Daniel
diff --git a/hw/core/machine.c b/hw/core/machine.c index ffc076ae84..f17bbe3275 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -745,6 +745,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) unsigned sockets = config->has_sockets ? config->sockets : 0; unsigned cores = config->has_cores ? config->cores : 0; unsigned threads = config->has_threads ? config->threads : 0; + unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; if (config->has_dies && config->dies != 0 && config->dies != 1) { error_setg(errp, "dies not supported by this machine's CPU topology"); @@ -758,8 +759,8 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) sockets = sockets > 0 ? sockets : 1; cpus = cores * threads * sockets; } else { - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; - sockets = ms->smp.max_cpus / (cores * threads); + maxcpus = maxcpus > 0 ? maxcpus : cpus; + sockets = maxcpus / (cores * threads); } } else if (cores == 0) { threads = threads > 0 ? threads : 1; @@ -776,19 +777,19 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) return; } - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; + maxcpus = maxcpus > 0 ? maxcpus : cpus; - if (ms->smp.max_cpus < cpus) { + if (maxcpus < cpus) { error_setg(errp, "maxcpus must be equal to or greater than smp"); return; } - if (sockets * cores * threads != ms->smp.max_cpus) { + if (sockets * cores * threads != maxcpus) { error_setg(errp, "Invalid CPU topology: " "sockets (%u) * cores (%u) * threads (%u) " "!= maxcpus (%u)", sockets, cores, threads, - ms->smp.max_cpus); + maxcpus); return; } @@ -796,6 +797,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) ms->smp.cores = cores; ms->smp.threads = threads; ms->smp.sockets = sockets; + ms->smp.max_cpus = maxcpus; } static void machine_class_init(ObjectClass *oc, void *data) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 8e1220db72..a9b22fdc01 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -717,6 +717,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err unsigned dies = config->has_dies ? config->dies : 1; unsigned cores = config->has_cores ? config->cores : 0; unsigned threads = config->has_threads ? config->threads : 0; + unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; /* compute missing values, prefer sockets over cores over threads */ if (cpus == 0 || sockets == 0) { @@ -726,8 +727,8 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err sockets = sockets > 0 ? sockets : 1; cpus = cores * threads * dies * sockets; } else { - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; - sockets = ms->smp.max_cpus / (cores * threads * dies); + maxcpus = maxcpus > 0 ? maxcpus : cpus; + sockets = maxcpus / (dies * cores * threads); } } else if (cores == 0) { threads = threads > 0 ? threads : 1; @@ -744,19 +745,19 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err return; } - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; + maxcpus = maxcpus > 0 ? maxcpus : cpus; - if (ms->smp.max_cpus < cpus) { + if (maxcpus < cpus) { error_setg(errp, "maxcpus must be equal to or greater than smp"); return; } - if (sockets * dies * cores * threads != ms->smp.max_cpus) { + if (sockets * dies * cores * threads != maxcpus) { error_setg(errp, "Invalid CPU topology deprecated: " "sockets (%u) * dies (%u) * cores (%u) * threads (%u) " "!= maxcpus (%u)", sockets, dies, cores, threads, - ms->smp.max_cpus); + maxcpus); return; } @@ -765,6 +766,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err ms->smp.threads = threads; ms->smp.sockets = sockets; ms->smp.dies = dies; + ms->smp.max_cpus = maxcpus; } static
It is currently allowed to explicitly specified the topology parameters as 0 in the -smp cmdlines, such as -smp cpus=8,maxcpus=0,sockets=0. And for the values of cpus/sockets/cores/threads, we always determine that they are ommited if either set to 0 in the cmdline(e.g. sockets=0) or just not explicitly specified, then we compute the ommited values. We probably should also treat "maxcpus=0" as ommited and then set the value to match smp cpus. This makes cmdlines like "-smp 8, maxcpus=0" start to work as "-smp 8,maxcpus=8,sockets=8,cores=1,threads=1". Note that this patch won't affect any existing working cmdlines, but will allow configuration like "-smp cpus=n,maxcpus=0" to be valid. Signed-off-by: Yanan Wang <wangyanan55@huawei.com> --- hw/core/machine.c | 14 ++++++++------ hw/i386/pc.c | 14 ++++++++------ 2 files changed, 16 insertions(+), 12 deletions(-)