| Message ID | 1449728144-6223-2-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Dec 10, 2015 at 11:45:36AM +0530, Bharata B Rao wrote: > Prevent guests from booting with CPU topologies that have partially > filled CPU cores or can result in partially filled CPU cores after CPU > hotplug like > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17 or > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > vl.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/vl.c b/vl.c > index 525929b..e656f53 100644 > --- a/vl.c > +++ b/vl.c > @@ -1252,6 +1252,19 @@ static void smp_parse(QemuOpts *opts) > smp_cores = cores > 0 ? cores : 1; > smp_threads = threads > 0 ? threads : 1; > > + if (smp_cpus % smp_threads) { > + error_report("cpu topology: " > + "smp_cpus (%u) should be multiple of threads (%u)", > + smp_cpus, smp_threads); > + exit(1); > + } > + > + if (max_cpus % smp_threads) { > + error_report("cpu topology: " > + "maxcpus (%u) should be multiple of threads (%u)", > + max_cpus, smp_threads); > + exit(1); > + } > } Adding this seems like it has a pretty high chance of causing regression, ie preventing previously working guests from booting with new QEMU. I know adding the check makes sense from a semantic POV, but are we willing to risk breaking people with such odd configurations ? Regards, Daniel
On Thu, Dec 10, 2015 at 10:25:28AM +0000, Daniel P. Berrange wrote: > On Thu, Dec 10, 2015 at 11:45:36AM +0530, Bharata B Rao wrote: > > Prevent guests from booting with CPU topologies that have partially > > filled CPU cores or can result in partially filled CPU cores after CPU > > hotplug like > > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17 or > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > --- > > vl.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/vl.c b/vl.c > > index 525929b..e656f53 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -1252,6 +1252,19 @@ static void smp_parse(QemuOpts *opts) > > smp_cores = cores > 0 ? cores : 1; > > smp_threads = threads > 0 ? threads : 1; > > > > + if (smp_cpus % smp_threads) { > > + error_report("cpu topology: " > > + "smp_cpus (%u) should be multiple of threads (%u)", > > + smp_cpus, smp_threads); > > + exit(1); > > + } > > + > > + if (max_cpus % smp_threads) { > > + error_report("cpu topology: " > > + "maxcpus (%u) should be multiple of threads (%u)", > > + max_cpus, smp_threads); > > + exit(1); > > + } > > } > > Adding this seems like it has a pretty high chance of causing regression, > ie preventing previously working guests from booting with new QEMU. I > know adding the check makes sense from a semantic POV, but are we willing > to risk breaking people with such odd configurations ? I wasn't sure about how much risk that would be and hence in my older version of PowerPC CPU hotplug patchset, I indeed supported such topologies: https://lists.gnu.org/archive/html/qemu-ppc/2015-09/msg00102.html But the code indeed looked ugly to support such special case. There was some discussion about this recently here: http://lists.gnu.org/archive/html/qemu-devel/2015-12/msg00396.html from where I sensed that it may be ok to dis-allow such topologies. Regards, Bharata.
On Fri, Dec 11, 2015 at 08:54:31AM +0530, Bharata B Rao wrote: > On Thu, Dec 10, 2015 at 10:25:28AM +0000, Daniel P. Berrange wrote: > > On Thu, Dec 10, 2015 at 11:45:36AM +0530, Bharata B Rao wrote: > > > Prevent guests from booting with CPU topologies that have partially > > > filled CPU cores or can result in partially filled CPU cores after CPU > > > hotplug like > > > > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17 or > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > --- > > > vl.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/vl.c b/vl.c > > > index 525929b..e656f53 100644 > > > --- a/vl.c > > > +++ b/vl.c > > > @@ -1252,6 +1252,19 @@ static void smp_parse(QemuOpts *opts) > > > smp_cores = cores > 0 ? cores : 1; > > > smp_threads = threads > 0 ? threads : 1; > > > > > > + if (smp_cpus % smp_threads) { > > > + error_report("cpu topology: " > > > + "smp_cpus (%u) should be multiple of threads (%u)", > > > + smp_cpus, smp_threads); > > > + exit(1); > > > + } > > > + > > > + if (max_cpus % smp_threads) { > > > + error_report("cpu topology: " > > > + "maxcpus (%u) should be multiple of threads (%u)", > > > + max_cpus, smp_threads); > > > + exit(1); > > > + } > > > } > > > > Adding this seems like it has a pretty high chance of causing regression, > > ie preventing previously working guests from booting with new QEMU. I > > know adding the check makes sense from a semantic POV, but are we willing > > to risk breaking people with such odd configurations ? > > I wasn't sure about how much risk that would be and hence in my older > version of PowerPC CPU hotplug patchset, I indeed supported such topologies: > > https://lists.gnu.org/archive/html/qemu-ppc/2015-09/msg00102.html > > But the code indeed looked ugly to support such special case. > > There was some discussion about this recently here: > > http://lists.gnu.org/archive/html/qemu-devel/2015-12/msg00396.html > > from where I sensed that it may be ok to dis-allow such topologies. I want to be as strict as possible and disallow such topologies, but Daniel has a point. Maybe we should make those checks machine-specific, so we can make pc-*-2.5 and older allow those broken configs. If we make it a MachineClass::validate_smp_config() method, for example, we could make TYPE_MACHINE point to a generic function containing the checks you implemented above (so all machines have those checks enabled by default), but let pc <= 2.5 override the method.
On Mon, Dec 14, 2015 at 03:37:52PM -0200, Eduardo Habkost wrote: > On Fri, Dec 11, 2015 at 08:54:31AM +0530, Bharata B Rao wrote: > > On Thu, Dec 10, 2015 at 10:25:28AM +0000, Daniel P. Berrange wrote: > > > On Thu, Dec 10, 2015 at 11:45:36AM +0530, Bharata B Rao wrote: > > > > Prevent guests from booting with CPU topologies that have partially > > > > filled CPU cores or can result in partially filled CPU cores after CPU > > > > hotplug like > > > > > > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or > > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17 or > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > --- > > > > vl.c | 13 +++++++++++++ > > > > 1 file changed, 13 insertions(+) > > > > > > > > diff --git a/vl.c b/vl.c > > > > index 525929b..e656f53 100644 > > > > --- a/vl.c > > > > +++ b/vl.c > > > > @@ -1252,6 +1252,19 @@ static void smp_parse(QemuOpts *opts) > > > > smp_cores = cores > 0 ? cores : 1; > > > > smp_threads = threads > 0 ? threads : 1; > > > > > > > > + if (smp_cpus % smp_threads) { > > > > + error_report("cpu topology: " > > > > + "smp_cpus (%u) should be multiple of threads (%u)", > > > > + smp_cpus, smp_threads); > > > > + exit(1); > > > > + } > > > > + > > > > + if (max_cpus % smp_threads) { > > > > + error_report("cpu topology: " > > > > + "maxcpus (%u) should be multiple of threads (%u)", > > > > + max_cpus, smp_threads); > > > > + exit(1); > > > > + } > > > > } > > > > > > Adding this seems like it has a pretty high chance of causing regression, > > > ie preventing previously working guests from booting with new QEMU. I > > > know adding the check makes sense from a semantic POV, but are we willing > > > to risk breaking people with such odd configurations ? > > > > I wasn't sure about how much risk that would be and hence in my older > > version of PowerPC CPU hotplug patchset, I indeed supported such topologies: > > > > https://lists.gnu.org/archive/html/qemu-ppc/2015-09/msg00102.html > > > > But the code indeed looked ugly to support such special case. > > > > There was some discussion about this recently here: > > > > http://lists.gnu.org/archive/html/qemu-devel/2015-12/msg00396.html > > > > from where I sensed that it may be ok to dis-allow such topologies. > > I want to be as strict as possible and disallow such topologies, > but Daniel has a point. Maybe we should make those checks > machine-specific, so we can make pc-*-2.5 and older allow those > broken configs. > > If we make it a MachineClass::validate_smp_config() method, for > example, we could make TYPE_MACHINE point to a generic function > containing the checks you implemented above (so all machines have > those checks enabled by default), but let pc <= 2.5 override the > method. Nice suggestion, will give it a try in the next iteration. Regards, Bharata.
diff --git a/vl.c b/vl.c index 525929b..e656f53 100644 --- a/vl.c +++ b/vl.c @@ -1252,6 +1252,19 @@ static void smp_parse(QemuOpts *opts) smp_cores = cores > 0 ? cores : 1; smp_threads = threads > 0 ? threads : 1; + if (smp_cpus % smp_threads) { + error_report("cpu topology: " + "smp_cpus (%u) should be multiple of threads (%u)", + smp_cpus, smp_threads); + exit(1); + } + + if (max_cpus % smp_threads) { + error_report("cpu topology: " + "maxcpus (%u) should be multiple of threads (%u)", + max_cpus, smp_threads); + exit(1); + } } if (max_cpus == 0) {
Prevent guests from booting with CPU topologies that have partially filled CPU cores or can result in partially filled CPU cores after CPU hotplug like -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or -smp 15,sockets=1,cores=4,threads=4,maxcpus=17 or Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- vl.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)