[libvirt] [PATCH] virsh: fix memtune's help message for swap_hard_limit
I found this when I used virsh memtune... BTW, how to fix http://libvirt.org/formatdomain.html 's memtune description ? ==
From 541ae04430f376e8168b413a20b35dce49779816 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 14:24:45 +0900 Subject: [PATCH 6/6] fix virsh commands' message for memtune'swap_hard_limit.
cgroup's /cgroup/memory/memory.memsw.limit_in_bytes is not for limitinit 'swap' but for 'memory+swap' (then, it's memsw) (So, this number cannot be smaller than memory.limit_in_bytes) Note: If other hypervisors than Linux support this and meaning is not same as memory+swap, the name swap_hard_limit will have confusion. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- tools/virsh.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 62fca17..3a8fdaa 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3033,7 +3033,7 @@ static const vshCmdOptDef opts_memtune[] = { {"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")}, + N_("Max memory+swap in kilobytes")}, {"min-guarantee", VSH_OT_INT, VSH_OFLAG_NONE, N_("Min guaranteed memory in kilobytes")}, {NULL, 0, 0, NULL} -- 1.7.4.1
于 2011年03月03日 13:25, KAMEZAWA Hiroyuki 写道:
I found this when I used virsh memtune...
Happened to see a bug for this yesterday.
BTW, how to fix http://libvirt.org/formatdomain.html 's memtune description ?
How about be consitent with what cgroup doc says? :-) http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=blob;f=Docum...
==
From 541ae04430f376e8168b413a20b35dce49779816 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 14:24:45 +0900 Subject: [PATCH 6/6] fix virsh commands' message for memtune'swap_hard_limit.
cgroup's /cgroup/memory/memory.memsw.limit_in_bytes is not for limitinit 'swap' but for 'memory+swap' (then, it's memsw) (So, this number cannot be smaller than memory.limit_in_bytes)
Yes, that's what BZ about.
Note: If other hypervisors than Linux support this and meaning is not same as memory+swap, the name swap_hard_limit will have confusion.
Currently, only LXC and QEMU driver support "swap_hard_limit" using cgroup, ESX just support setting "min_guarantee", so perhaps renaming "swap_hard_limit" to something like "memswap_hard_limit" is a good idea?
Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com> --- tools/virsh.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 62fca17..3a8fdaa 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3033,7 +3033,7 @@ static const vshCmdOptDef opts_memtune[] = { {"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")}, + N_("Max memory+swap in kilobytes")}, {"min-guarantee", VSH_OT_INT, VSH_OFLAG_NONE, N_("Min guaranteed memory in kilobytes")}, {NULL, 0, 0, NULL}
On Thu, 03 Mar 2011 14:02:32 +0800 Osier Yang <jyang@redhat.com> wrote:
于 2011年03月03日 13:25, KAMEZAWA Hiroyuki 写道:
I found this when I used virsh memtune...
Happened to see a bug for this yesterday.
Oh ;)
BTW, how to fix http://libvirt.org/formatdomain.html 's memtune description ?
How about be consitent with what cgroup doc says? :-)
http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=blob;f=Docum...
memory.memsw.usage_in_bytes # show current memory+Swap usage memory.limit_in_bytes # set/show limit of memory usage memory.memsw.limit_in_bytes # set/show limit of memory+Swap usage memory.failcnt # show the number of memory usage hits limits Maybe nice.
==
From 541ae04430f376e8168b413a20b35dce49779816 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 14:24:45 +0900 Subject: [PATCH 6/6] fix virsh commands' message for memtune'swap_hard_limit.
cgroup's /cgroup/memory/memory.memsw.limit_in_bytes is not for limitinit 'swap' but for 'memory+swap' (then, it's memsw) (So, this number cannot be smaller than memory.limit_in_bytes)
Yes, that's what BZ about.
Note: If other hypervisors than Linux support this and meaning is not same as memory+swap, the name swap_hard_limit will have confusion.
Currently, only LXC and QEMU driver support "swap_hard_limit" using cgroup, ESX just support setting "min_guarantee", so perhaps renaming "swap_hard_limit" to something like "memswap_hard_limit" is a good idea?
Yes, I think it's better. Should I prepare patches ? or you'll do ? Thanks, -Kame
于 2011年03月03日 14:10, KAMEZAWA Hiroyuki 写道:
On Thu, 03 Mar 2011 14:02:32 +0800 Osier Yang<jyang@redhat.com> wrote:
于 2011年03月03日 13:25, KAMEZAWA Hiroyuki 写道:
I found this when I used virsh memtune...
Happened to see a bug for this yesterday.
Oh ;)
BTW, how to fix http://libvirt.org/formatdomain.html 's memtune description ?
How about be consitent with what cgroup doc says? :-)
http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=blob;f=Docum...
memory.memsw.usage_in_bytes # show current memory+Swap usage memory.limit_in_bytes # set/show limit of memory usage memory.memsw.limit_in_bytes # set/show limit of memory+Swap usage memory.failcnt # show the number of memory usage hits limits
Maybe nice.
==
From 541ae04430f376e8168b413a20b35dce49779816 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 14:24:45 +0900 Subject: [PATCH 6/6] fix virsh commands' message for memtune'swap_hard_limit.
cgroup's /cgroup/memory/memory.memsw.limit_in_bytes is not for limitinit 'swap' but for 'memory+swap' (then, it's memsw) (So, this number cannot be smaller than memory.limit_in_bytes)
Yes, that's what BZ about.
Note: If other hypervisors than Linux support this and meaning is not same as memory+swap, the name swap_hard_limit will have confusion.
Currently, only LXC and QEMU driver support "swap_hard_limit" using cgroup, ESX just support setting "min_guarantee", so perhaps renaming "swap_hard_limit" to something like "memswap_hard_limit" is a good idea?
Yes, I think it's better. Should I prepare patches ? or you'll do ?
Let's see other guys's opinions before doing it, :) Regards Osier
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 @@ <memtune> <hard_limit>1048576</hard_limit> <soft_limit>131072</soft_limit> - <swap_hard_limit>2097152</swap_hard_limit> + <memswap_hard_limit>2097152</memswap_hard_limit> <min_guarantee>65536</min_guarantee> </memtune> <vcpu cpuset="1-4,^3,6" current="1">2</vcpu> @@ -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
On Fri, 4 Mar 2011 14:46:12 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
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.)
Argh, thats me... :( Thanks for the patch Kame. Reviewed-by: "Nikunj A. Dadhania" <nikunj@linux.vnet.ibm.com> Regards, Nikunj
On Mon, 07 Mar 2011 15:11:12 +0530 "Nikunj A. Dadhania" <nikunj@linux.vnet.ibm.com> wrote:
Argh, thats me... :( Thanks for the patch Kame.
Reviewed-by: "Nikunj A. Dadhania" <nikunj@linux.vnet.ibm.com>
Thanks, rebased onto the latest git tree. ==
From 421a27337458648b808f0e45cdfd83192313d7cc Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 9 Mar 2011 17:18:41 +0900
Subject: [PATCH] rename swap_limit as memswap_limit. cgroup's /cgroup/memory/memory.memsw.limit_in_bytes is not for limitinit 'swap' but for 'memory+swap' (then, it's memsw) (So, this number cannot be smaller than memory.limit_in_bytes) Note: If other hypervisors than Linux support this and meaning is not same as memory+swap, the name swap_hard_limit will have confusion. Changelog: - rebased onto the latest libvirt-git. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Reviewed-by: "Nikunj A. Dadhania" <nikunj@linux.vnet.ibm.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 | 18 ++++++++-------- 14 files changed, 75 insertions(+), 57 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0c7c965..f764d09 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -284,7 +284,7 @@ <memtune> <hard_limit>1048576</hard_limit> <soft_limit>131072</soft_limit> - <swap_hard_limit>2097152</swap_hard_limit> + <memswap_hard_limit>2097152</memswap_hard_limit> <min_guarantee>65536</min_guarantee> </memtune> <vcpu cpuset="1-4,^3,6" current="1">2</vcpu> @@ -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 618b350..d2600fa 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 16e1291..fdf37e1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5192,9 +5192,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) { @@ -7685,7 +7693,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", @@ -7699,12 +7707,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 efcf3c5..f7a308e 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 0f7cbad..3897447 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4522,19 +4522,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)) { @@ -4642,15 +4642,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 8551acd..6e38ef5 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 14c6d6b..b790248 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3034,8 +3034,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} @@ -3045,7 +3045,7 @@ static int cmdMemtune(vshControl * ctl, const vshCmd * cmd) { virDomainPtr dom; - long long hard_limit = 0, soft_limit = 0, swap_hard_limit = 0; + long long hard_limit = 0, soft_limit = 0, memswap_hard_limit = 0; long long min_guarantee = 0; int nparams = 0; unsigned int i = 0; @@ -3060,7 +3060,7 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) if (vshCommandOptLongLong(cmd, "hard-limit", &hard_limit) < 0 || vshCommandOptLongLong(cmd, "soft-limit", &soft_limit) < 0 || - vshCommandOptLongLong(cmd, "swap-hard-limit", &swap_hard_limit) < 0 || + vshCommandOptLongLong(cmd, "swap-hard-limit", &memswap_hard_limit) < 0 || vshCommandOptLongLong(cmd, "min-guarantee", &min_guarantee) < 0) { vshError(ctl, "%s", _("Unable to parse integer parameter")); @@ -3073,7 +3073,7 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) if (soft_limit) nparams++; - if (swap_hard_limit) + if (memswap_hard_limit) nparams++; if (min_guarantee) @@ -3159,11 +3159,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
On 03/09/2011 01:13 AM, KAMEZAWA Hiroyuki wrote:
+++ 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>
This needs fixing. We need to support both old and new styles, since we've had a release that used the old name. That means we need to add a new test, rather than modifying an existing test, to show that we can parse both styles when reading XML, but that we generate the new style when writing. Maybe danpb or DV will have some further comments on whether it is wise to deprecate XML like this; it's unpleasant business to think about backwards compatibility. We might be stuck with just documenting that the choice of name was unfortunate to avoid the hassle of back-compat; but I hope it doesn't come to that.
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 618b350..d2600fa 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"
Good, you didn't delete the old name. However, you no longer have any code that recognizes the old string value of the name. Suppose that virsh from 0.8.8 is talking to libvirtd from 0.9.0 - that means libvirtd has to recognize the old name as well as the new name. (The astute reader might complain: "But the #define for memswap_hard_limit was already renamed once before, in commit 916f95b." However, when you realize that the option was introduced in commit bf1b76f, and that both commit 916f95b and bf1b76f were first released as part of 0.8.5, therefore there was no release made with the intermediate name, so that particular rename did not have any back-compat issues like this proposed rename).
/** * virDomainMemoryParameter: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 16e1291..fdf37e1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5192,9 +5192,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>")); + }
Good - this still parses the old XML, even though the .rng file didn't mention it.
+++ 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)) {
Bad - here's where you forgot to also check STREQ against the old spelling of the limit name.
+++ b/src/qemu/qemu_driver.c @@ -4522,19 +4522,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)) {
Likewise.
--- 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>
Back to my earlier complaint that you need to test both old and new naming.
diff --git a/tools/virsh.c b/tools/virsh.c index 14c6d6b..b790248 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3034,8 +3034,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")},
This name change might break existing clients of virsh. Is there any way to support the older name? Maybe we need to add some more framework into virsh option parsing to allow option aliases that the parser recognizes but which don't get documented, so that someone still doing --swap-hard-limit will work? I guess I need more convincing that this rename is worth the hassle, or whether we are stuck with the smaller but less controversial patch of documenting that the name isn't optimal for what it really represents. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Mar 14, 2011 at 09:24:58PM -0600, Eric Blake wrote:
On 03/09/2011 01:13 AM, KAMEZAWA Hiroyuki wrote:
+++ 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>
This needs fixing. We need to support both old and new styles, since we've had a release that used the old name. That means we need to add a new test, rather than modifying an existing test, to show that we can parse both styles when reading XML, but that we generate the new style when writing.
Adding back-compat hacks are only reasonable if we have already had an accident & mistakenly changed public facing XML/API. Given, that we're not in that situation, we should simply not make the proposed changes to the XML/API, rather than changing it & adding compat hacks.
Maybe danpb or DV will have some further comments on whether it is wise to deprecate XML like this; it's unpleasant business to think about backwards compatibility. We might be stuck with just documenting that the choice of name was unfortunate to avoid the hassle of back-compat; but I hope it doesn't come to that.
diff --git a/tools/virsh.c b/tools/virsh.c index 14c6d6b..b790248 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3034,8 +3034,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")},
This name change might break existing clients of virsh. Is there any way to support the older name? Maybe we need to add some more framework into virsh option parsing to allow option aliases that the parser recognizes but which don't get documented, so that someone still doing --swap-hard-limit will work?
Agreed, we shouldn't change this either.
I guess I need more convincing that this rename is worth the hassle, or whether we are stuck with the smaller but less controversial patch of documenting that the name isn't optimal for what it really represents.
Agreed, we should simply improve docs for the existing names to clarify their meaning. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, 15 Mar 2011 14:30:02 +0000 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Mar 14, 2011 at 09:24:58PM -0600, Eric Blake wrote:
On 03/09/2011 01:13 AM, KAMEZAWA Hiroyuki wrote:
+++ 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>
This needs fixing. We need to support both old and new styles, since we've had a release that used the old name. That means we need to add a new test, rather than modifying an existing test, to show that we can parse both styles when reading XML, but that we generate the new style when writing.
Adding back-compat hacks are only reasonable if we have already had an accident & mistakenly changed public facing XML/API. Given, that we're not in that situation, we should simply not make the proposed changes to the XML/API, rather than changing it & adding compat hacks.
Maybe danpb or DV will have some further comments on whether it is wise to deprecate XML like this; it's unpleasant business to think about backwards compatibility. We might be stuck with just documenting that the choice of name was unfortunate to avoid the hassle of back-compat; but I hope it doesn't come to that.
diff --git a/tools/virsh.c b/tools/virsh.c index 14c6d6b..b790248 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3034,8 +3034,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")},
This name change might break existing clients of virsh. Is there any way to support the older name? Maybe we need to add some more framework into virsh option parsing to allow option aliases that the parser recognizes but which don't get documented, so that someone still doing --swap-hard-limit will work?
Agreed, we shouldn't change this either.
I guess I need more convincing that this rename is worth the hassle, or whether we are stuck with the smaller but less controversial patch of documenting that the name isn't optimal for what it really represents.
Agreed, we should simply improve docs for the existing names to clarify their meaning.
Ok, I'll stop. -Kame
On Wed, Mar 09, 2011 at 05:13:47PM +0900, KAMEZAWA Hiroyuki wrote:
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 618b350..d2600fa 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:
NACK to both these changes. The XML and public API must *never* be changed once included in a release. The current names may not be the perfect choices, but we can't change them now I'm afraid. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, 15 Mar 2011 14:27:19 +0000 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Mar 09, 2011 at 05:13:47PM +0900, KAMEZAWA Hiroyuki wrote:
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 618b350..d2600fa 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:
NACK to both these changes. The XML and public API must *never* be changed once included in a release. The current names may not be the perfect choices, but we can't change them now I'm afraid.
Hmm. Then, only messages should be fixed. Nikunj, please fix. Thanks, -Kame
On Wed, 16 Mar 2011 09:37:32 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
On Tue, 15 Mar 2011 14:27:19 +0000 "Daniel P. Berrange" <berrange@redhat.com> wrote:
NACK to both these changes. The XML and public API must *never* be changed once included in a release. The current names may not be the perfect choices, but we can't change them now I'm afraid.
Hmm. Then, only messages should be fixed. Nikunj, please fix.
Ok, here is the patch with doc changes and cgroup api rename == From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> * Correct the documentation for cgroup: the swap_hard_limit indicates mem+swap_hard_limit. * Change cgroup private apis to: virCgroupGet/SetMemSwapHardLimit Signed-off-by: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 5 +++-- include/libvirt/libvirt.h.in | 3 ++- src/libvirt_private.syms | 4 ++-- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 4 ++-- src/qemu/qemu_cgroup.c | 2 +- src/qemu/qemu_driver.c | 6 +++--- src/util/cgroup.c | 16 ++++++++-------- src/util/cgroup.h | 4 ++-- 9 files changed, 24 insertions(+), 22 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index dad268d..52bd4e6 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -231,8 +231,9 @@ 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 - (i.e. blocks of 1024 bytes)</dd> + memory plus swap the guest can use. The units for this value are + kilobytes (i.e. blocks of 1024 bytes). This has to be more than + hard_limit value provided</dd> <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/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 055eb2e..87bb9c5 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -729,7 +729,8 @@ typedef enum { * VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT: * * Macro for the swap tunable swap_hard_limit: it represents the maximum swap - * the guest can use. + * plus memory the guest can use. This limit has to be more than + * VIR_DOMAIN_MEMORY_HARD_LIMIT. */ #define VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT "swap_hard_limit" diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2ce4bed..025988e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -72,7 +72,7 @@ virCgroupGetFreezerState; virCgroupGetMemoryHardLimit; virCgroupGetMemorySoftLimit; virCgroupGetMemoryUsage; -virCgroupGetSwapHardLimit; +virCgroupGetMemSwapHardLimit; virCgroupMounted; virCgroupRemove; virCgroupSetCpuShares; @@ -80,7 +80,7 @@ virCgroupSetFreezerState; virCgroupSetMemory; virCgroupSetMemoryHardLimit; virCgroupSetMemorySoftLimit; -virCgroupSetSwapHardLimit; +virCgroupSetMemSwapHardLimit; # command.h diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index af0b70c..cfb0356 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -135,7 +135,7 @@ static int lxcSetContainerResources(virDomainDefPtr def) } if(def->mem.swap_hard_limit) { - rc = virCgroupSetSwapHardLimit(cgroup, def->mem.swap_hard_limit); + rc = virCgroupSetMemSwapHardLimit(cgroup, def->mem.swap_hard_limit); if (rc != 0) { virReportSystemError(-rc, _("Unable to set swap hard limit for domain %s"), diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index c4fe936..0eca48e 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -780,7 +780,7 @@ static int lxcDomainSetMemoryParameters(virDomainPtr dom, 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")); @@ -886,7 +886,7 @@ 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")); diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index e5536c0..a25b486 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -291,7 +291,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, } if (vm->def->mem.swap_hard_limit != 0) { - rc = virCgroupSetSwapHardLimit(cgroup, vm->def->mem.swap_hard_limit); + rc = virCgroupSetMemSwapHardLimit(cgroup, vm->def->mem.swap_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 395f72f..9b5fdec 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7002,7 +7002,7 @@ static int qemuDomainSetMemoryParameters(virDomainPtr dom, 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")); @@ -7112,8 +7112,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, param->value.ul = val; break; - case 2: /* fill swap hard limit here */ - rc = virCgroupGetSwapHardLimit(group, &val); + case 2: /* fill mem+swap hard limit here */ + rc = virCgroupGetMemSwapHardLimit(group, &val); if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to get swap hard limit")); diff --git a/src/util/cgroup.c b/src/util/cgroup.c index cd9caba..71c928e 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -977,14 +977,14 @@ int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long long *kb) } /** - * virCgroupSetSwapHardLimit: + * virCgroupSetMemSwapHardLimit: * - * @group: The cgroup to change swap hard limit for - * @kb: The swap amount in kilobytes + * @group: The cgroup to change mem+swap hard limit for + * @kb: The mem+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; @@ -1003,14 +1003,14 @@ int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long long kb) } /** - * virCgroupGetSwapHardLimit: + * virCgroupGetMemSwapHardLimit: * - * @group: The cgroup to get swap hard limit for - * @kb: The swap amount in kilobytes + * @group: The cgroup to get mem+swap hard limit for + * @kb: The mem+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 964da7a..8333b1b 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -47,8 +47,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);
On 03/15/2011 11:07 PM, Nikunj A. Dadhania wrote:
Hmm. Then, only messages should be fixed. Nikunj, please fix.
Ok, here is the patch with doc changes and cgroup api rename
== From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com>
* Correct the documentation for cgroup: the swap_hard_limit indicates mem+swap_hard_limit. * Change cgroup private apis to: virCgroupGet/SetMemSwapHardLimit
Signed-off-by: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com>
Hmm, I had merge conflicts when applying this. You may want to double check the result; it looks like you haven't updated your copy of git in a while. It's good to use 'git rebase' before posting updated patches; personally, I like the results of 'git config branch.master.rebase true' which tells git to automatically rebase my pending changes on top of the latest upstream any time I do 'git pull'.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index dad268d..52bd4e6 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -231,8 +231,9 @@ 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 - (i.e. blocks of 1024 bytes)</dd> + memory plus swap the guest can use. The units for this value are + kilobytes (i.e. blocks of 1024 bytes). This has to be more than + hard_limit value provided</dd>
No TABs in html.in. 'make syntax-check' should warn on that since commit 1652fa2f on 9 Feb. ACK that this version only touches public docs and private functions, which is acceptable. Once I rebased it onto modern libvirt.git, I pushed it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/09/2011 01:13 AM, KAMEZAWA Hiroyuki wrote:
Subject: [PATCH] rename swap_limit as memswap_limit.
cgroup's /cgroup/memory/memory.memsw.limit_in_bytes is not for limitinit 'swap' but for 'memory+swap' (then, it's memsw) (So, this number cannot be smaller than memory.limit_in_bytes)
Note: @@ -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
For the record - patching this sentence to mention that <code>swap_hard_limit</code> covers memory plus swap is reasonable,
/** - * 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.
as is updating this documentation sentence (but not renaming the macro). I'd rather see 'memory plus swap' than 'memory+swap' in documentation.
-virCgroupGetSwapHardLimit; +virCgroupGetMemSwapHardLimit;
Renaming this internal virCgroup helper function is doable; I'm not sure whether we need that rename, but it doesn't hurt (anything in libvirt_private.syms can be renamed or API altered at will).
- 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"));
Tweaking the error message is okay. Not very many places left in this patch that are worth keeping, but at least you've already identified where to document things. One alternative idea is leaving the field with the name swap_hard_limit, and manually computing that limit that we write to the cgroup file by adding in the memory hard limit, and when reading back from a cgroup file, doing two reads and the subtraction so that the user is just presented with a swap limit. That is, change the API to do what the name implies, instead of changing the name to match what the underlying implementation expects, by making libvirt be the glue that does the math for you. However, that's potentially confusing in its own right, since swap limit isn't a notion directly visible in the cgroup controller, and also not quite backwards compatible (it doesn't break API, but older guests might not be expecting the newer semantics and use an incorrect size for a limit when expecting the cgroup semantics). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (5)
-
Daniel P. Berrange -
Eric Blake -
KAMEZAWA Hiroyuki -
Nikunj A. Dadhania -
Osier Yang