Re: [libvirt] [PATCH 2/7] qemu: Abstract code for memory controller setting into a helper

On Fri, May 17, 2013 at 07:59:32PM +0800, Osier Yang wrote: > --- > src/qemu/qemu_cgroup.c | 120 ++++++++++++++++++++++++++++--------------------- > 1 file changed, 68 insertions(+), 52 deletions(-) > > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c > index 0c4792e..2751be0 100644 > --- a/src/qemu/qemu_cgroup.c > +++ b/src/qemu/qemu_cgroup.c > @@ -455,6 +455,72 @@ qemuSetupBlkioCgroup(virDomainObjPtr vm) > return 0; > } > My email client is showing trailing whitespace here. Not sure if that is genuine or not. Also 2 blank lines between functions

> +static int > +qemuSetupMemoryCgroup(virDomainObjPtr vm) { Put the '{' on a new line as you did with previous patches

> + 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 || > + vm->def->mem.soft_limit != 0 || > + vm->def->mem.swap_hard_limit != 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Memory cgroup is not available on this host")); > + return -1; > + } else { > + VIR_WARN("Could not autoset a RSS limit for domain %s", vm->def->name); > + return 0; > + } Not sure why we need this VIR_WARN at all. If no limits are set in the XML, then we should not warn about a missing feature that we don't actually need.

> @@ -668,58 +734,8 @@ int qemuSetupCgroup(virQEMUDriverPtr driver, > if (qemuSetupBlkioCgroup(vm) < 0) > goto cleanup; > > - if (virCgroupHasController(priv->cgroup, 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 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); > - 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(priv->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(priv->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 if (vm->def->mem.hard_limit != 0 || > - vm->def->mem.soft_limit != 0 || > - vm->def->mem.swap_hard_limit != 0) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("Memory cgroup is not available on this host")); > - } else { > - VIR_WARN("Could not autoset a RSS limit for domain %s", vm->def->name); > - } > + if (qemuSetupMemoryCgroup(vm) < 0) > + goto cleanup; ACK with warning removed