qemu: Fix setting of memory tunables

Refactoring done in 19c6ad9ac7 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

src/libvirt_private.syms

@@ -1836,6 +1836,7 @@ safezero;
 virArgvToString;
 virAsprintf;
 virBuildPathInternal;
+virCompareLimitUlong;
 virDirCreate;
 virDoubleToStr;
 virEnumFromString;

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,63 +7157,64 @@ 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;
     }
 
-    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;
-        }
-
-        if (flags & VIR_DOMAIN_AFFECT_CONFIG)
-            persistentDef->mem.hard_limit = memory_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_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;
-        }
+    /* Soft limit doesn't clash with the others */
+    QEMU_SET_MEM_PARAMETER(virCgroupSetMemorySoftLimit, soft_limit);
 
-        if (flags & VIR_DOMAIN_AFFECT_CONFIG)
-            persistentDef->mem.soft_limit = memory_soft_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;
     }
 
+    QEMU_SET_MEM_PARAMETER(virCgroupSetMemSwapHardLimit, swap_hard_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)
         goto cleanup;

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;
+}

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__ */