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

[libvirt] [PATCH 1/2] virDomainNumatuneGetMode: Report if numatune was defined



So far, we are not reporting if numatune was even defined. The
value of zero is blindly returned (which maps onto
VIR_DOMAIN_NUMATUNE_MEM_STRICT). Unfortunately, we are making
decisions based on this value. Instead, we should not inly return
the correct value, but report to the caller if the value is valid
at all.

Signed-off-by: Michal Privoznik <mprivozn redhat com>
---

Notes:
    I know, qemuDomainGetNumaParameters() is not fixed yet, that's what the very
    next patch does.  For better view on this patch use --ignore-space-change.

 src/conf/numa_conf.c          | 38 +++++++++++++++++++++++++++++---------
 src/conf/numa_conf.h          |  5 +++--
 src/lxc/lxc_cgroup.c          |  5 +++--
 src/lxc/lxc_controller.c      | 30 +++++++++++++++---------------
 src/parallels/parallels_sdk.c |  5 +++--
 src/qemu/qemu_cgroup.c        | 15 +++++++++------
 src/qemu/qemu_command.c       |  4 ++--
 src/qemu/qemu_driver.c        | 22 ++++++++++++++--------
 src/qemu/qemu_process.c       | 28 ++++++++++++++--------------
 9 files changed, 92 insertions(+), 60 deletions(-)

diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index e4dc2f8..57da215 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -351,20 +351,40 @@ virDomainNumaFree(virDomainNumaPtr numa)
     VIR_FREE(numa);
 }
 
-virDomainNumatuneMemMode
-virDomainNumatuneGetMode(virDomainNumaPtr numatune,
-                         int cellid)
+/**
+ * virDomainNumatuneGetMode:
+ * @numatune: pointer to numatune definition
+ * @cellid: cell selector
+ * @mode: where to store the result
+ *
+ * Get the defined mode for domain's memory. It's safe to pass
+ * NULL to @mode if the return value is the only info needed.
+ *
+ * Returns: 0 on success (with @mode updated)
+ *         -1 if no mode was defined in XML
+ */
+int virDomainNumatuneGetMode(virDomainNumaPtr numatune,
+                             int cellid,
+                             virDomainNumatuneMemMode *mode)
 {
+    int ret = -1;
+    virDomainNumatuneMemMode tmp_mode;
+
     if (!numatune)
-        return 0;
+        return ret;
 
     if (virDomainNumatuneNodeSpecified(numatune, cellid))
-        return numatune->mem_nodes[cellid].mode;
+        tmp_mode = numatune->mem_nodes[cellid].mode;
+    else if (numatune->memory.specified)
+        tmp_mode = numatune->memory.mode;
+    else
+        goto cleanup;
 
-    if (numatune->memory.specified)
-        return numatune->memory.mode;
-
-    return 0;
+    if (mode)
+        *mode = tmp_mode;
+    ret = 0;
+ cleanup:
+    return ret;
 }
 
 virBitmapPtr
diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h
index ded6e01..6739065 100644
--- a/src/conf/numa_conf.h
+++ b/src/conf/numa_conf.h
@@ -72,8 +72,9 @@ int virDomainNumatuneFormatXML(virBufferPtr buf, virDomainNumaPtr numatune)
 /*
  * Getters
  */
-virDomainNumatuneMemMode virDomainNumatuneGetMode(virDomainNumaPtr numatune,
-                                                  int cellid);
+int virDomainNumatuneGetMode(virDomainNumaPtr numatune,
+                             int cellid,
+                             virDomainNumatuneMemMode *mode);
 
 virBitmapPtr virDomainNumatuneGetNodeset(virDomainNumaPtr numatune,
                                          virBitmapPtr auto_nodeset,
diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index 5e959a2..507d567 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -69,6 +69,7 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def,
 {
     int ret = -1;
     char *mask = NULL;
+    virDomainNumatuneMemMode mode;
 
     if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO &&
         def->cpumask) {
@@ -81,8 +82,8 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def,
         VIR_FREE(mask);
     }
 
-    if (virDomainNumatuneGetMode(def->numa, -1) !=
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
+    if (virDomainNumatuneGetMode(def->numa, -1, &mode) < 0 ||
+        mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
         ret = 0;
         goto cleanup;
     }
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index ba44e09..efbe71f 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -741,25 +741,25 @@ static int virLXCControllerSetupResourceLimits(virLXCControllerPtr ctrl)
     virBitmapPtr nodeset = NULL;
     virDomainNumatuneMemMode mode;
 
-    mode = virDomainNumatuneGetMode(ctrl->def->numa, -1);
+    if (virDomainNumatuneGetMode(ctrl->def->numa, -1, &mode) == 0) {
+        if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+            virCgroupControllerAvailable(VIR_CGROUP_CONTROLLER_CPUSET)) {
+            /* Use virNuma* API iff necessary. Once set and child is exec()-ed,
+             * there's no way for us to change it. Rely on cgroups (if available
+             * and enabled in the config) rather than virNuma*. */
+            VIR_DEBUG("Relying on CGroups for memory binding");
+        } else {
 
-    if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
-        virCgroupControllerAvailable(VIR_CGROUP_CONTROLLER_CPUSET)) {
-        /* Use virNuma* API iff necessary. Once set and child is exec()-ed,
-         * there's no way for us to change it. Rely on cgroups (if available
-         * and enabled in the config) rather than virNuma*. */
-        VIR_DEBUG("Relying on CGroups for memory binding");
-    } else {
+            VIR_DEBUG("Setting up process resource limits");
 
-        VIR_DEBUG("Setting up process resource limits");
+            if (virLXCControllerGetNumadAdvice(ctrl, &auto_nodeset) < 0)
+                goto cleanup;
 
-        if (virLXCControllerGetNumadAdvice(ctrl, &auto_nodeset) < 0)
-            goto cleanup;
+            nodeset = virDomainNumatuneGetNodeset(ctrl->def->numa, auto_nodeset, -1);
 
-        nodeset = virDomainNumatuneGetNodeset(ctrl->def->numa, auto_nodeset, -1);
-
-        if (virNumaSetupMemoryPolicy(mode, nodeset) < 0)
-            goto cleanup;
+            if (virNumaSetupMemoryPolicy(mode, nodeset) < 0)
+                goto cleanup;
+        }
     }
 
     if (virLXCControllerSetupCpuAffinity(ctrl) < 0)
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c
index 786e0ad..2a44504 100644
--- a/src/parallels/parallels_sdk.c
+++ b/src/parallels/parallels_sdk.c
@@ -1845,6 +1845,7 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def)
     size_t i;
     PRL_VM_TYPE vmType;
     PRL_RESULT pret;
+    virDomainNumatuneMemMode memMode;
 
     if (def->title) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -1924,8 +1925,8 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def)
      * virDomainDefPtr always contain non zero NUMA configuration
      * So, just make sure this configuration does't differ from auto generated.
      */
-    if ((virDomainNumatuneGetMode(def->numa, -1) !=
-         VIR_DOMAIN_NUMATUNE_MEM_STRICT) ||
+    if ((virDomainNumatuneGetMode(def->numa, -1, &memMode) == 0 &&
+         memMode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) ||
          virDomainNumatuneHasPerNodeBinding(def->numa)) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                         _("numa parameters are not supported "
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index e24989b..96677dd 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -613,14 +613,15 @@ qemuSetupCpusetMems(virDomainObjPtr vm)
 {
     virCgroupPtr cgroup_temp = NULL;
     qemuDomainObjPrivatePtr priv = vm->privateData;
+    virDomainNumatuneMemMode mode;
     char *mem_mask = NULL;
     int ret = -1;
 
     if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET))
         return 0;
 
-    if (virDomainNumatuneGetMode(vm->def->numa, -1) !=
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT)
+    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mode) < 0 ||
+        mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT)
         return 0;
 
     if (virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
@@ -979,6 +980,7 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
     unsigned long long period = vm->def->cputune.period;
     long long quota = vm->def->cputune.quota;
     char *mem_mask = NULL;
+    virDomainNumatuneMemMode mem_mode;
 
     if ((period || quota) &&
         !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
@@ -1009,8 +1011,8 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
         return 0;
     }
 
-    if (virDomainNumatuneGetMode(vm->def->numa, -1) ==
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 &&
+        mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
         virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
                                             priv->autoNodeset,
                                             &mem_mask, -1) < 0)
@@ -1153,6 +1155,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
     unsigned long long period = vm->def->cputune.period;
     long long quota = vm->def->cputune.quota;
     char *mem_mask = NULL;
+    virDomainNumatuneMemMode mem_mode;
 
     if ((period || quota) &&
         !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
@@ -1176,8 +1179,8 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
     if (priv->cgroup == NULL)
         return 0;
 
-    if (virDomainNumatuneGetMode(vm->def->numa, -1) ==
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 &&
+        mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
         virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
                                             priv->autoNodeset,
                                             &mem_mask, -1) < 0)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 3ccd35d..6ecc9de 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4707,7 +4707,6 @@ qemuBuildMemoryBackendStr(unsigned long long size,
         return -1;
 
     memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, guestNode);
-    mode = virDomainNumatuneGetMode(def->numa, guestNode);
 
     if (pagesize == 0 || pagesize != system_page_size) {
         /* Find the huge page size we want to use */
@@ -4823,7 +4822,8 @@ qemuBuildMemoryBackendStr(unsigned long long size,
             goto cleanup;
     }
 
-    if (nodemask) {
+    if (nodemask &&
+        virDomainNumatuneGetMode(def->numa, guestNode, &mode) == 0) {
         if (!virNumaNodesetIsAvailable(nodemask))
             goto cleanup;
         if (virJSONValueObjectAdd(props,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0cc0a29..e7f235b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4737,6 +4737,7 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
     int ncpupids;
     virCgroupPtr cgroup_vcpu = NULL;
     char *mem_mask = NULL;
+    virDomainNumatuneMemMode mem_mode;
 
     qemuDomainObjEnterMonitor(driver, vm);
 
@@ -4804,8 +4805,8 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
-    if (virDomainNumatuneGetMode(vm->def->numa, -1) ==
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 &&
+        mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
         virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
                                             priv->autoNodeset,
                                             &mem_mask, -1) < 0)
@@ -6113,6 +6114,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,
     qemuMonitorIOThreadInfoPtr *new_iothreads = NULL;
     virCgroupPtr cgroup_iothread = NULL;
     char *mem_mask = NULL;
+    virDomainNumatuneMemMode mode;
     virDomainIOThreadIDDefPtr iothrid;
     virBitmapPtr cpumask;
 
@@ -6154,8 +6156,8 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,
     }
     vm->def->iothreads = exp_niothreads;
 
-    if (virDomainNumatuneGetMode(vm->def->numa, -1) ==
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mode) == 0 &&
+        mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
         virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
                                             priv->autoNodeset,
                                             &mem_mask, -1) < 0)
@@ -10330,11 +10332,12 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
     virCgroupPtr cgroup_temp = NULL;
     qemuDomainObjPrivatePtr priv = vm->privateData;
     char *nodeset_str = NULL;
+    virDomainNumatuneMemMode mode;
     size_t i = 0;
     int ret = -1;
 
-    if (virDomainNumatuneGetMode(vm->def->numa, -1) !=
-        VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
+    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mode) < 0 ||
+        mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                        _("change of nodeset for running domain "
                          "requires strict numa mode"));
@@ -10391,6 +10394,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
     virCapsPtr caps = NULL;
     qemuDomainObjPrivatePtr priv;
     virBitmapPtr nodeset = NULL;
+    virDomainNumatuneMemMode config_mode;
     int mode = -1;
 
     virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
@@ -10466,7 +10470,8 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
 
     if (flags & VIR_DOMAIN_AFFECT_LIVE) {
         if (mode != -1 &&
-            virDomainNumatuneGetMode(vm->def->numa, -1) != mode) {
+            virDomainNumatuneGetMode(vm->def->numa, -1, &config_mode) == 0 &&
+            config_mode != mode) {
             virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                            _("can't change numatune mode for running domain"));
             goto endjob;
@@ -10567,7 +10572,8 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
                                         VIR_TYPED_PARAM_INT, 0) < 0)
                 goto cleanup;
 
-            param->value.i = virDomainNumatuneGetMode(def->numa, -1);
+            virDomainNumatuneGetMode(def->numa, -1,
+                                     (virDomainNumatuneMemMode *) &param->value.i);
             break;
 
         case 1: /* fill numa nodeset here */
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 2b3d9b5..118fc52 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3135,21 +3135,21 @@ static int qemuProcessHook(void *data)
     if (virSecurityManagerClearSocketLabel(h->driver->securityManager, h->vm->def) < 0)
         goto cleanup;
 
-    mode = virDomainNumatuneGetMode(h->vm->def->numa, -1);
+    if (virDomainNumatuneGetMode(h->vm->def->numa, -1, &mode) == 0) {
+        if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
+            h->cfg->cgroupControllers & (1 << VIR_CGROUP_CONTROLLER_CPUSET) &&
+            virCgroupControllerAvailable(VIR_CGROUP_CONTROLLER_CPUSET)) {
+            /* Use virNuma* API iff necessary. Once set and child is exec()-ed,
+             * there's no way for us to change it. Rely on cgroups (if available
+             * and enabled in the config) rather than virNuma*. */
+            VIR_DEBUG("Relying on CGroups for memory binding");
+        } else {
+            nodeset = virDomainNumatuneGetNodeset(h->vm->def->numa,
+                                                  priv->autoNodeset, -1);
 
-    if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
-        h->cfg->cgroupControllers & (1 << VIR_CGROUP_CONTROLLER_CPUSET) &&
-        virCgroupControllerAvailable(VIR_CGROUP_CONTROLLER_CPUSET)) {
-        /* Use virNuma* API iff necessary. Once set and child is exec()-ed,
-         * there's no way for us to change it. Rely on cgroups (if available
-         * and enabled in the config) rather than virNuma*. */
-        VIR_DEBUG("Relying on CGroups for memory binding");
-    } else {
-        nodeset = virDomainNumatuneGetNodeset(h->vm->def->numa,
-                                              priv->autoNodeset, -1);
-
-        if (virNumaSetupMemoryPolicy(mode, nodeset) < 0)
-            goto cleanup;
+            if (virNumaSetupMemoryPolicy(mode, nodeset) < 0)
+                goto cleanup;
+        }
     }
 
     ret = 0;
-- 
2.3.6


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