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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list