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 --- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 95 ++++++++++++++++++++++++------------------------ src/util/virutil.c | 12 ++++++ src/util/virutil.h | 2 + 4 files changed, 63 insertions(+), 47 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 30fdcd7..ec644ee 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1835,6 +1835,7 @@ safezero; virArgvToString; virAsprintf; virBuildPathInternal; +virCompareLimitUlong; virDirCreate; virDoubleToStr; virEnumFromString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cee5557..a174fb0 100644 --- 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 9cc3672..491a004 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -3832,3 +3832,15 @@ virFindFCHostCapableVport(const char *sysfs_prefix ATTRIBUTE_UNUSED) } #endif /* __linux__ */ + +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 7b37d65..39033db 100644 --- 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__ */ -- 1.8.1.5