On Thu, Jan 22, 2015 at 02:08:46PM +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1170492 In one of our previous commits (dc8b7ce7) we've obsoleted <cputune/> in favor of <numatune/> and others. If old element was passed it was
That commit was not trying to obsolete <cputune/>, it was just a re-factor and there should be no behavioural change caused by this commit.
basically ignored and interesting settings were copied from the new one. Well with one exception we'd forgotten about: emulatorpin. Imagine that domain is configured as follows: <vcpu placement='static' current='2'>6</vcpu> <cputune> <emulatorpin cpuset='1-3'/> </cputune> This is perfectly valid as only old style elements are used. However, adding new style elements messes up the XML: <vcpu placement='auto' current='2'>6</vcpu> <cputune> <emulatorpin cpuset='1-3'/>
Well, here you're saying "my foot is on the floor"
</cputune> <numatune> <memory mode='strict' placement='auto'/>
and "shoot somewhere down", so you're most probably going to shoot yourself to the foot.
</numatune> Since <numatune/> is auto, <vcpu/> becomes auto as well. However in that case we can't guarantee that emulator will be pinned onto selected nodes.
We can guarantee that, those elements have completely different meanings. <emupatorpin/> says on which pCPUs the execution can run and <memory/> under <numatune/> says from which host memory NUMA nodes the memory should be allocated.
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
src/conf/domain_conf.c | 12 +++++++-
.../qemuxml2argv-cputune-numatune.xml | 35 ++++++++++++++++++++++
.../qemuxml2xmlout-cputune-numatune.xml | 32 ++++++++++++++++++++
tests/qemuxml2xmltest.c | 1 +
4 files changed, 79 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-numatune.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8792f5e..0b8af6d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13173,8 +13173,18 @@ virDomainDefParseXML(xmlDocPtr xml,
ctxt) < 0)
goto error;
- if (virDomainNumatuneHasPlacementAuto(def->numatune) && !def->cpumask)
+ if (virDomainNumatuneHasPlacementAuto(def->numatune) && !def->cpumask) {
+ /* If numatune is used, it obsoletes some older settings
+ * like /domain/vcpu/@placement or
+ * /domain/cputune/emulatorpin. For more info see comment
+ * a few lines above where emulatorpin is parsed. */
def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO;
+ if (def->cputune.emulatorpin) {
+ VIR_WARN("Ignore emulatorpin for <numatune> placement is 'auto'");
+ virDomainVcpuPinDefFree(def->cputune.emulatorpin);
+ def->cputune.emulatorpin = NULL;
+ }
+ }
This will change this: <vcpu placement='static'>4</vcpu> <cputune> <vcpupin vcpu='0' cpuset='0'/> <emulatorpin cpuset='1'/> </cputune> <numatune> <memory mode='strict' placement='auto'/> </numatune> to: <vcpu placement='static'>4</vcpu> <cputune> <vcpupin vcpu='0' cpuset='0'/> </cputune> <numatune> <memory mode='strict' placement='auto'/> </numatune> And I doubt that's what you were looking for. Either don't change anything the user requested, i.e. set def->placement_mode to VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO if and only if there is no def->cpumask, no vcpupin and no emulatorpin *or* if you want to prevent users shooting themselves into the foot (which we don't do very often) make sure that any 'auto' settings added by them removes any other pinning and sets everything possible to 'auto' in order for the VM to function correctly. If this is not the case and I misunderstood your patch, I'd be glad to discuss that :)
Attachment:
pgpSqIhqErFdJ.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list