[libvirt] [PATCH] qemu: Set reasonable RSS limit on domain startup

[Doug Goldstein]

On Tue, Jul 17, 2012 at 11:45 AM, Michal Privoznik <mprivozn at redhat.com> wrote:
> If there's a memory leak in qemu or qemu is exploited the host's
> system will sooner or later start trashing instead of killing
> the bad process. This however has impact on performance and other
> guests as well. Therefore we should set a reasonable RSS limit
> even when user hasn't set any. It's better to be secure by default.
> ---
>  src/qemu/qemu_cgroup.c |   73 +++++++++++++++++++++++++++---------------------
>  1 files changed, 41 insertions(+), 32 deletions(-)
>
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index e39f5e1..2c158c1 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -339,42 +339,51 @@ int qemuSetupCgroup(struct qemud_driver *driver,
>          }
>      }
>
> -    if (vm->def->mem.hard_limit != 0 ||
> -        vm->def->mem.soft_limit != 0 ||
> -        vm->def->mem.swap_hard_limit != 0) {
> -        if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) {
> -            if (vm->def->mem.hard_limit != 0) {
> -                rc = virCgroupSetMemoryHardLimit(cgroup, vm->def->mem.hard_limit);
> -                if (rc != 0) {
> -                    virReportSystemError(-rc,
> -                                         _("Unable to set memory hard limit for domain %s"),
> -                                         vm->def->name);
> -                    goto cleanup;
> -                }
> -            }
> -            if (vm->def->mem.soft_limit != 0) {
> -                rc = virCgroupSetMemorySoftLimit(cgroup, vm->def->mem.soft_limit);
> -                if (rc != 0) {
> -                    virReportSystemError(-rc,
> -                                         _("Unable to set memory soft limit for domain %s"),
> -                                         vm->def->name);
> -                    goto cleanup;
> -                }
> +    if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) {
> +        unsigned long long hard_limit = vm->def->mem.hard_limit;
> +
> +        if (!hard_limit) {
> +            /* If there is no hard_limit set, set a reasonable
> +             * one to avoid system trashing caused by exploited qemu.
> +             * As 'reasonable limit' has been chosen:
> +             *     (1 + k) * (domain memory) + F
> +             * where k = 0.02 and F = 200MB. */
> +            hard_limit = vm->def->mem.max_balloon * 1.02 + 204800;
> +        }
> +
> +        rc = virCgroupSetMemoryHardLimit(cgroup, hard_limit);
> +        if (rc != 0) {
> +            virReportSystemError(-rc,
> +                                 _("Unable to set memory hard limit for domain %s"),
> +                                 vm->def->name);
> +            goto cleanup;
> +        }
> +        if (vm->def->mem.soft_limit != 0) {
> +            rc = virCgroupSetMemorySoftLimit(cgroup, vm->def->mem.soft_limit);
> +            if (rc != 0) {
> +                virReportSystemError(-rc,
> +                                     _("Unable to set memory soft limit for domain %s"),
> +                                     vm->def->name);
> +                goto cleanup;
>              }
> +        }
>
> -            if (vm->def->mem.swap_hard_limit != 0) {
> -                rc = virCgroupSetMemSwapHardLimit(cgroup, vm->def->mem.swap_hard_limit);
> -                if (rc != 0) {
> -                    virReportSystemError(-rc,
> -                                         _("Unable to set swap hard limit for domain %s"),
> -                                         vm->def->name);
> -                    goto cleanup;
> -                }
> +        if (vm->def->mem.swap_hard_limit != 0) {
> +            rc = virCgroupSetMemSwapHardLimit(cgroup, vm->def->mem.swap_hard_limit);
> +            if (rc != 0) {
> +                virReportSystemError(-rc,
> +                                     _("Unable to set swap hard limit for domain %s"),
> +                                     vm->def->name);
> +                goto cleanup;
>              }
> -        } else {
> -            qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                            _("Memory cgroup is not available on this host"));
>          }
> +    } else if (vm->def->mem.hard_limit != 0 ||
> +               vm->def->mem.soft_limit != 0 ||
> +               vm->def->mem.swap_hard_limit != 0) {
> +        qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                        _("Memory cgroup is not available on this host"));
> +    } else {
> +        VIR_WARN("Could not autoset a RSS limit for domain %s", vm->def->name);
>      }
>
>      if (vm->def->cputune.shares != 0) {
> --
> 1.7.8.6
>

Looks like a good change. My only question would be if its better to
look up the guest video RAM instead of assuming that QEMU overhead +
guest video RAM will amount to no more than about 200MB (I say about
because of the 1.02 fudge factor). If its not really possible to grab
that or it just makes sense to go with a set value like this then ACK
from me.

-- 
Doug Goldstein