On Wed, Mar 04, 2015 at 17:17:07 +0100, Pavel Hrdina wrote:
There was a mess in the way how we store unlimited value for memory limits and how we handled values provided by user. Internally there were two possible ways how to store unlimited value: as 0 value or as VIR_DOMAIN_MEMORY_PARAM_UNLIMITED. Because we chose to store memory limits as unsigned long long, we cannot use -1 to represent unlimited. It's much easier for us to say that everything greater than VIR_DOMAIN_MEMORY_PARAM_UNLIMITED means unlimited and leave 0 as valid value despite that it makes no sense to set limit to 0.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1146539
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.html.in | 4 +- src/conf/domain_conf.c | 75 +++++++++++++++++----- src/libvirt-domain.c | 3 + src/lxc/lxc_cgroup.c | 18 +++--- src/lxc/lxc_driver.c | 3 - src/lxc/lxc_fuse.c | 12 ++-- src/lxc/lxc_native.c | 6 +- src/openvz/openvz_conf.c | 4 +- src/openvz/openvz_driver.c | 4 +- src/qemu/qemu_cgroup.c | 24 +++---- src/qemu/qemu_command.c | 8 ++- src/qemu/qemu_driver.c | 6 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 5 +- src/util/virutil.c | 8 +-- tests/qemuxml2argvdata/qemuxml2argv-memtune.xml | 2 +- .../qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml | 2 +- 17 files changed, 116 insertions(+), 70 deletions(-)
...
+/** + * virDomainParseMemoryLimit: + * + * @xpath: XPath to memory amount + * @units_xpath: XPath to units attribute + * @ctxt: XPath context + * @mem: scaled memory amount is stored here + * + * Parse a memory element or attribute located at @xpath within @ctxt, and + * store the result into @mem, in blocks of 1024. The value is scaled by + * units located at @units_xpath (or the 'unit' attribute under @xpath if + * @units_xpath is NULL). If units are not present, he default scale of 1024 + * is used. The value must not exceed VIR_DOMAIN_MEMORY_PARAM_UNLIMITED + * once scaled. + * + * This helper should be used only on *_limit memory elements. + * + * Return 0 on success, -1 on failure after issuing error. + */ +static int +virDomainParseMemoryLimit(const char *xpath, + const char *units_xpath, + xmlXPathContextPtr ctxt, + unsigned long long *mem) +{ + int ret; + unsigned long long bytes; + + ret = virDomainParseScaledValue(xpath, units_xpath, ctxt, &bytes, 1024, + VIR_DOMAIN_MEMORY_PARAM_UNLIMITED << 10, + false); + + if (ret < 0) + return -1; + + if (ret == 0) + *mem = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + else + VIR_SET_MEMORY_LIMIT(*mem, VIR_DIV_UP(bytes, 1024));
As said in previous patch, this looks very opaque. Having it as *mem = virMemoryTruncate(VIR_DIV_UP(bytes, 1024); would be better. (the name is subject to change).
+ + return 0; +} + +
....
- if (def->mem.hard_limit && - virCgroupSetMemoryHardLimit(cgroup, def->mem.hard_limit) < 0) - goto cleanup; + if (def->mem.hard_limit < VIR_DOMAIN_MEMORY_PARAM_UNLIMITED)
Perhaps adding a function virMemoryLimitIsSet(def->mem.hard_limit) would also make things more clear too.
+ if (virCgroupSetMemoryHardLimit(cgroup, def->mem.hard_limit) < 0) + goto cleanup;
- if (def->mem.soft_limit && - virCgroupSetMemorySoftLimit(cgroup, def->mem.soft_limit) < 0) - goto cleanup; + if (def->mem.soft_limit < VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + if (virCgroupSetMemorySoftLimit(cgroup, def->mem.soft_limit) < 0) + goto cleanup;
- if (def->mem.swap_hard_limit && - virCgroupSetMemSwapHardLimit(cgroup, def->mem.swap_hard_limit) < 0) - goto cleanup; + if (def->mem.swap_hard_limit < VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + if (virCgroupSetMemSwapHardLimit(cgroup, def->mem.swap_hard_limit) < 0) + goto cleanup;
ret = 0; cleanup:
...
diff --git a/src/util/virutil.c b/src/util/virutil.c index 4a95292..02ad626 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2367,8 +2367,7 @@ virFindFCHostCapableVport(const char *sysfs_prefix ATTRIBUTE_UNUSED) /** * 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. + * Compare two unsigned long long numbers. * * Returns 0 if the numbers are equal, -1 if b is greater, 1 if a is greater. */ @@ -2378,10 +2377,7 @@ virCompareLimitUlong(unsigned long long a, unsigned long long b) if (a == b) return 0;
- if (!b) - return -1; - - if (a == 0 || a > b) + if (a > b) return 1;
This whole function now doesn't make sense. Please remove it.
return -1; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml b/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml index 1a244f0..f838d95 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml @@ -5,7 +5,7 @@ <currentMemory unit='KiB'>219136</currentMemory> <memtune> <hard_limit unit='KiB'>512000</hard_limit> - <soft_limit unit='bytes'>131071999</soft_limit> + <soft_limit unit='bytes'>0</soft_limit>
This is for demonstration purposes?
<swap_hard_limit unit='KB'>1048576</swap_hard_limit> </memtune> <vcpu placement='static'>1</vcpu> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml index 92dcacf..07989d1 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml @@ -5,7 +5,7 @@ <currentMemory unit='KiB'>219136</currentMemory> <memtune> <hard_limit unit='KiB'>512000</hard_limit> - <soft_limit unit='KiB'>128000</soft_limit> + <soft_limit unit='KiB'>0</soft_limit> <swap_hard_limit unit='KiB'>1024000</swap_hard_limit> </memtune> <vcpu placement='static'>1</vcpu>
Looks good otherwise. Peter