Re: [libvirt] [PATCH] conf: Disallow emulatorpin when numatune's in effect
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(a)redhat.com>
---
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 :)
Attachments:
- attachment.sig (application/pgp-signature — 819 bytes)