[libvirt] [PATCH v4] memtune: Let virsh know the unlimited value for memory tunables

[Matthias Bolte]

2011/1/13 Nikunj A. Dadhania <nikunj at linux.vnet.ibm.com>:
> On Wed, 12 Jan 2011 10:21:04 -0700, Eric Blake <eblake at redhat.com> wrote:
>> On 01/12/2011 12:56 AM, Nikunj A. Dadhania wrote:
>> > Here is the patch, now the set calls are also ull.
>> >
>> > Still virCgroupGetMemoryUsage is not changed, this will require changes
>> > in virDomainInfoPtr (info->memory). I am not sure if I should have them
>> > in this patch.
>>
>> It can be a separate patch, if desired, but it is probably still needed.
>>
> virCgroupGetMemoryUsage is called only from lxcDomainGetInfo, that is
> the reason I thought that this change may not be needed.
>
>> > +++ b/tools/virsh.c
>> > @@ -2987,9 +2987,14 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd)
>> >                               params[i].value.l);
>> >                      break;
>> >                  case VIR_DOMAIN_MEMORY_PARAM_ULLONG:
>> > -                    vshPrint(ctl, "%-15s: %llu\n", params[i].field,
>> > -                             params[i].value.ul);
>> > +                {
>> > +                    if (params[i].value.ul == VIR_DOMAIN_MEMORY_PARAM_UNLIMITED)
>> > +                        vshPrint(ctl, "%-15s: unlimited\n", params[i].field);
>> > +                    else
>> > +                        vshPrint(ctl, "%-15s: %llu\n", params[i].field,
>> > +                                 params[i].value.ul);
>>
>> Do we want any back-compat considerations?  That is, if a newer virsh is
>> talking to an older server, which still answered INT64_MAX>>10 instead
>> of the new VIR_DOMAIN_MEMORY_PARAM_UNLIMITED, should we recognize that
>> situation as another reason to print "unlimited"?
>>
> As Mattias suggested in the other mail, this adds more complications. My
> take is to have VIR_DOMAIN_MEMORY_PARAM_UNLIMITED as the max value.
>
> Here is the patch which adds setting as well as displaying the
> "unlimited" values. Now in virsh to specify "unlimited" the user would
> need to pass "-1"
>
> for example:
> virsh # memtune lxcbb1 --hard-limit -1
>
> From: Nikunj A. Dadhania <nikunj at linux.vnet.ibm.com>
>
> Display and set unlimited when the memory cgroup settings. Unlimited is
> represented by INT64_MAX in memory cgroup.
>
> v4: Fix handling of setting unlimited values
> v3: Make virCgroupSet memory call ull
>
> Signed-off-by: Nikunj A. Dadhania <nikunj at linux.vnet.ibm.com>
> Reported-by: Justin Clift <jclift at redhat.com>
> ---
>  include/libvirt/libvirt.h.in |    1
>  src/lxc/lxc_driver.c         |    2 -
>  src/qemu/qemu_driver.c       |    2 -
>  src/util/cgroup.c            |   93 +++++++++++++++++++++++++++++++-----------
>  src/util/cgroup.h            |   14 +++---
>  tools/virsh.c                |   13 +++++-
>  6 files changed, 90 insertions(+), 35 deletions(-)
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 3c6a54a..3ee47b9 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -696,6 +696,7 @@ typedef enum {
>  */
>
>  #define VIR_DOMAIN_MEMORY_FIELD_LENGTH 80
> +#define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED INT64_MAX

If I understand it correctly we currently use INT64_MAX>>10 to
indicate "unlimited". Therefore, we should define
VIR_DOMAIN_MEMORY_PARAM_UNLIMITED to INT64_MAX>>10 as this would
preserve the existing behavior.

I think this patch started to fix the problem that virsh memtune
reports 9007199254740991 (== INT64_MAX>>10) when the value actually
means unlimited. We want virsh to report "unlimited" in this case.

As we cannot fix the problem in a better way (like using -1 for
unlimited) anymore, we try to workaround it:

First add a define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED for the magic
value INT64_MAX>>10, but stick to this value, because changing it
could break existing applications.

Second make virsh memtune detect VIR_DOMAIN_MEMORY_PARAM_UNLIMITED and
print "unlimited" in that case instead of the actual numeric value.

Third make virsh memtune accept -1 as unlimited and translate it to
VIR_DOMAIN_MEMORY_PARAM_UNLIMITED.

You already addressed the second and third point in your patch so
we're close to a proper workaround.

Matthias