The prlimit() function allows both getting and setting limits for a process; expose the same functionality in our wrapper. Add the const modifier for new_limit, in accordance with the prototype for prlimit(). --- src/util/virprocess.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 4b18903..9b38834 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -725,15 +725,19 @@ int virProcessSetNamespaces(size_t nfdlist, #if HAVE_PRLIMIT static int -virProcessPrLimit(pid_t pid, int resource, struct rlimit *rlim) +virProcessPrLimit(pid_t pid, + int resource, + const struct rlimit *new_limit, + struct rlimit *old_limit) { - return prlimit(pid, resource, rlim, NULL); + return prlimit(pid, resource, new_limit, old_limit); } #elif HAVE_SETRLIMIT static int virProcessPrLimit(pid_t pid ATTRIBUTE_UNUSED, int resource ATTRIBUTE_UNUSED, - struct rlimit *rlim ATTRIBUTE_UNUSED) + const struct rlimit *new_limit ATTRIBUTE_UNUSED, + struct rlimit *old_limit ATTRIBUTE_UNUSED) { errno = ENOSYS; return -1; @@ -758,7 +762,7 @@ virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes) return -1; } } else { - if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, &rlim) < 0) { + if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, &rlim, NULL) < 0) { virReportSystemError(errno, _("cannot limit locked memory " "of process %lld to %llu"), @@ -803,7 +807,7 @@ virProcessSetMaxProcesses(pid_t pid, unsigned int procs) return -1; } } else { - if (virProcessPrLimit(pid, RLIMIT_NPROC, &rlim) < 0) { + if (virProcessPrLimit(pid, RLIMIT_NPROC, &rlim, NULL) < 0) { virReportSystemError(errno, _("cannot limit number of subprocesses " "of process %lld to %u"), @@ -851,7 +855,7 @@ virProcessSetMaxFiles(pid_t pid, unsigned int files) return -1; } } else { - if (virProcessPrLimit(pid, RLIMIT_NOFILE, &rlim) < 0) { + if (virProcessPrLimit(pid, RLIMIT_NOFILE, &rlim, NULL) < 0) { virReportSystemError(errno, _("cannot limit number of open files " "of process %lld to %u"), -- 2.5.0

This function can be used to retrieve the current locked memory limit for a process, so that the setting can be later restored. Add a configure check for getrlimit(), which we now use. --- configure.ac | 2 +- src/libvirt_private.syms | 1 + src/util/virprocess.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 2 ++ 4 files changed, 46 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index f481c50..c2a567f 100644 --- a/configure.ac +++ b/configure.ac @@ -279,7 +279,7 @@ AC_CHECK_SIZEOF([long]) dnl Availability of various common functions (non-fatal if missing), dnl and various less common threadsafe functions AC_CHECK_FUNCS_ONCE([cfmakeraw fallocate geteuid getgid getgrnam_r \ - getmntent_r getpwuid_r getuid kill mmap newlocale posix_fallocate \ + getmntent_r getpwuid_r getrlimit getuid kill mmap newlocale posix_fallocate \ posix_memalign prlimit regexec sched_getaffinity setgroups setns \ setrlimit symlink sysctlbyname getifaddrs sched_setscheduler]) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7e60d87..e330a16 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2022,6 +2022,7 @@ virPortAllocatorSetUsed; virProcessAbort; virProcessExitWithStatus; virProcessGetAffinity; +virProcessGetMaxMemLock; virProcessGetNamespaces; virProcessGetPids; virProcessGetStartTime; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 9b38834..b14164a 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -788,6 +788,48 @@ virProcessSetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED, unsigned long long bytes) } #endif /* ! (HAVE_SETRLIMIT && defined(RLIMIT_MEMLOCK)) */ +#if HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK) +int +virProcessGetMaxMemLock(pid_t pid, + unsigned long long *bytes) +{ + struct rlimit rlim; + + if (!bytes) + return 0; + + if (pid == 0) { + if (getrlimit(RLIMIT_MEMLOCK, &rlim) < 0) { + virReportSystemError(errno, + "%s", + _("cannot get locked memory limit")); + return -1; + } + } else { + if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, NULL, &rlim) < 0) { + virReportSystemError(errno, + _("cannot get locked memory limit " + "of process %lld"), + (long long int) pid); + return -1; + } + } + + /* virProcessSetMaxMemLock() sets both rlim_cur and rlim_max to the + * same value, so we can retrieve just rlim_max here */ + *bytes = rlim.rlim_max; + + return 0; +} +#else /* ! (HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)) */ +int +virProcessGetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED, + unsigned long long *bytes) +{ + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); + return -1; +} +#endif /* ! (HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)) */ #if HAVE_SETRLIMIT && defined(RLIMIT_NPROC) int diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 1768009..a7a1fe9 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -76,6 +76,8 @@ int virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes); int virProcessSetMaxProcesses(pid_t pid, unsigned int procs); int virProcessSetMaxFiles(pid_t pid, unsigned int files); +int virProcessGetMaxMemLock(pid_t pid, unsigned long long *bytes); + /* Callback to run code within the mount namespace tied to the given * pid. This function must use only async-signal-safe functions, as * it gets run after a fork of a multi-threaded process. The return -- 2.5.0

On Wed, 2015-12-09 at 12:00 -0500, John Ferlan wrote: I think ignoring failures in getting the current memory locking limit is the right thing to do in the upper layers, as that just means we won't be able to restore the original limit afterwards and that's not really a huge problem. However, at this level, we're dealing with low-level functionality and I think all failures should be reported appropriately, eg. with a negative return value and a proper error logged. What other API do you mean? I'd rather squeeze the second and third lines together or leave it as it is, if that's the same to you. I don't know, but I don't think we should concern ourself too much with that case as virProcessSetMaxMemLock(), which will be used to restore whatever value this function returns, ignores the value 0. Having a function that either fails as unimplemented or succeeds depending on its arguments doesn't feel right to me. I see, however, that all virProcessSetMax*() functions behave this way already so it makes sense to change do the same for consistency's sake. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

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

On 11/24/2015 08:56 AM, Andrea Bolognani wrote: OK - so now I see how this is going to be used... patch 3's multi purpose MaxMemLock makes a bit more sense; however, I'm still wondering how we'd ever get to a point where the a last removal wouldn't have set the lockbytes value to anything but the original (yes, patch 5 is missing from the current removal)... Seems like perhaps the original_memlock is fixing the bug that was shown in patch 5 - that the hostdev removal didn't return our value properly. IOW: If the original_memlock adjustment was taken out, is there a case (after patch 5 is applied) where when memory locking is no longer required that the value after the removal wouldn't have been what was saved in original_memlock. Perhaps would have been easier to have added the Adjust code without original_memlock, then replace code as is done here, then add the hostdev case, then add the original value. Just trying to separate new functionality from code motion/creation of helper code. The current code says don't run the "reset" code if the SetMaxMemLock failed... With this change, in the removedef code you'll call the AdjustMaxMemLock regardless of whether we failed to set. That fail to set (at this point in time) could be the "GetMaxMemLock" failed or the "SetMaxMemLock" failed. The point being at removedef, how do we know if the failure was because we failed to Adjust or something else. If we failed to Adjust, then calling it again will of course more than likely fail. John

On 11/24/2015 08:56 AM, Andrea Bolognani wrote: Since the add was in qemuDomainAttachHostPCIDevice, why is the remove not in qemuDomainDetachHostPCIDevice? Doing it here would mean it's call for any host device removal - whether or not it was ever added. John

On 12/10/2015 10:03 AM, Andrea Bolognani wrote: Yeah - this all got confusing as I was paging back and forth... "DetachDevice" and "RemoveDevice"... Then I reviewed another change for shmem and got even more confused. Since qemuDomainRemoveHostDevice could be called for any host device removal and not specifically the HostPCIDevice, I was pointing out by calling qemuDomainAdjustMaxMemLock there you could be calling it even when a HostPCIDevice wasn't being removed. It was clear to me when I wrote it ;-) John