[libvirt PATCH 16/17] qemu: Wire up external limit manager

[Michal Privoznik]

On 3/5/21 8:14 PM, Andrea Bolognani wrote:
> When the config knob is enabled, we simply skip the part where
> limits are set; for the memory locking limit, which can change
> dynamically over the lifetime of the guest, we still make sure
> that the external process has set it correctly and error out
> if that turns out not to be the case
> 
> This commit is better viewed with 'git show -w'.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1916346
> 
> Signed-off-by: Andrea Bolognani <abologna at redhat.com>
> ---
>   src/conf/domain_conf.h  |  1 +
>   src/qemu/qemu_domain.c  | 39 ++++++++++++++++++++++--------------
>   src/qemu/qemu_process.c | 44 +++++++++++++++++++++++------------------
>   3 files changed, 50 insertions(+), 34 deletions(-)
> 
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 7d208d881c..d1333020e1 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2807,6 +2807,7 @@ struct _virDomainObj {
>       size_t ndeprecations;
>       char **deprecations;
>   
> +    bool externalLimitManager; /* Whether process limits are handled outside of libvirt */
>       unsigned long long originalMemlock; /* Original RLIMIT_MEMLOCK, zero if no
>                                            * restore will be required later */
>   };
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f8b0e1a62a..0d9adb2f9c 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -9246,23 +9246,32 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm,
>       if (virProcessGetMaxMemLock(vm->pid, &currentMemLock) < 0)
>           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 (!vm->externalLimitManager) {
> +        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;
> +            }
> +        } else {
> +            /* Once memory locking is no longer required, we can restore the
> +             * original, usually very low, limit */
> +            desiredMemLock = vm->originalMemlock;
> +            vm->originalMemlock = 0;
>           }
> -    } else {
> -        /* Once memory locking is no longer required, we can restore the
> -         * original, usually very low, limit */
> -        desiredMemLock = vm->originalMemlock;
> -        vm->originalMemlock = 0;
> -    }
>   
> -    if (desiredMemLock > 0 &&
> -        virProcessSetMaxMemLock(vm->pid, desiredMemLock) < 0) {
> -        return -1;
> +        if (desiredMemLock > 0 &&
> +            virProcessSetMaxMemLock(vm->pid, desiredMemLock) < 0) {
> +            return -1;
> +        }
> +    } else {
> +        if (currentMemLock < desiredMemLock) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("insufficient memlock limit (%llu < %llu)"),
> +                           currentMemLock, desiredMemLock);

Should we go this way? Our limit computation is nothing but a guess 
(even though it might be more educated than others). I think we should 
reduce this to a warning. User had chosen not to set any limits and 
might have tracked down (e.g. via strace) the actual value needed which 
may be smaller than our guess.
Another reason is that if domain is started with a VFIO/mdev device then 
no memlock is set at all but here, during hotplug it would be.

> +            return -1;
> +        }
>       }
>   
>       return 0;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c05cbe3570..2eac3934c7 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -7016,25 +7016,31 @@ qemuProcessLaunch(virConnectPtr conn,
>       virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData);
>       virCommandSetUmask(cmd, 0x002);
>   
> -    VIR_DEBUG("Setting up process limits");
> -
> -    /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
> -     * significant amount of memory, so we need to set the limit accordingly */
> -    maxMemLock = qemuDomainGetMemLockLimitBytes(vm->def, false);
> -
> -    /* For all these settings, zero indicates that the limit should
> -     * not be set explicitly and the default/inherited limit should
> -     * be applied instead */
> -    if (maxMemLock > 0)
> -        virCommandSetMaxMemLock(cmd, maxMemLock);
> -    if (cfg->maxProcesses > 0)
> -        virCommandSetMaxProcesses(cmd, cfg->maxProcesses);
> -    if (cfg->maxFiles > 0)
> -        virCommandSetMaxFiles(cmd, cfg->maxFiles);
> -
> -    /* In this case, however, zero means that core dumps should be
> -     * disabled, and so we always need to set the limit explicitly */
> -    virCommandSetMaxCoreSize(cmd, cfg->maxCore);
> +    if (cfg->externalLimitManager) {
> +        VIR_DEBUG("Not setting up process limits (handled externally)");
> +
> +        vm->externalLimitManager = true;
> +    } else {
> +        VIR_DEBUG("Setting up process limits");
> +
> +        /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
> +         * significant amount of memory, so we need to set the limit accordingly */
> +        maxMemLock = qemuDomainGetMemLockLimitBytes(vm->def, false);
> +
> +        /* For all these settings, zero indicates that the limit should
> +         * not be set explicitly and the default/inherited limit should
> +         * be applied instead */
> +        if (maxMemLock > 0)
> +            virCommandSetMaxMemLock(cmd, maxMemLock);
> +        if (cfg->maxProcesses > 0)
> +            virCommandSetMaxProcesses(cmd, cfg->maxProcesses);
> +        if (cfg->maxFiles > 0)
> +            virCommandSetMaxFiles(cmd, cfg->maxFiles);
> +
> +        /* In this case, however, zero means that core dumps should be
> +         * disabled, and so we always need to set the limit explicitly */
> +        virCommandSetMaxCoreSize(cmd, cfg->maxCore);
> +    }
>   
>       VIR_DEBUG("Setting up security labelling");
>       if (qemuSecuritySetChildProcessLabel(driver->securityManager,
> 

Michal