[libvirt] [PATCH 1/3] qemu: Move memory limit computation to a reusable function

[Laine Stump]

On 06/28/2013 11:04 AM, Jiri Denemark wrote:
> ---
>  src/qemu/qemu_cgroup.c | 21 ++-------------------
>  src/qemu/qemu_domain.c | 29 +++++++++++++++++++++++++++++
>  src/qemu/qemu_domain.h |  2 ++
>  3 files changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 5f54ca6..22bf78e 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -461,9 +461,7 @@ static int
>  qemuSetupMemoryCgroup(virDomainObjPtr vm)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> -    unsigned long long hard_limit;
>      int rc;
> -    int i;
>  
>      if (!virCgroupHasController(priv->cgroup,VIR_CGROUP_CONTROLLER_MEMORY)) {
>          if (vm->def->mem.hard_limit != 0 ||
> @@ -477,23 +475,8 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm)
>          }
>      }
>  
> -    hard_limit = vm->def->mem.hard_limit;
> -    if (!hard_limit) {
> -        /* If there is no hard_limit set, set a reasonable one to avoid
> -         * system thrashing caused by exploited qemu.  A 'reasonable
> -         * limit' has been chosen:
> -         *     (1 + k) * (domain memory + total video memory) + (32MB for
> -         *     cache per each disk) + F
> -         * where k = 0.5 and F = 200MB.  The cache for disks is important as
> -         * kernel cache on the host side counts into the RSS limit. */
> -        hard_limit = vm->def->mem.max_balloon;
> -        for (i = 0; i < vm->def->nvideos; i++)
> -            hard_limit += vm->def->videos[i]->vram;
> -        hard_limit = hard_limit * 1.5 + 204800;
> -        hard_limit += vm->def->ndisks * 32768;
> -    }
> -
> -    rc = virCgroupSetMemoryHardLimit(priv->cgroup, hard_limit);
> +    rc = virCgroupSetMemoryHardLimit(priv->cgroup,
> +                                     qemuDomainMemoryLimit(vm->def));
>      if (rc != 0) {
>          virReportSystemError(-rc,
>                               _("Unable to set memory hard limit for domain %s"),
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 8d79066..77b94ec 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2181,3 +2181,32 @@ cleanup:
>      virObjectUnref(cfg);
>      return ret;
>  }
> +
> +
> +unsigned long long
> +qemuDomainMemoryLimit(virDomainDefPtr def)
> +{
> +    unsigned long long mem;
> +    int i;
> +
> +    if (def->mem.hard_limit) {
> +        mem = def->mem.hard_limit;
> +    } else {
> +        /* If there is no hard_limit set, compute a reasonable one to avoid
> +         * system thrashing caused by exploited qemu.  A 'reasonable
> +         * limit' has been chosen:
> +         *     (1 + k) * (domain memory + total video memory) + (32MB for
> +         *     cache per each disk) + F
> +         * where k = 0.5 and F = 200MB.  The cache for disks is important as
> +         * kernel cache on the host side counts into the RSS limit.
> +         */
> +        mem = def->mem.max_balloon;
> +        for (i = 0; i < def->nvideos; i++)
> +            mem += def->videos[i]->vram;
> +        mem *= 1.5;
> +        mem += def->ndisks * 32768;
> +        mem += 204800;
> +    }


I know you're just doing a cut-paste here, but I'm curious how we
arrived at this formula. On systems using vfio, it ends up locking *way*
more memory than previously. For my test guest with 4GB of ram assigned,
the old computation would lock 5GB of ram for the qemu process. With the
new method in this patch it ends up locking just about 7GB. I don't know
if that's a case of the original simple VFIO amount being inadequate, or
the new method goind overboard; just thought there should be at least
minimal discussion before it went in (note that I tried it on a guest
with VFIO passthrough and it does work without problems.

Aside from that, ACK to the idea.

> +
> +    return mem;
> +}
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 068a4c3..ade2095 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -351,4 +351,6 @@ extern virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks;
>  extern virDomainXMLNamespace virQEMUDriverDomainXMLNamespace;
>  extern virDomainDefParserConfig virQEMUDriverDomainDefParserConfig;
>  
> +unsigned long long qemuDomainMemoryLimit(virDomainDefPtr def);
> +
>  #endif /* __QEMU_DOMAIN_H__ */