From fa006c4fdd8c8580bca3db1436e3201350e973f4 Mon Sep 17 00:00:00 2001 From: Peter Krempa Date: Wed, 17 Apr 2013 17:50:56 +0200 Subject: [PATCH] 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 --- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 95 ++++++++++++++++++++-------------------- src/util/virutil.c | 20 +++++++++ src/util/virutil.h | 2 + 4 files changed, 71 insertions(+), 47 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9f3a597a55..82fb7ac511 100644 --- 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 0de7ffda76..d23c26ae32 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 9cc36721f5..b9de33c4eb 100644 --- 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 7b37d65cff..39033db780 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__ */ -- GitLab