Re: [libvirt] [PATCH v3] conf: Don't mangle vcpu placement randomly
On Thu, Jan 29, 2015 at 03:12:51PM +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1170492
In one of our previous commits (dc8b7ce7) we've done a functional
change even though it was intended as pure refactor. The problem is,
that the following XML:
<vcpu placement='static' current='2'>6</vcpu>
<cputune>
<emulatorpin cpuset='1-3'/>
</cputune>
<numatune>
<memory mode='strict' placement='auto'/>
</numatune>
gets translated into this one:
<vcpu placement='auto' current='2'>6</vcpu>
<cputune>
<emulatorpin cpuset='1-3'/>
</cputune>
<numatune>
<memory mode='strict' placement='auto'/>
</numatune>
And on the subsequent edit, the emulatorpin was removed, yes. We
could say that <vcpu placement='auto'/> is used only for vCPUs, but
that would change the current behaviour.
We should not change the vcpu placement mode. Moreover, we're
doing
something similar in case of emulatorpin and iothreadpin. If they were
set, but vcpu placement was auto, we've mistakenly removed them from
the domain XML even though we are able to set them independently on
vcpus.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
diff to v2:
-Martin's review worked in
src/conf/domain_conf.c | 84 +++++++++-------------
.../qemuxml2argv-cputune-numatune.xml | 35 +++++++++
tests/qemuxml2xmltest.c | 1 +
3 files changed, 70 insertions(+), 50 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.xml
[...]
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.xml
b/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.xml
new file mode 100644
index 0000000..9759b48
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.xml
@@ -0,0 +1,35 @@
+<domain type='kvm'>
+ <name>dummy2</name>
+ <uuid>4d92ec27-9ebf-400b-ae91-20c71c647c19</uuid>
+ <memory unit='KiB'>131072</memory>
+ <currentMemory unit='KiB'>65536</currentMemory>
+ <vcpu placement='auto' current='2'>6</vcpu>
+ <cputune>
+ <emulatorpin cpuset='1-3'/>
+ </cputune>
+ <numatune>
+ <memory mode='strict' placement='auto'/>
+ </numatune>
You're testing that we won't remove the emulatorpin, but where's the
test that we won't change the vcpu placement?
ACK with that test added and this diff squashed in:
diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in
index f8d5f89..c5ad6f4 100644
--- i/docs/formatdomain.html.in
+++ w/docs/formatdomain.html.in
@@ -577,14 +577,12 @@
<dt><code>emulatorpin</code></dt>
<dd>
The optional <code>emulatorpin</code> element specifies which of
host
- physical CPUs the "emulator", a subset of a domain not including
vcpu,
- will be pinned to. If this is omitted, and attribute
+ physical CPUs the "emulator", a subset of a domain not including vcpu
+ or iothreads will be pinned to. If this is omitted, and attribute
<code>cpuset</code> of element <code>vcpu</code> is not
specified,
"emulator" is pinned to all the physical CPUs by default. It contains
one required attribute <code>cpuset</code> specifying which
physical
- CPUs to pin to. NB, <code>emulatorpin</code> is not allowed if
- attribute <code>placement</code> of element
<code>vcpu</code> is
- "auto".
+ CPUs to pin to.
</dd>
<dt><code>iothreadpin</code></dt>
<dd>
@@ -598,8 +596,6 @@
<code>iothread</code> value begins at "1" through the
number of
<a
href="#elementsIOThreadsAllocation"><code>iothreads</code></a>
allocated to the domain. A value of "0" is not permitted.
- NB, <code>iothreadpin</code> is not allowed if attribute
- <code>placement</code> of element <code>vcpu</code> is
"auto".
<span class="since">Since 1.2.9</span>
</dd>
<dt><code>shares</code></dt>
--
Attachments:
- attachment.sig (application/pgp-signature — 819 bytes)