qemu: Fix setting of memory tunables

Refactoring done in 19c6ad9ac7e7eb2fd3c8262bff5f087b508ad07f didn't
correctly take into account the order cgroup limit modification needs to
be done in. This resulted into errors when decreasing the limits.

The operations need to take place in this order:

decrease hard limit
change swap hard limit

or

change swap hard limit
increase hard limit

This patch also fixes the check if the hard_limit is less than
swap_hard_limit to print better error messages. For this purpose I
introduced a helper function virCompareLimitUlong to compare limit
values where value of 0 is equal to unlimited. Additionally the check is
now applied also when the user does not provide all of the tunables
through the API and in that case the currently set values are used.

This patch resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=950478

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9f3a597a55ec3bb1baf48e3cd73aef45859f3963..82fb7ac511da5d979492c4ce459219b58b8aa1e4 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1836,6 +1836,7 @@ safezero;
 virArgvToString;
 virAsprintf;
 virBuildPathInternal;
+virCompareLimitUlong;
 virDirCreate;
 virDoubleToStr;
 virEnumFromString;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0de7ffda76496cc2cc54174d49dfdec231f87970..d23c26ae32a6f629f648ce063d88284fbc8193f2 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7103,11 +7103,11 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
     virDomainDefPtr persistentDef = NULL;
     virDomainObjPtr vm = NULL;
     unsigned long long swap_hard_limit;
-    unsigned long long memory_hard_limit;
-    unsigned long long memory_soft_limit;
+    unsigned long long hard_limit = 0;
+    unsigned long long soft_limit = 0;
     bool set_swap_hard_limit = false;
-    bool set_memory_hard_limit = false;
-    bool set_memory_soft_limit = false;
+    bool set_hard_limit = false;
+    bool set_soft_limit = false;
     virQEMUDriverConfigPtr cfg = NULL;
     int ret = -1;
     int rc;
@@ -7157,62 +7157,63 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
         set_ ## VALUE = true;
 
     VIR_GET_LIMIT_PARAMETER(VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, swap_hard_limit)
-    VIR_GET_LIMIT_PARAMETER(VIR_DOMAIN_MEMORY_HARD_LIMIT, memory_hard_limit)
-    VIR_GET_LIMIT_PARAMETER(VIR_DOMAIN_MEMORY_SOFT_LIMIT, memory_soft_limit)
+    VIR_GET_LIMIT_PARAMETER(VIR_DOMAIN_MEMORY_HARD_LIMIT, hard_limit)
+    VIR_GET_LIMIT_PARAMETER(VIR_DOMAIN_MEMORY_SOFT_LIMIT, soft_limit)
 
 #undef VIR_GET_LIMIT_PARAMETER
 
+    /* Swap hard limit must be greater than hard limit.
+     * Note that limit of 0 denotes unlimited */
+    if (set_swap_hard_limit || set_hard_limit) {
+        unsigned long long mem_limit = vm->def->mem.hard_limit;
+        unsigned long long swap_limit = vm->def->mem.swap_hard_limit;
 
-    /* It will fail if hard limit greater than swap hard limit anyway */
-    if (set_swap_hard_limit && set_memory_hard_limit &&
-        memory_hard_limit > swap_hard_limit) {
-        virReportError(VIR_ERR_INVALID_ARG, "%s",
-                       _("memory hard_limit tunable value must be lower than "
-                         "swap_hard_limit"));
-        goto cleanup;
-    }
+        if (set_swap_hard_limit)
+            swap_limit = swap_hard_limit;
 
-    if (set_swap_hard_limit) {
-        if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-            if ((rc = virCgroupSetMemSwapHardLimit(priv->cgroup, swap_hard_limit)) < 0) {
-                virReportSystemError(-rc, "%s",
-                                     _("unable to set memory swap_hard_limit tunable"));
-                goto cleanup;
-            }
-            vm->def->mem.swap_hard_limit = swap_hard_limit;
+        if (set_hard_limit)
+            mem_limit = hard_limit;
+
+        if (virCompareLimitUlong(mem_limit, swap_limit) > 0) {
+            virReportError(VIR_ERR_INVALID_ARG, "%s",
+                           _("memory hard_limit tunable value must be lower "
+                             "than swap_hard_limit"));
+            goto cleanup;
         }
+    }
 
-        if (flags & VIR_DOMAIN_AFFECT_CONFIG)
-            persistentDef->mem.swap_hard_limit = swap_hard_limit;
+#define QEMU_SET_MEM_PARAMETER(FUNC, VALUE)                                     \
+    if (set_ ## VALUE) {                                                        \
+        if (flags & VIR_DOMAIN_AFFECT_LIVE) {                                   \
+            if ((rc = FUNC(priv->cgroup, VALUE)) < 0) {                         \
+                virReportSystemError(-rc, _("unable to set memory %s tunable"), \
+                                     #VALUE);                                   \
+                                                                                \
+                goto cleanup;                                                   \
+            }                                                                   \
+            vm->def->mem.VALUE = VALUE;                                         \
+        }                                                                       \
+                                                                                \
+        if (flags & VIR_DOMAIN_AFFECT_CONFIG)                                   \
+            persistentDef->mem.VALUE = VALUE;                                   \
     }
 
-    if (set_memory_hard_limit) {
-        if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-            if ((rc = virCgroupSetMemoryHardLimit(priv->cgroup, memory_hard_limit)) < 0) {
-                virReportSystemError(-rc, "%s",
-                                     _("unable to set memory hard_limit tunable"));
-                goto cleanup;
-            }
-            vm->def->mem.hard_limit = memory_hard_limit;
-        }
+    /* Soft limit doesn't clash with the others */
+    QEMU_SET_MEM_PARAMETER(virCgroupSetMemorySoftLimit, soft_limit);
 
-        if (flags & VIR_DOMAIN_AFFECT_CONFIG)
-            persistentDef->mem.hard_limit = memory_hard_limit;
+    /* set hard limit before swap hard limit if decreasing it */
+    if (virCompareLimitUlong(vm->def->mem.hard_limit, hard_limit) > 0) {
+        QEMU_SET_MEM_PARAMETER(virCgroupSetMemoryHardLimit, hard_limit);
+        /* inhibit changing the limit a second time */
+        set_hard_limit = false;
     }
 
-    if (set_memory_soft_limit) {
-        if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-            if ((rc = virCgroupSetMemorySoftLimit(priv->cgroup, memory_soft_limit)) < 0) {
-                virReportSystemError(-rc, "%s",
-                                     _("unable to set memory soft_limit tunable"));
-                goto cleanup;
-            }
-            vm->def->mem.soft_limit = memory_soft_limit;
-        }
+    QEMU_SET_MEM_PARAMETER(virCgroupSetMemSwapHardLimit, swap_hard_limit);
 
-        if (flags & VIR_DOMAIN_AFFECT_CONFIG)
-            persistentDef->mem.soft_limit = memory_soft_limit;
-    }
+    /* otherwise increase it after swap hard limit */
+    QEMU_SET_MEM_PARAMETER(virCgroupSetMemoryHardLimit, hard_limit);
+
+#undef QEMU_SET_MEM_PARAMETER
 
     if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
         virDomainSaveConfig(cfg->configDir, persistentDef) < 0)

diff --git a/src/util/virutil.c b/src/util/virutil.c
index 9cc36721f52dcc4dc9e8c6bebaff54fd8d8c22a8..b9de33c4eb929466e3ac6668546a524508e9db96 100644 (file)
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -3832,3 +3832,23 @@ virFindFCHostCapableVport(const char *sysfs_prefix ATTRIBUTE_UNUSED)
 }
 
 #endif /* __linux__ */
+
+/**
+ * virCompareLimitUlong:
+ *
+ * Compare two unsigned long long numbers. Value '0' of the arguments has a
+ * special meaning of 'unlimited' and thus greater than any other value.
+ *
+ * Returns 0 if the numbers are equal, -1 if b is greater, 1 if a is greater.
+ */
+int
+virCompareLimitUlong(unsigned long long a, unsigned long b)
+{
+    if (a == b)
+        return 0;
+
+    if (a == 0 || a > b)
+        return 1;
+
+    return -1;
+}

diff --git a/src/util/virutil.h b/src/util/virutil.h
index 7b37d65cff18f997fcc10c152d1be9862ba2d272..39033db780c9bea4cf786b6af7cda7d7400fca2e 100644 (file)
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -324,4 +324,6 @@ char *virGetFCHostNameByWWN(const char *sysfs_prefix,
 
 char *virFindFCHostCapableVport(const char *sysfs_prefix);
 
+int virCompareLimitUlong(unsigned long long a, unsigned long b);
+
 #endif /* __VIR_UTIL_H__ */