[libvirt] [PATCH 2/8] qemu: Remove qemuDomainRequiresMemLock()

[Martin Kletzander]

On Thu, Mar 23, 2017 at 07:16:41PM +0100, Andrea Bolognani wrote:
>Instead of having a separate function, we can simply return
>zero from the existing qemuDomainGetMemLockLimitBytes() to
>signal the caller that the memory locking limit doesn't need
>to be set for the guest.
>
>Having a single function instead of two makes it less likely
>that we will use the wrong value, which is exactly what
>happened when we started applying the limit that was meant
>for VFIO-using guests to <memoryBacking><locked>-using
>guests.
>---
> src/qemu/qemu_command.c |  4 +---
> src/qemu/qemu_domain.c  | 62 ++++++++++++++++++-------------------------------
> src/qemu/qemu_domain.h  |  1 -
> 3 files changed, 24 insertions(+), 43 deletions(-)
>
>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>index 2045c2e..52f6e00 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -9740,7 +9740,6 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>     unsigned int bootHostdevNet = 0;
>
>-
>     VIR_DEBUG("driver=%p def=%p mon=%p json=%d "
>               "qemuCaps=%p migrateURI=%s snapshot=%p vmop=%d",
>               driver, def, monitor_chr, monitor_json,
>@@ -9966,8 +9965,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>
>     /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
>      * significant amount of memory, so we need to set the limit accordingly */
>-    if (qemuDomainRequiresMemLock(def))
>-        virCommandSetMaxMemLock(cmd, qemuDomainGetMemLockLimitBytes(def));
>+    virCommandSetMaxMemLock(cmd, qemuDomainGetMemLockLimitBytes(def));
>
>     if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MSG_TIMESTAMP) &&
>         cfg->logTimestamp)
>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index 8fa43f2..2e2ba4f 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -6199,18 +6199,20 @@ qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver,
>
> /**
>  * qemuDomainGetMemLockLimitBytes:
>- *
>  * @def: domain definition
>  *
>- * Returns the size of the memory in bytes that needs to be set as
>- * RLIMIT_MEMLOCK for the QEMU process.
>- * If a mem.hard_limit is set, then that value is preferred; otherwise, the
>- * value returned may depend upon the architecture or devices present.
>+ * Calculate the memory locking limit that needs to be set in order for
>+ * the guest to operate properly. The limit depends on a number of factors,
>+ * including certain configuration options and less immediately apparent ones
>+ * such as the guest architecture or the use of certain devices.
>+ *
>+ * Returns: the memory locking limit, or 0 if setting the limit is not needed
>  */
> unsigned long long
> qemuDomainGetMemLockLimitBytes(virDomainDefPtr def)
> {
>-    unsigned long long memKB;
>+    unsigned long long memKB = 0;
>+    size_t i;
>
>     /* prefer the hard limit */
>     if (virMemoryLimitIsSet(def->mem.hard_limit)) {
>@@ -6218,13 +6220,17 @@ qemuDomainGetMemLockLimitBytes(virDomainDefPtr def)
>         goto done;
>     }
>
>-    if (ARCH_IS_PPC64(def->os.arch)) {
>+    if (def->mem.locked) {
>+        memKB = virDomainDefGetMemoryTotal(def) + 1024 * 1024;
>+        goto done;
>+    }
>+
>+    if (ARCH_IS_PPC64(def->os.arch) && def->virtType == VIR_DOMAIN_VIRT_KVM) {
>         unsigned long long maxMemory;
>         unsigned long long memory;
>         unsigned long long baseLimit;
>         unsigned long long passthroughLimit;
>         size_t nPCIHostBridges;
>-        size_t i;
>         bool usesVFIO = false;
>
>         /* TODO: Detect at runtime once we start using more than just
>@@ -6314,44 +6320,21 @@ qemuDomainGetMemLockLimitBytes(virDomainDefPtr def)
>      *
>      * Note that this may not be valid for all platforms.
>      */
>-    memKB = virDomainDefGetMemoryTotal(def) + 1024 * 1024;
>-
>- done:
>-    return memKB << 10;
>-}
>-
>-
>-/**
>- * @def: domain definition
>- *
>- * Returns true if the locked memory limit needs to be set or updated because
>- * of domain configuration, VFIO passthrough devices or architecture-specific
>- * requirements.
>- * */
>-bool
>-qemuDomainRequiresMemLock(virDomainDefPtr def)
>-{
>-    size_t i;
>-
>-    if (def->mem.locked)
>-        return true;
>-
>     for (i = 0; i < def->nhostdevs; i++) {
>         virDomainHostdevDefPtr dev = def->hostdevs[i];
>
>         if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
>             dev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
>-            dev->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
>-            return true;
>+            dev->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
>+            memKB = virDomainDefGetMemoryTotal(def) + 1024 * 1024;

Shouldn't this be raising memKB for _each_ host device?

>+        }
>     }
>
>-    /* ppc64 KVM domains need to lock some memory even when VFIO is not used */
>-    if (ARCH_IS_PPC64(def->os.arch) && def->virtType == VIR_DOMAIN_VIRT_KVM)
>-        return true;
>-
>-    return false;
>+ done:
>+    return memKB << 10;
> }
>
>+
> /**
>  * qemuDomainAdjustMaxMemLock:
>  * @vm: domain
>@@ -6372,7 +6355,9 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm)
>     unsigned long long bytes = 0;
>     int ret = -1;
>
>-    if (qemuDomainRequiresMemLock(vm->def)) {
>+    bytes = qemuDomainGetMemLockLimitBytes(vm->def);
>+
>+    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
>@@ -6381,7 +6366,6 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm)
>             if (virProcessGetMaxMemLock(vm->pid, &(vm->original_memlock)) < 0)
>                 vm->original_memlock = 0;
>         }
>-        bytes = qemuDomainGetMemLockLimitBytes(vm->def);
>     } else {
>         /* Once memory locking is no longer required, we can restore the
>          * original, usually very low, limit */

This function has weird behaviour, even when it's documented.  But it
makes sense, it just takes a while.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170327/16f56a25/attachment-0001.sig>