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

[Andrea Bolognani]

On Wed, 2015-12-09 at 12:19 -0500, John Ferlan wrote:
> 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.

Yeah, a rebase was definitely in order :)

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

I believe you understand the rationale now.

> > +{
> > +    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...

Makes sense. Moreover, I can't really imagine a case where getting the
current memory limit would fail but setting a new one immediately
afterwards would succeed instead.

> > +        }
> > +        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...

I believe this, too, is now clear to you.

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

Of course, this one should keep on being be a severe failure and abort
the whole operation.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team