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

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.

> +/** > + * 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.