[libvirt PATCH v2 0/4] qemu: Only raise memlock limit if necessary
This feature has been requested by KubeVirt developers and will make it possible for them to make some VFIO-related features, such as migration and hotplug, work correctly. https://bugzilla.redhat.com/show_bug.cgi?id=1916346 Changes from [v1]: * prep patches have been pushed; * parsing /proc file is now explicitly restricted to Linux only and only uses safe libvirt helpers; * the qemu.conf knob has been dropped in favor of adopting a behavior that should work correctly in all scenarios. [v1] https://listman.redhat.com/archives/libvir-list/2021-March/msg00293.html Andrea Bolognani (4): util: Try to get limits from /proc qemu: Don't ignore virProcessGetMaxMemLock() errors qemu: Refactor qemuDomainAdjustMaxMemLock() qemu: Only raise memlock limit if necessary src/qemu/qemu_domain.c | 36 +++++++++------ src/util/virprocess.c | 102 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 13 deletions(-) -- 2.26.2
Calling prlimit() requires elevated privileges, specifically CAP_SYS_RESOURCE, and getrlimit() only works for the current process which is too limiting for our needs; /proc/$pid/limits, on the other hand, can be read by any process, so implement parsing that file as a fallback for when prlimit() fails. This is useful in containerized environments. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/util/virprocess.c | 102 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 8428c91182..4fa854090d 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -757,6 +757,99 @@ virProcessSetRLimit(int resource, #endif /* WITH_SETRLIMIT */ #if WITH_GETRLIMIT +static const char* +virProcessLimitResourceToLabel(int resource) +{ + switch (resource) { +# if defined(RLIMIT_MEMLOCK) + case RLIMIT_MEMLOCK: + return "Max locked memory"; +# endif /* defined(RLIMIT_MEMLOCK) */ + +# if defined(RLIMIT_NPROC) + case RLIMIT_NPROC: + return "Max processes"; +# endif /* defined(RLIMIT_NPROC) */ + +# if defined(RLIMIT_NOFILE) + case RLIMIT_NOFILE: + return "Max open files"; +# endif /* defined(RLIMIT_NOFILE) */ + +# if defined(RLIMIT_CORE) + case RLIMIT_CORE: + return "Max core file size"; +# endif /* defined(RLIMIT_CORE) */ + + default: + return NULL; + } +} + +# if defined(__linux__) +static int +virProcessGetLimitFromProc(pid_t pid, + int resource, + struct rlimit *limit) +{ + g_autofree char *procfile = NULL; + g_autofree char *buf = NULL; + g_auto(GStrv) lines = NULL; + const char *label; + size_t i; + + if (!(label = virProcessLimitResourceToLabel(resource))) { + return -1; + } + + procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid); + + if (virFileReadAllQuiet(procfile, 2048, &buf) < 0) + return -1; + + if (!(lines = g_strsplit(buf, "\n", 0))) + return -1; + + for (i = 0; lines[i]; i++) { + g_autofree char *softLimit = NULL; + g_autofree char *hardLimit = NULL; + char *line = lines[i]; + unsigned long long tmp; + + if (!(line = STRSKIP(line, label))) + continue; + + if (sscanf(line, "%ms %ms %*s", &softLimit, &hardLimit) < 2) + return -1; + + if (STREQ(softLimit, "unlimited")) { + limit->rlim_cur = RLIM_INFINITY; + } else { + if (virStrToLong_ull(softLimit, NULL, 10, &tmp) < 0) + return -1; + limit->rlim_cur = tmp; + } + if (STREQ(hardLimit, "unlimited")) { + limit->rlim_max = RLIM_INFINITY; + } else { + if (virStrToLong_ull(hardLimit, NULL, 10, &tmp) < 0) + return -1; + limit->rlim_max = tmp; + } + } + + return 0; +} +# else /* !defined(__linux__) */ +static int +virProcessGetLimitFromProc(pid_t pid G_GNUC_UNUSED, + int resource G_GNUC_UNUSED, + struct rlimit *limit G_GNUC_UNUSED) +{ + return -1; +} +# endif /* !defined(__linux__) */ + static int virProcessGetLimit(pid_t pid, int resource, @@ -768,6 +861,15 @@ virProcessGetLimit(pid_t pid, if (virProcessPrLimit(pid, resource, NULL, old_limit) == 0) return 0; + /* For whatever reason, using prlimit() on another process - even + * when it's just to obtain the current limit rather than changing + * it - requires CAP_SYS_RESOURCE, which we might not have in a + * containerized environment; on the other hand, no particular + * permission is needed to poke around /proc, so try that if going + * through the syscall didn't work */ + if (virProcessGetLimitFromProc(pid, resource, old_limit) == 0) + return 0; + if (same_process && virProcessGetRLimit(resource, old_limit) == 0) return 0; -- 2.26.2
On 3/9/21 2:47 PM, Andrea Bolognani wrote:
Calling prlimit() requires elevated privileges, specifically CAP_SYS_RESOURCE, and getrlimit() only works for the current process which is too limiting for our needs; /proc/$pid/limits, on the other hand, can be read by any process, so implement parsing that file as a fallback for when prlimit() fails.
This is useful in containerized environments.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/util/virprocess.c | 102 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 8428c91182..4fa854090d 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -757,6 +757,99 @@ virProcessSetRLimit(int resource, #endif /* WITH_SETRLIMIT */
#if WITH_GETRLIMIT +static const char* +virProcessLimitResourceToLabel(int resource) +{ + switch (resource) { +# if defined(RLIMIT_MEMLOCK) + case RLIMIT_MEMLOCK: + return "Max locked memory"; +# endif /* defined(RLIMIT_MEMLOCK) */ + +# if defined(RLIMIT_NPROC) + case RLIMIT_NPROC: + return "Max processes"; +# endif /* defined(RLIMIT_NPROC) */ + +# if defined(RLIMIT_NOFILE) + case RLIMIT_NOFILE: + return "Max open files"; +# endif /* defined(RLIMIT_NOFILE) */ + +# if defined(RLIMIT_CORE) + case RLIMIT_CORE: + return "Max core file size"; +# endif /* defined(RLIMIT_CORE) */ + + default: + return NULL; + } +} + +# if defined(__linux__) +static int +virProcessGetLimitFromProc(pid_t pid, + int resource, + struct rlimit *limit) +{ + g_autofree char *procfile = NULL; + g_autofree char *buf = NULL; + g_auto(GStrv) lines = NULL; + const char *label; + size_t i; + + if (!(label = virProcessLimitResourceToLabel(resource))) { + return -1; + }
While I am in favor of curly braces, this is incosistent with the rest of the function. However, you'll need to add another line here, probably so in the end you'll need to keep them. See [1].
+ + procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid); + + if (virFileReadAllQuiet(procfile, 2048, &buf) < 0) + return -1; + + if (!(lines = g_strsplit(buf, "\n", 0))) + return -1; + + for (i = 0; lines[i]; i++) { + g_autofree char *softLimit = NULL; + g_autofree char *hardLimit = NULL; + char *line = lines[i]; + unsigned long long tmp; + + if (!(line = STRSKIP(line, label))) + continue; + + if (sscanf(line, "%ms %ms %*s", &softLimit, &hardLimit) < 2) + return -1; + + if (STREQ(softLimit, "unlimited")) { + limit->rlim_cur = RLIM_INFINITY; + } else { + if (virStrToLong_ull(softLimit, NULL, 10, &tmp) < 0) + return -1; + limit->rlim_cur = tmp; + } + if (STREQ(hardLimit, "unlimited")) { + limit->rlim_max = RLIM_INFINITY; + } else { + if (virStrToLong_ull(hardLimit, NULL, 10, &tmp) < 0) + return -1; + limit->rlim_max = tmp; + } + } + + return 0; +} +# else /* !defined(__linux__) */ +static int +virProcessGetLimitFromProc(pid_t pid G_GNUC_UNUSED, + int resource G_GNUC_UNUSED, + struct rlimit *limit G_GNUC_UNUSED) +{ + return -1; +} +# endif /* !defined(__linux__) */ + static int virProcessGetLimit(pid_t pid, int resource, @@ -768,6 +861,15 @@ virProcessGetLimit(pid_t pid, if (virProcessPrLimit(pid, resource, NULL, old_limit) == 0) return 0;
+ /* For whatever reason, using prlimit() on another process - even + * when it's just to obtain the current limit rather than changing + * it - requires CAP_SYS_RESOURCE, which we might not have in a + * containerized environment; on the other hand, no particular + * permission is needed to poke around /proc, so try that if going + * through the syscall didn't work */ + if (virProcessGetLimitFromProc(pid, resource, old_limit) == 0) + return 0;
There is just a single caller of this virProcessGetLimit() function and it expects errno to be set on failure. But that's not always the case with your patch. Moreover, I'd expect the !Linux stub to set ENOSYS.
+ if (same_process && virProcessGetRLimit(resource, old_limit) == 0) return 0;
Michal
On Mon, 2021-03-15 at 10:21 +0100, Michal Privoznik wrote:
On 3/9/21 2:47 PM, Andrea Bolognani wrote:
+static int +virProcessGetLimitFromProc(pid_t pid, + int resource, + struct rlimit *limit) +{ + g_autofree char *procfile = NULL; + g_autofree char *buf = NULL; + g_auto(GStrv) lines = NULL; + const char *label; + size_t i; + + if (!(label = virProcessLimitResourceToLabel(resource))) { + return -1; + }
While I am in favor of curly braces, this is incosistent with the rest of the function. However, you'll need to add another line here, probably so in the end you'll need to keep them. See [1].
@@ -768,6 +861,15 @@ virProcessGetLimit(pid_t pid, if (virProcessPrLimit(pid, resource, NULL, old_limit) == 0) return 0;
+ /* For whatever reason, using prlimit() on another process - even + * when it's just to obtain the current limit rather than changing + * it - requires CAP_SYS_RESOURCE, which we might not have in a + * containerized environment; on the other hand, no particular + * permission is needed to poke around /proc, so try that if going + * through the syscall didn't work */ + if (virProcessGetLimitFromProc(pid, resource, old_limit) == 0) + return 0;
There is just a single caller of this virProcessGetLimit() function and it expects errno to be set on failure. But that's not always the case with your patch. Moreover, I'd expect the !Linux stub to set ENOSYS.
Good catch! The changes you're suggesting are not trivial enough for me to feel comfortable simply applying them locally and then pushing right away. Would the diff below look reasonable squashed in? diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 4fa854090d..cae0dabae3 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -799,16 +799,17 @@ virProcessGetLimitFromProc(pid_t pid, size_t i; if (!(label = virProcessLimitResourceToLabel(resource))) { + virReportSystemError(EINVAL, "%s", _("Invalid resource")); return -1; } procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid); if (virFileReadAllQuiet(procfile, 2048, &buf) < 0) - return -1; + goto error; if (!(lines = g_strsplit(buf, "\n", 0))) - return -1; + goto error; for (i = 0; lines[i]; i++) { g_autofree char *softLimit = NULL; @@ -820,25 +821,29 @@ virProcessGetLimitFromProc(pid_t pid, continue; if (sscanf(line, "%ms %ms %*s", &softLimit, &hardLimit) < 2) - return -1; + goto error; if (STREQ(softLimit, "unlimited")) { limit->rlim_cur = RLIM_INFINITY; } else { if (virStrToLong_ull(softLimit, NULL, 10, &tmp) < 0) - return -1; + goto error; limit->rlim_cur = tmp; } if (STREQ(hardLimit, "unlimited")) { limit->rlim_max = RLIM_INFINITY; } else { if (virStrToLong_ull(hardLimit, NULL, 10, &tmp) < 0) - return -1; + goto error; limit->rlim_max = tmp; } } return 0; + + error: + virReportSystemError(EIO, "%s", _("Input/output error")); + return -1; } # else /* !defined(__linux__) */ static int @@ -846,6 +851,7 @@ virProcessGetLimitFromProc(pid_t pid G_GNUC_UNUSED, int resource G_GNUC_UNUSED, struct rlimit *limit G_GNUC_UNUSED) { + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); return -1; } # endif /* !defined(__linux__) */ -- Andrea Bolognani / Red Hat / Virtualization
On Mon, 2021-03-15 at 11:57 +0100, Andrea Bolognani wrote:
The changes you're suggesting are not trivial enough for me to feel comfortable simply applying them locally and then pushing right away. Would the diff below look reasonable squashed in?
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 4fa854090d..cae0dabae3 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -799,16 +799,17 @@ virProcessGetLimitFromProc(pid_t pid, size_t i;
if (!(label = virProcessLimitResourceToLabel(resource))) { + virReportSystemError(EINVAL, "%s", _("Invalid resource")); return -1; }
procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid);
if (virFileReadAllQuiet(procfile, 2048, &buf) < 0) - return -1; + goto error;
[...]
+ error: + virReportSystemError(EIO, "%s", _("Input/output error")); + return -1; } # else /* !defined(__linux__) */ static int @@ -846,6 +851,7 @@ virProcessGetLimitFromProc(pid_t pid G_GNUC_UNUSED, int resource G_GNUC_UNUSED, struct rlimit *limit G_GNUC_UNUSED) { + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); return -1; } # endif /* !defined(__linux__) */
This probably makes more sense: diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 4fa854090d..b173856b7a 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -799,16 +799,17 @@ virProcessGetLimitFromProc(pid_t pid, size_t i; if (!(label = virProcessLimitResourceToLabel(resource))) { + errno = EINVAL; return -1; } procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid); if (virFileReadAllQuiet(procfile, 2048, &buf) < 0) - return -1; + goto error; if (!(lines = g_strsplit(buf, "\n", 0))) - return -1; + goto error; for (i = 0; lines[i]; i++) { g_autofree char *softLimit = NULL; @@ -820,25 +821,29 @@ virProcessGetLimitFromProc(pid_t pid, continue; if (sscanf(line, "%ms %ms %*s", &softLimit, &hardLimit) < 2) - return -1; + goto error; if (STREQ(softLimit, "unlimited")) { limit->rlim_cur = RLIM_INFINITY; } else { if (virStrToLong_ull(softLimit, NULL, 10, &tmp) < 0) - return -1; + goto error; limit->rlim_cur = tmp; } if (STREQ(hardLimit, "unlimited")) { limit->rlim_max = RLIM_INFINITY; } else { if (virStrToLong_ull(hardLimit, NULL, 10, &tmp) < 0) - return -1; + goto error; limit->rlim_max = tmp; } } return 0; + + error: + errno = EIO; + return -1; } # else /* !defined(__linux__) */ static int @@ -846,6 +851,7 @@ virProcessGetLimitFromProc(pid_t pid G_GNUC_UNUSED, int resource G_GNUC_UNUSED, struct rlimit *limit G_GNUC_UNUSED) { + errno = ENOSYS; return -1; } # endif /* !defined(__linux__) */ -- Andrea Bolognani / Red Hat / Virtualization
On 3/15/21 12:22 PM, Andrea Bolognani wrote:
On Mon, 2021-03-15 at 11:57 +0100, Andrea Bolognani wrote:
The changes you're suggesting are not trivial enough for me to feel comfortable simply applying them locally and then pushing right away. Would the diff below look reasonable squashed in?
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 4fa854090d..cae0dabae3 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -799,16 +799,17 @@ virProcessGetLimitFromProc(pid_t pid, size_t i;
if (!(label = virProcessLimitResourceToLabel(resource))) { + virReportSystemError(EINVAL, "%s", _("Invalid resource")); return -1; }
procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid);
if (virFileReadAllQuiet(procfile, 2048, &buf) < 0) - return -1; + goto error;
[...]
+ error: + virReportSystemError(EIO, "%s", _("Input/output error")); + return -1; } # else /* !defined(__linux__) */ static int @@ -846,6 +851,7 @@ virProcessGetLimitFromProc(pid_t pid G_GNUC_UNUSED, int resource G_GNUC_UNUSED, struct rlimit *limit G_GNUC_UNUSED) { + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); return -1; } # endif /* !defined(__linux__) */
This probably makes more sense:
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 4fa854090d..b173856b7a 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -799,16 +799,17 @@ virProcessGetLimitFromProc(pid_t pid, size_t i;
if (!(label = virProcessLimitResourceToLabel(resource))) { + errno = EINVAL; return -1; }
procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid);
if (virFileReadAllQuiet(procfile, 2048, &buf) < 0) - return -1; + goto error;
virFileReadAllQuiet() sets errno, this would just overwrite it with something less specific.
if (!(lines = g_strsplit(buf, "\n", 0))) - return -1; + goto error;
I think this check can be dropped. g_strsplit() doesn't ever return NULL really, does it?
for (i = 0; lines[i]; i++) { g_autofree char *softLimit = NULL; @@ -820,25 +821,29 @@ virProcessGetLimitFromProc(pid_t pid, continue;
if (sscanf(line, "%ms %ms %*s", &softLimit, &hardLimit) < 2) - return -1; + goto error;
if (STREQ(softLimit, "unlimited")) { limit->rlim_cur = RLIM_INFINITY; } else { if (virStrToLong_ull(softLimit, NULL, 10, &tmp) < 0) - return -1; + goto error; limit->rlim_cur = tmp; } if (STREQ(hardLimit, "unlimited")) { limit->rlim_max = RLIM_INFINITY; } else { if (virStrToLong_ull(hardLimit, NULL, 10, &tmp) < 0) - return -1; + goto error; limit->rlim_max = tmp; } }
return 0; + + error: + errno = EIO; + return -1; } # else /* !defined(__linux__) */ static int @@ -846,6 +851,7 @@ virProcessGetLimitFromProc(pid_t pid G_GNUC_UNUSED, int resource G_GNUC_UNUSED, struct rlimit *limit G_GNUC_UNUSED) { + errno = ENOSYS; return -1; } # endif /* !defined(__linux__) */
The rest looks good. Michal
On Mon, 2021-03-15 at 14:28 +0100, Michal Privoznik wrote:
On 3/15/21 12:22 PM, Andrea Bolognani wrote:
if (virFileReadAllQuiet(procfile, 2048, &buf) < 0) - return -1; + goto error;
virFileReadAllQuiet() sets errno, this would just overwrite it with something less specific.
Good point. I've changed this to if (virFileReadAllQuiet(procfile, 2048, &buf) < 0) { /* This function already sets errno, so don't overwrite that * and return immediately instead */ return -1; } so that some explanation for the choice is retained.
if (!(lines = g_strsplit(buf, "\n", 0))) - return -1; + goto error;
I think this check can be dropped. g_strsplit() doesn't ever return NULL really, does it?
You're right, it doesn't. I've changed it.
The rest looks good.
Thanks for the review and for the very good suggestions! I'll see whether I can get some feedback from KubeVirt developers regarding whether or not this actually improves things for them before pushing. -- Andrea Bolognani / Red Hat / Virtualization
Now that we've implemented a fallback for the function that obtains the information from /proc, there is no reason we would get a failure unless there's something seriously wrong with the environment we're running in, in which case we're better off reporting the issue to the user rather than pretending everything is fine. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 73e2473dce..1e9178764b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9246,11 +9246,10 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm, if (bytes) { /* 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. Failing to obtain the current limit is not a critical - * failure, it just means we'll be unable to lower it later */ + * required */ if (!vm->originalMemlock) { if (virProcessGetMaxMemLock(vm->pid, &(vm->originalMemlock)) < 0) - vm->originalMemlock = 0; + return -1; } } else { /* Once memory locking is no longer required, we can restore the -- 2.26.2
Store the current memory locking limit and the desired one separately, which will help with later changes. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1e9178764b..f8b0e1a62a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9239,27 +9239,29 @@ int qemuDomainAdjustMaxMemLock(virDomainObjPtr vm, bool forceVFIO) { - unsigned long long bytes = 0; + unsigned long long currentMemLock = 0; + unsigned long long desiredMemLock = 0; - bytes = qemuDomainGetMemLockLimitBytes(vm->def, forceVFIO); + desiredMemLock = qemuDomainGetMemLockLimitBytes(vm->def, forceVFIO); + if (virProcessGetMaxMemLock(vm->pid, ¤tMemLock) < 0) + return -1; - if (bytes) { + if (desiredMemLock > 0) { /* 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->originalMemlock) { - if (virProcessGetMaxMemLock(vm->pid, &(vm->originalMemlock)) < 0) - return -1; + if (vm->originalMemlock == 0) { + vm->originalMemlock = currentMemLock; } } else { /* Once memory locking is no longer required, we can restore the * original, usually very low, limit */ - bytes = vm->originalMemlock; + desiredMemLock = vm->originalMemlock; vm->originalMemlock = 0; } - if (bytes > 0 && - virProcessSetMaxMemLock(vm->pid, bytes) < 0) { + if (desiredMemLock > 0 && + virProcessSetMaxMemLock(vm->pid, desiredMemLock) < 0) { return -1; } -- 2.26.2
Attempting to set the memlock limit might fail if we're running in a containerized environment where CAP_SYS_RESOURCE is not available, and if the limit is already high enough there's no point in trying to raise anyway. https://bugzilla.redhat.com/show_bug.cgi?id=1916346 Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f8b0e1a62a..560c73b560 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9247,11 +9247,20 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm, return -1; if (desiredMemLock > 0) { - /* 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->originalMemlock == 0) { - vm->originalMemlock = currentMemLock; + if (currentMemLock < desiredMemLock) { + /* 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->originalMemlock == 0) { + vm->originalMemlock = currentMemLock; + } + } else { + /* If the limit is already high enough, we can assume + * that some external process is taking care of managing + * process limits and we shouldn't do anything ourselves: + * we're probably running in a containerized environment + * where we don't have enough privilege anyway */ + desiredMemLock = 0; } } else { /* Once memory locking is no longer required, we can restore the -- 2.26.2
On 3/9/21 2:47 PM, Andrea Bolognani wrote:
This feature has been requested by KubeVirt developers and will make it possible for them to make some VFIO-related features, such as migration and hotplug, work correctly.
https://bugzilla.redhat.com/show_bug.cgi?id=1916346
Changes from [v1]:
* prep patches have been pushed;
* parsing /proc file is now explicitly restricted to Linux only and only uses safe libvirt helpers;
* the qemu.conf knob has been dropped in favor of adopting a behavior that should work correctly in all scenarios.
[v1] https://listman.redhat.com/archives/libvir-list/2021-March/msg00293.html
Andrea Bolognani (4): util: Try to get limits from /proc qemu: Don't ignore virProcessGetMaxMemLock() errors qemu: Refactor qemuDomainAdjustMaxMemLock() qemu: Only raise memlock limit if necessary
src/qemu/qemu_domain.c | 36 +++++++++------ src/util/virprocess.c | 102 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 13 deletions(-)
If you fix small nits in 1/4 then: Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Andrea Bolognani -
Michal Privoznik