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

Re: [libvirt] [PATCH] virsh: fix memtune's help message for swap_hard_limit



On Thu, 03 Mar 2011 14:47:36 +0800
Osier Yang <jyang redhat com> wrote:
> > Yes, I think it's better. Should I prepare patches ? or you'll do ?
> 
> Let's see other guys's opinions before doing it, :)
> 

A patch (full change version) is here. maybe good input for discussion.

==
>From 0bceb6f204f7551c701d9f60fecd82562b38bd50 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa hiroyu jp fujitsu com>
Date: Fri, 4 Mar 2011 14:46:08 +0900
Subject: [PATCH] Rename swap_hard_limit as memswap_hard_limit

This patch affects qemu and lxc, using memory cgroup.

Linux's memory cgroup's memory.memsw.limit_in_bytes is a limit
for memory+swap, not for swap.
(This behavior is for avoiding bad influence for global vmscan
 and never disturb kswapd behavior by user's setting.)

libvirt's support for memsw.limit_in_bytes is named as
swap_hard_limit and documenteda as "hard limit for swap usage".
This is misleading.

This patch renames all swap_hard_limit things as memswap_hard_limit.

After this, domain's XML definition
      <swap_hard_limit> ... </swap_hard_limit>
will be
      <memswap_hard_limit> ...</memswap_hard_limit>

And macro VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT should be
VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT

virsh commands' --swap-hard-limit will be memswap-hard-limit

virCgroupGetSwapHardLimit will be virCgroupGetMemSwapHardLimit
will be
virCgroupSetSwapHardLimit will be virCgroupSetMemSwapHardLimit

This patch keeps backwoard compatibility in XML parser and
virsh memtune commandline argument. But it shows warning.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa hiroyu jp fujitsu com>
---
 docs/formatdomain.html.in                       |    9 ++++---
 docs/schemas/domain.rng                         |    2 +-
 include/libvirt/libvirt.h.in                    |   16 +++++++++---
 src/conf/domain_conf.c                          |   24 +++++++++++++------
 src/conf/domain_conf.h                          |    2 +-
 src/libvirt_private.syms                        |    4 +-
 src/lxc/lxc_controller.c                        |    8 +++---
 src/lxc/lxc_driver.c                            |   12 +++++-----
 src/qemu/qemu_cgroup.c                          |    5 ++-
 src/qemu/qemu_driver.c                          |   16 ++++++------
 src/util/cgroup.c                               |   10 ++++----
 src/util/cgroup.h                               |    4 +-
 tests/qemuxml2argvdata/qemuxml2argv-memtune.xml |    2 +-
 tools/virsh.c                                   |   28 ++++++++++++++--------
 14 files changed, 84 insertions(+), 58 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 84b1cab..60cf572 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -284,7 +284,7 @@
   &lt;memtune&gt;
     &lt;hard_limit&gt;1048576&lt;/hard_limit&gt;
     &lt;soft_limit&gt;131072&lt;/soft_limit&gt;
-    &lt;swap_hard_limit&gt;2097152&lt;/swap_hard_limit&gt;
+    &lt;memswap_hard_limit&gt;2097152&lt;/memswap_hard_limit&gt;
     &lt;min_guarantee&gt;65536&lt;/min_guarantee&gt;
   &lt;/memtune&gt;
   &lt;vcpu cpuset="1-4,^3,6" current="1"&gt;2&lt;/vcpu&gt;
@@ -323,10 +323,11 @@
       <dd> The optional <code>soft_limit</code> element is the memory limit to
         enforce during memory contention. The units for this value are
         kilobytes (i.e. blocks of 1024 bytes)</dd>
-      <dt><code>swap_hard_limit</code></dt>
-      <dd> The optional <code>swap_hard_limit</code> element is the maximum
-        swap the guest can use. The units for this value are kilobytes
+      <dt><code>memswap_hard_limit</code></dt>
+      <dd> The optional <code>memswap_hard_limit</code> element is the maximum
+        memory+swap the guest can use. The units for this value are kilobytes
         (i.e. blocks of 1024 bytes)</dd>
+        </span>
       <dt><code>min_guarantee</code></dt>
       <dd> The optional <code>min_guarantee</code> element is the guaranteed
         minimum memory allocation for the guest. The units for this value are
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
index 8b215f3..2ac50a4 100644
--- a/docs/schemas/domain.rng
+++ b/docs/schemas/domain.rng
@@ -341,7 +341,7 @@
           </optional>
           <!-- Maximum swap area the VM can use -->
           <optional>
-            <element name="swap_hard_limit">
+            <element name="memswap_hard_limit">
               <ref name="memoryKB"/>
             </element>
           </optional>
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 5dfb752..0b397b8 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -735,13 +735,21 @@ typedef enum {
 #define VIR_DOMAIN_MEMORY_MIN_GUARANTEE "min_guarantee"
 
 /**
- * VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT:
+ * VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT:
  *
- * Macro for the swap tunable swap_hard_limit: it represents the maximum swap
- * the guest can use.
+ * Macro for the swap tunable memswap_hard_limit: it represents the maximum
+ * memory+swap the guest can use.
  */
 
-#define VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT "swap_hard_limit"
+#define VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT "memswap_hard_limit"
+
+/**
+ * VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT
+ *
+ * macro for comaptibility with old version. please use
+ * VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT
+ */
+#define VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT "memswap_hard_limit"
 
 /**
  * virDomainMemoryParameter:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c8350c6..555e90e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5188,9 +5188,17 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
                       &def->mem.min_guarantee) < 0)
         def->mem.min_guarantee = 0;
 
-    if (virXPathULong("string(./memtune/swap_hard_limit[1])", ctxt,
-                      &def->mem.swap_hard_limit) < 0)
-        def->mem.swap_hard_limit = 0;
+    if (virXPathULong("string(./memtune/memswap_hard_limit[1])", ctxt,
+                      &def->mem.memswap_hard_limit) < 0)
+        def->mem.memswap_hard_limit = 0;
+    /* internal support for old style */
+    if (def->mem.memswap_hard_limit == 0 &&
+        virXPathULong("string(./memtune/swap_hard_limit[1])", ctxt,
+                      &def->mem.memswap_hard_limit) < 0) {
+        def->mem.memswap_hard_limit = 0;
+        VIR_WARN("%s", _("<swap_hard_limit> is obsolete, please use"
+                         "<memswap_hard_limit>"));
+    }
 
     n = virXPathULong("string(./vcpu[1])", ctxt, &count);
     if (n == -2) {
@@ -7681,7 +7689,7 @@ char *virDomainDefFormat(virDomainDefPtr def,
 
     /* add memtune only if there are any */
     if (def->mem.hard_limit || def->mem.soft_limit || def->mem.min_guarantee ||
-        def->mem.swap_hard_limit)
+        def->mem.memswap_hard_limit)
         virBufferVSprintf(&buf, "  <memtune>\n");
     if (def->mem.hard_limit) {
         virBufferVSprintf(&buf, "    <hard_limit>%lu</hard_limit>\n",
@@ -7695,12 +7703,12 @@ char *virDomainDefFormat(virDomainDefPtr def,
         virBufferVSprintf(&buf, "    <min_guarantee>%lu</min_guarantee>\n",
                           def->mem.min_guarantee);
     }
-    if (def->mem.swap_hard_limit) {
-        virBufferVSprintf(&buf, "    <swap_hard_limit>%lu</swap_hard_limit>\n",
-                          def->mem.swap_hard_limit);
+    if (def->mem.memswap_hard_limit) {
+        virBufferVSprintf(&buf, "    <memswap_hard_limit>%lu</memswap_hard_limit>\n",
+                          def->mem.memswap_hard_limit);
     }
     if (def->mem.hard_limit || def->mem.soft_limit || def->mem.min_guarantee ||
-        def->mem.swap_hard_limit)
+        def->mem.memswap_hard_limit)
         virBufferVSprintf(&buf, "  </memtune>\n");
 
     if (def->mem.hugepage_backed) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 30aeccc..f71321c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1053,7 +1053,7 @@ struct _virDomainDef {
         unsigned long hard_limit;
         unsigned long soft_limit;
         unsigned long min_guarantee;
-        unsigned long swap_hard_limit;
+        unsigned long memswap_hard_limit;
     } mem;
     unsigned short vcpus;
     unsigned short maxvcpus;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5e63a12..fce0fe5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -74,7 +74,7 @@ virCgroupGetFreezerState;
 virCgroupGetMemoryHardLimit;
 virCgroupGetMemorySoftLimit;
 virCgroupGetMemoryUsage;
-virCgroupGetSwapHardLimit;
+virCgroupGetMemSwapHardLimit;
 virCgroupKill;
 virCgroupKillRecursive;
 virCgroupKillPainfully;
@@ -86,7 +86,7 @@ virCgroupSetFreezerState;
 virCgroupSetMemory;
 virCgroupSetMemoryHardLimit;
 virCgroupSetMemorySoftLimit;
-virCgroupSetSwapHardLimit;
+virCgroupSetMemSwapHardLimit;
 
 
 # command.h
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index d2b113c..8a62eaa 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -145,12 +145,12 @@ static int lxcSetContainerResources(virDomainDefPtr def)
         }
     }
 
-    if(def->mem.swap_hard_limit) {
-        rc = virCgroupSetSwapHardLimit(cgroup, def->mem.swap_hard_limit);
+    if(def->mem.memswap_hard_limit) {
+        rc = virCgroupSetMemSwapHardLimit(cgroup, def->mem.memswap_hard_limit);
         if (rc != 0) {
             virReportSystemError(-rc,
-                                 _("Unable to set swap hard limit for domain %s"),
-                                 def->name);
+                    _("Unable to set memory+swap hard limit for domain %s"),
+                   def->name);
             goto cleanup;
         }
     }
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 5b6f784..1e3a05a 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -770,16 +770,16 @@ static int lxcDomainSetMemoryParameters(virDomainPtr dom,
                                      _("unable to set memory soft_limit tunable"));
                 ret = -1;
             }
-        } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT)) {
+        } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT)) {
             int rc;
             if (param->type != VIR_DOMAIN_MEMORY_PARAM_ULLONG) {
                 lxcError(VIR_ERR_INVALID_ARG, "%s",
-                         _("invalid type for swap_hard_limit tunable, expected a 'ullong'"));
+                         _("invalid type for memswap_hard_limit tunable, expected a 'ullong'"));
                 ret = -1;
                 continue;
             }
 
-            rc = virCgroupSetSwapHardLimit(cgroup, params[i].value.ul);
+            rc = virCgroupSetMemSwapHardLimit(cgroup, params[i].value.ul);
             if (rc != 0) {
                 virReportSystemError(-rc, "%s",
                                      _("unable to set swap_hard_limit tunable"));
@@ -885,15 +885,15 @@ static int lxcDomainGetMemoryParameters(virDomainPtr dom,
             break;
 
         case 2: /* fill swap hard limit here */
-            rc = virCgroupGetSwapHardLimit(cgroup, &val);
+            rc = virCgroupGetMemSwapHardLimit(cgroup, &val);
             if (rc != 0) {
                 virReportSystemError(-rc, "%s",
                                      _("unable to get swap hard limit"));
                 goto cleanup;
             }
-            if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT) == NULL) {
+            if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT) == NULL) {
                 lxcError(VIR_ERR_INTERNAL_ERROR,
-                         "%s", _("Field swap hard limit too long for destination"));
+                  "%s", _("Field memory+swap hard limit too long for destination"));
                 goto cleanup;
             }
             param->value.ul = val;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index e71d3fa..0bac1be 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -330,8 +330,9 @@ int qemuSetupCgroup(struct qemud_driver *driver,
             }
         }
 
-        if (vm->def->mem.swap_hard_limit != 0) {
-            rc = virCgroupSetSwapHardLimit(cgroup, vm->def->mem.swap_hard_limit);
+        if (vm->def->mem.memswap_hard_limit != 0) {
+            rc = virCgroupSetMemSwapHardLimit(cgroup,
+                        vm->def->mem.memswap_hard_limit);
             if (rc != 0) {
                 virReportSystemError(-rc,
                                      _("Unable to set swap hard limit for domain %s"),
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c9095bb..9f55ca8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4521,19 +4521,19 @@ static int qemuDomainSetMemoryParameters(virDomainPtr dom,
                                      _("unable to set memory soft_limit tunable"));
                 ret = -1;
             }
-        } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT)) {
+        } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT)) {
             int rc;
             if (param->type != VIR_DOMAIN_MEMORY_PARAM_ULLONG) {
                 qemuReportError(VIR_ERR_INVALID_ARG, "%s",
-                                _("invalid type for swap_hard_limit tunable, expected a 'ullong'"));
+                                _("invalid type for memswap_hard_limit tunable, expected a 'ullong'"));
                 ret = -1;
                 continue;
             }
 
-            rc = virCgroupSetSwapHardLimit(group, params[i].value.ul);
+            rc = virCgroupSetMemSwapHardLimit(group, params[i].value.ul);
             if (rc != 0) {
                 virReportSystemError(-rc, "%s",
-                                     _("unable to set swap_hard_limit tunable"));
+                                _("unable to set memswap_hard_limit tunable"));
                 ret = -1;
             }
         } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_MIN_GUARANTEE)) {
@@ -4641,15 +4641,15 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom,
             break;
 
         case 2: /* fill swap hard limit here */
-            rc = virCgroupGetSwapHardLimit(group, &val);
+            rc = virCgroupGetMemSwapHardLimit(group, &val);
             if (rc != 0) {
                 virReportSystemError(-rc, "%s",
-                                     _("unable to get swap hard limit"));
+                                     _("unable to get memory+swap hard limit"));
                 goto cleanup;
             }
-            if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT) == NULL) {
+            if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT) == NULL) {
                 qemuReportError(VIR_ERR_INTERNAL_ERROR,
-                                "%s", _("Field swap hard limit too long for destination"));
+                    "%s", _("Field memory+swap hard limit too long for destination"));
                 goto cleanup;
             }
             param->value.ul = val;
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index c5b8cdd..a2de923 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -1033,14 +1033,14 @@ int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long long *kb)
 }
 
 /**
- * virCgroupSetSwapHardLimit:
+ * virCgroupSetMemSwapHardLimit:
  *
- * @group: The cgroup to change swap hard limit for
+ * @group: The cgroup to change memory+swap hard limit for
  * @kb: The swap amount in kilobytes
  *
  * Returns: 0 on success
  */
-int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long long kb)
+int virCgroupSetMemSwapHardLimit(virCgroupPtr group, unsigned long long kb)
 {
     unsigned long long maxkb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
 
@@ -1061,12 +1061,12 @@ int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long long kb)
 /**
  * virCgroupGetSwapHardLimit:
  *
- * @group: The cgroup to get swap hard limit for
+ * @group: The cgroup to get memory+swap hard limit for
  * @kb: The swap amount in kilobytes
  *
  * Returns: 0 on success
  */
-int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long long *kb)
+int virCgroupGetMemSwapHardLimit(virCgroupPtr group, unsigned long long *kb)
 {
     long long unsigned int limit_in_bytes;
     int ret;
diff --git a/src/util/cgroup.h b/src/util/cgroup.h
index d468cb3..fd16a40 100644
--- a/src/util/cgroup.h
+++ b/src/util/cgroup.h
@@ -52,8 +52,8 @@ int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long long kb);
 int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long long *kb);
 int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long long kb);
 int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long long *kb);
-int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long long kb);
-int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long long *kb);
+int virCgroupSetMemSwapHardLimit(virCgroupPtr group, unsigned long long kb);
+int virCgroupGetMemSwapHardLimit(virCgroupPtr group, unsigned long long *kb);
 
 int virCgroupDenyAllDevices(virCgroupPtr group);
 
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml b/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml
index 37b5c88..e0a1825 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml
@@ -6,7 +6,7 @@
   <memtune>
     <hard_limit>512000</hard_limit>
     <soft_limit>128000</soft_limit>
-    <swap_hard_limit>1024000</swap_hard_limit>
+    <memswap_hard_limit>1024000</memswap_hard_limit>
   </memtune>
   <vcpu>1</vcpu>
   <os>
diff --git a/tools/virsh.c b/tools/virsh.c
index 62fca17..62df3b9 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -3032,8 +3032,8 @@ static const vshCmdOptDef opts_memtune[] = {
      N_("Max memory in kilobytes")},
     {"soft-limit", VSH_OT_INT, VSH_OFLAG_NONE,
      N_("Memory during contention in kilobytes")},
-    {"swap-hard-limit", VSH_OT_INT, VSH_OFLAG_NONE,
-     N_("Max swap in kilobytes")},
+    {"memswap-hard-limit", VSH_OT_INT, VSH_OFLAG_NONE,
+     N_("Max memory+swap in kilobytes")},
     {"min-guarantee", VSH_OT_INT, VSH_OFLAG_NONE,
      N_("Min guaranteed memory in kilobytes")},
     {NULL, 0, 0, NULL}
@@ -3043,7 +3043,7 @@ static int
 cmdMemtune(vshControl * ctl, const vshCmd * cmd)
 {
     virDomainPtr dom;
-    long long hard_limit, soft_limit, swap_hard_limit, min_guarantee;
+    long long hard_limit, soft_limit, memswap_hard_limit, min_guarantee;
     int nparams = 0;
     unsigned int i = 0;
     virMemoryParameterPtr params = NULL, temp = NULL;
@@ -3065,10 +3065,18 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd)
     if (soft_limit)
         nparams++;
 
-    swap_hard_limit =
-        vshCommandOptLongLong(cmd, "swap-hard-limit", NULL);
-    if (swap_hard_limit)
+    memswap_hard_limit =
+        vshCommandOptLongLong(cmd, "memswap-hard-limit", NULL);
+    if (memswap_hard_limit) {
         nparams++;
+    } else { /* for backward compatibility */
+        memswap_hard_limit =
+            vshCommandOptLongLong(cmd, "swap-hard-limit", NULL);
+        if (memswap_hard_limit)
+            vshError(ctl, "%s",
+               _("Warning swap_hard_limit is obsolete,use memswap_hard_limit"));
+        goto cleanup;
+    }
 
     min_guarantee =
         vshCommandOptLongLong(cmd, "min-guarantee", NULL);
@@ -3155,11 +3163,11 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd)
                 strncpy(temp->field, VIR_DOMAIN_MEMORY_HARD_LIMIT,
                         sizeof(temp->field));
                 hard_limit = 0;
-            } else if (swap_hard_limit) {
-                temp->value.ul = swap_hard_limit;
-                strncpy(temp->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT,
+            } else if (memswap_hard_limit) {
+                temp->value.ul = memswap_hard_limit;
+                strncpy(temp->field, VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT,
                         sizeof(temp->field));
-                swap_hard_limit = 0;
+                memswap_hard_limit = 0;
             } else if (min_guarantee) {
                 temp->value.ul = min_guarantee;
                 strncpy(temp->field, VIR_DOMAIN_MEMORY_MIN_GUARANTEE,
-- 
1.7.4.1



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