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@xxxxxxxxxx> > --- > 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list