[libvirt] [PATCH] conf: Disallow emulatorpin when numatune's in effect

[Martin Kletzander]

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 at 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 :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150126/420c506f/attachment-0001.sig>