[libvirt] [PATCH 3/6] qemu: Add qemuDomainAdjustMaxMemLock()

[John Ferlan]

On 11/24/2015 08:56 AM, Andrea Bolognani wrote:
> This function detects whether a domain needs RLIMIT_MEMLOCK
> to be set, and if so, uses an appropriate value.
> 
> It also stores the original value inside the virDomainObj for
> the domain so that it can be later restored.
> ---
>  src/conf/domain_conf.h |  3 +++
>  src/qemu/qemu_domain.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_domain.h |  1 +
>  3 files changed, 54 insertions(+)
> 

NB: This patch wouldn't "git am -3" apply for me - looks like you'll be
having some merge conflicts to deal with.

> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8d43ee6..9e28ac9 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2386,6 +2386,9 @@ struct _virDomainObj {
>      void (*privateDataFreeFunc)(void *);
>  
>      int taint;
> +
> +    unsigned long long original_memlock; /* Original RLIMIT_MEMLOCK, zero if no
> +                                          * restore will be required later */
>  };
>  
>  typedef struct _virDomainObjList virDomainObjList;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 18513f9..51a1770 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -40,6 +40,7 @@
>  #include "virstoragefile.h"
>  #include "virstring.h"
>  #include "virthreadjob.h"
> +#include "virprocess.h"
>  
>  #include "storage/storage_driver.h"
>  
> @@ -3954,3 +3955,52 @@ qemuDomainRequiresMlock(virDomainDefPtr def)
>  
>      return false;
>  }
> +
> +/**
> + * qemuDomainAdjustMaxMemLock:
> + *
> + * @vm: domain
> + *
> + * Adjust the memory locking limit for the QEMU process associated to @vm, in
> + * order to comply with VFIO or architecture requirements.
> + *
> + * The limit will not be changed unless doing so is needed; the first time
> + * the limit is changed, the original (default) limit is stored in @vm and
> + * that value will be restored if qemuDomainAdjustMaxMemLock() is called once
> + * memory locking is no longer required.
> + *
> + * Returns: 0 on success, <0 on failure
> + */
> +int
> +qemuDomainAdjustMaxMemLock(virDomainObjPtr vm)

It seems this function does two things...

#1 Get some original value if the domain requires mlock
#2 Restores the original value

Not sure it's best to mix the logic...

> +{
> +    unsigned long long bytes = 0;
> +    int ret = -1;
> +
> +    if (qemuDomainRequiresMlock(vm->def)) {
> +        /* If this is the first time adjusting the limit, save the current
> +         * value so that we can restore it once memory locking is no longer
> +         * required */
> +        if (!vm->original_memlock) {
> +            if (virProcessGetMaxMemLock(vm->pid, &(vm->original_memlock)) < 0)
> +                goto out;

Failure scenario 1 - I couldn't get some original value

Should this really be a failure? Could fail for 4 reasons:

1. getrlimit returns fail when pid == 0
2. getrlimit && RLIMIT_MEMLOCK not available
3. prlimit returns fail
4. prlimit not available or in the case shown by commit id '05f79a38'
the oddity from a mingw build.

Given how the code is written today, if arg2 was NULL then a "0" would
have been returned.

So if it returns 0, then that means the "ability" to reset to some
initial value is not available... Thus we just return skinny, dumb, and
happy rather than failing.  EG no worse than today.

For archs/envs that support getting the value - sure we can support
resetting the value...


> +        }
> +        bytes = qemuDomainGetMlockLimitBytes(vm->def);
> +    } else {
> +        /* Once memory locking is no longer required, we can restore the
> +         * original, usually very low, limit */
> +        bytes = vm->original_memlock;

Not sure I understand what is meant by "is no longer required" (yet - I
haven't read forward)... However, if it never was required, then all the
following code does nothing since original_memlock would be 0 (and
nothing here or in future patches) sets it unless the domain requires it.

IOW: How/why does a domain go from at one point requiring Mlock to not
requiring it?  It's not lock a running PPC64 domain is going to change
it's arch type...

> +        vm->original_memlock = 0;
> +    }
> +
> +    /* Don't do anything unless we're actually setting a limit */
> +    if (bytes) {
> +        if (virProcessSetMaxMemLock(vm->pid, bytes) < 0)

Failure scenario 2 - I couldn't set a new limit.  E.G - in my mind the
true failure. I couldn't set a new value.

John

> +            goto out;
> +    }
> +
> +    ret = 0;
> +
> + out:
> +     return ret;
> +}
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 271dce9..cc64df3 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -486,6 +486,7 @@ int qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver,
>  
>  unsigned long long qemuDomainGetMlockLimitBytes(virDomainDefPtr def);
>  bool qemuDomainRequiresMlock(virDomainDefPtr def);
> +int qemuDomainAdjustMaxMemLock(virDomainObjPtr vm);
>  
>  int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
>                                         virQEMUCapsPtr qemuCaps,
>