On Fri, Jun 28, 2013 at 17:24:42 -0400, Laine Stump wrote:
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.
That's because previously this computation was made only for hard_limit and the limit for locking was just completely ignoring it and used max_balloon + 1GB. However, this computation does not count with VFIO in any way so I just combined the two computations by adding the extra 1GB for VFIO into this shared code (in patch 2/3). I'm not sure if that's the right think to do or not but it's certainly better than before when memory locking limit completely ignore the need for VRAM and per-disk cache. Unfortunately, the original formula was suggested by Avi, who moved to new challenges. Perhaps others could jump in and share their opinions (Paolo? :-P). Be careful, though, that we're actually discussing qemuDomainMemoryLimit code after patch 2/3 is applied, that is with the VFIO stuff counted in. The complete code of this function is: 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. * * Moreover, VFIO requires some amount for IO space. Alex Williamson * suggested adding 1GiB for IO space just to be safe (some finer * tuning might be nice, though). */ 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; for (i = 0; i < def->nhostdevs; i++) { virDomainHostdevDefPtr hostdev = def->hostdevs[i]; if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && hostdev->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) { mem += 1024 * 1024; break; } } } return mem; } Jirka