[libvirt] [PATCH 3/3] memtune: change the way how we store unlimited value
Peter Krempa
pkrempa at redhat.com
Thu Mar 5 09:25:45 UTC 2015
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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150305/3b7ec7a9/attachment-0001.sig>
More information about the libvir-list
mailing list