[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 20/05/13 19:08, Daniel P. Berrange wrote:
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

I found no trailing spaces.


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

Okay.


+    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.

Agreed. Having a warning for no XML config is confused. I removed it.


@@ -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


Pushed. Thanks.

Osier


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]