[libvirt] [PATCH v2 2/3] qemu: hotplug: Fix mlock limit handling on memory hotplug

[John Ferlan]

On 11/09/2015 07:50 AM, Peter Krempa wrote:
> If mlock is required either due to use of VFIO hostdevs or due to the
> fact that it's enabled it needs to be tweaked prior to adding new memory
> or after removing a module. Add a helper to determine when it's
> necessary and reuse it both on hotplug and hotunplug.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1273491
> ---
>  src/qemu/qemu_domain.c  | 27 +++++++++++++++++++++++++++
>  src/qemu/qemu_domain.h  |  1 +
>  src/qemu/qemu_hotplug.c | 32 +++++++++++++++++++++++++++++---
>  3 files changed, 57 insertions(+), 3 deletions(-)
> 

FWIW: My comments in v1 when someone sets hard_limit, doesn't have VFIO,
starts with some memory device objects and expectations. I had a slight
memory lapse as to what qemuDomainGetMlockLimitBytes returns. :-)

Now that I have recovered from that page fault, when a hard_limit is
set, regardless of what memory is added the value used in the call to
virProcessSetMaxMemLock won't change...

Still we could/should document in formatdomain that memory devices
present or added would need be accounted for when considering what
hard_limit is set.

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index a06624e..6f95f76 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3643,3 +3643,30 @@ qemuDomainGetMlockLimitBytes(virDomainDefPtr def)
> 
>      return memKB << 10;
>  }
> +
> +
> +/**
> + * @def: domain definition
> + *
> + * Returns ture if the locked memory limit needs to be set or updated due to
> + * configuration or passthrough devices.
> + * */
> +bool
> +qemuDomainRequiresMlock(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;
> +    }
> +
> +    return false;
> +}
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index e34370b..4be998c 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -483,5 +483,6 @@ int qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver,
>                                        virDomainObjPtr vm);
> 
>  unsigned long long qemuDomainGetMlockLimitBytes(virDomainDefPtr def);
> +bool qemuDomainRequiresMlock(virDomainDefPtr def);
> 
>  #endif /* __QEMU_DOMAIN_H__ */
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index e7fc036..1e7a653 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1768,6 +1768,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>      virJSONValuePtr props = NULL;
>      virObjectEventPtr event;
>      bool fix_balloon = false;
> +    bool mlock = false;
>      int id;
>      int ret = -1;
> 
> @@ -1802,16 +1803,25 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>          goto cleanup;
>      }
> 
> +    mlock = qemuDomainRequiresMlock(vm->def);
> +
> +    if (mlock &&
> +        virProcessSetMaxMemLock(vm->pid,
> +                                qemuDomainGetMlockLimitBytes(vm->def)) < 0) {
> +        mlock = false;

Coverity found a need for:

    virJSONValueFree(props);


ACK - with the adjustment

John

BTW: If you think it matters - if we got the LimitBytes before inserting
vm->mem to the domain def, then we could know whether it changed and
whether we needed to Set it here and the error path. One would like to
believe an initial setting to hard_limit (such as 4G) wouldn't have a
failure if someone passed 4G again...


> +        goto removedef;
> +    }
> +
>      qemuDomainObjEnterMonitor(driver, vm);
>      if (qemuMonitorAddObject(priv->mon, backendType, objalias, props) < 0)
> -        goto removedef;
> +        goto exit_monitor;
> 
>      if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
>          virErrorPtr err = virSaveLastError();
>          ignore_value(qemuMonitorDelObject(priv->mon, objalias));
>          virSetError(err);
>          virFreeError(err);
> -        goto removedef;
> +        goto exit_monitor;
>      }
> 
>      if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> @@ -1845,17 +1855,27 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>      virDomainMemoryDefFree(mem);
>      return ret;
> 
> - removedef:
> + exit_monitor:
>      if (qemuDomainObjExitMonitor(driver, vm) < 0) {
>          mem = NULL;
>          goto audit;
>      }
> 
> + removedef:
>      if ((id = virDomainMemoryFindByDef(vm->def, mem)) >= 0)
>          mem = virDomainMemoryRemove(vm->def, id);
>      else
>          mem = NULL;
> 
> +    /* reset the mlock limit */
> +    if (mlock) {
> +        virErrorPtr err = virSaveLastError();
> +        ignore_value(virProcessSetMaxMemLock(vm->pid,
> +                                             qemuDomainGetMlockLimitBytes(vm->def)));
> +        virSetError(err);
> +        virFreeError(err);
> +    }
> +
>      goto audit;
>  }
> 
> @@ -2947,6 +2967,12 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
>          virDomainMemoryRemove(vm->def, idx);
> 
>      virDomainMemoryDefFree(mem);
> +
> +    /* decrease the mlock limit after memory unplug if necessary */
> +    if (qemuDomainRequiresMlock(vm->def))
> +        ignore_value(virProcessSetMaxMemLock(vm->pid,
> +                                             qemuDomainGetMlockLimitBytes(vm->def)));
> +
>      return 0;
>  }
>