[PATCH v3] conf: Don't mangle vcpu placement randomly

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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>

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@xxxxxxxxxx>
---

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/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d562e1a..706e5d2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13084,28 +13084,20 @@ virDomainDefParseXML(xmlDocPtr xml,
         goto error;
     }
 
-    /* Ignore emulatorpin if <vcpu> placement is "auto", they
-     * conflicts with each other, and <vcpu> placement can't be
-     * simply ignored, as <numatune>'s placement defaults to it.
-     */
     if (n) {
-        if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) {
-            if (n > 1) {
-                virReportError(VIR_ERR_XML_ERROR, "%s",
-                               _("only one emulatorpin is supported"));
-                VIR_FREE(nodes);
-                goto error;
-            }
-
-            def->cputune.emulatorpin = virDomainVcpuPinDefParseXML(nodes[0],
-                                                                   ctxt, 0,
-                                                                   true, false);
-
-            if (!def->cputune.emulatorpin)
-                goto error;
-        } else {
-            VIR_WARN("Ignore emulatorpin for <vcpu> placement is 'auto'");
+        if (n > 1) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("only one emulatorpin is supported"));
+            VIR_FREE(nodes);
+            goto error;
         }
+
+        def->cputune.emulatorpin = virDomainVcpuPinDefParseXML(nodes[0],
+                                                               ctxt, 0,
+                                                               true, false);
+
+        if (!def->cputune.emulatorpin)
+            goto error;
     }
     VIR_FREE(nodes);
 
@@ -13116,38 +13108,28 @@ virDomainDefParseXML(xmlDocPtr xml,
         goto error;
     }
 
-    /* Ignore iothreadpin if <vcpu> placement is "auto", they
-     * conflict with each other, and <vcpu> placement can't be
-     * simply ignored, as <numatune>'s placement defaults to it.
-     */
-    if (n) {
-        if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) {
-            if (VIR_ALLOC_N(def->cputune.iothreadspin, n) < 0)
-                goto error;
+    if (n && VIR_ALLOC_N(def->cputune.iothreadspin, n) < 0)
+        goto error;
 
-            for (i = 0; i < n; i++) {
-                virDomainVcpuPinDefPtr iothreadpin = NULL;
-                iothreadpin = virDomainVcpuPinDefParseXML(nodes[i], ctxt,
-                                                          def->iothreads,
-                                                          false, true);
-                if (!iothreadpin)
-                    goto error;
+    for (i = 0; i < n; i++) {
+        virDomainVcpuPinDefPtr iothreadpin = NULL;
+        iothreadpin = virDomainVcpuPinDefParseXML(nodes[i], ctxt,
+                                                  def->iothreads,
+                                                  false, true);
+        if (!iothreadpin)
+            goto error;
 
-                if (virDomainVcpuPinIsDuplicate(def->cputune.iothreadspin,
-                                                def->cputune.niothreadspin,
-                                                iothreadpin->vcpuid)) {
-                    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                                   _("duplicate iothreadpin for same iothread"));
-                    virDomainVcpuPinDefFree(iothreadpin);
-                    goto error;
-                }
-
-                def->cputune.iothreadspin[def->cputune.niothreadspin++] =
-                    iothreadpin;
-            }
-        } else {
-            VIR_WARN("Ignore iothreadpin for <vcpu> placement is 'auto'");
+        if (virDomainVcpuPinIsDuplicate(def->cputune.iothreadspin,
+                                        def->cputune.niothreadspin,
+                                        iothreadpin->vcpuid)) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("duplicate iothreadpin for same iothread"));
+            virDomainVcpuPinDefFree(iothreadpin);
+            goto error;
         }
+
+        def->cputune.iothreadspin[def->cputune.niothreadspin++] =
+            iothreadpin;
     }
     VIR_FREE(nodes);
 
@@ -13185,7 +13167,9 @@ virDomainDefParseXML(xmlDocPtr xml,
                                   ctxt) < 0)
         goto error;
 
-    if (virDomainNumatuneHasPlacementAuto(def->numatune) && !def->cpumask)
+    if (virDomainNumatuneHasPlacementAuto(def->numatune) &&
+        !def->cpumask && !def->cputune.vcpupin &&
+        !def->cputune.emulatorpin && !def->cputune.iothreadspin)
         def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO;
 
     if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) {
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>
+  <os>
+    <type arch='x86_64' machine='pc-q35-2.3'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='sata' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
+    </controller>
+    <controller type='pci' index='0' model='pcie-root'/>
+    <controller type='pci' index='1' model='dmi-to-pci-bridge'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/>
+    </controller>
+    <controller type='pci' index='2' model='pci-bridge'>
+      <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0'/>
+    </controller>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 4abb303..285538a 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -310,6 +310,7 @@ mymain(void)
     DO_TEST("blkiotune-device");
     DO_TEST("cputune");
     DO_TEST("cputune-zero-shares");
+    DO_TEST("cputune-numatune");
 
     DO_TEST("smp");
     DO_TEST("iothreads");
-- 
2.0.5

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]