Re: [PATCH 3/3] memtune: change the way how we store unlimited value

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]