[libvirt] Bug in virsh memtune on RHEL6?
Hi all, Getting strange numbers from the virsh memtune command on RHEL 6. (git head compiled, rather than RHEL 6 packages) virsh # list Id Name State ---------------------------------- 4 Fedora_14_x64 running virsh # memtune Fedora_14_x64 hard_limit : 9007199254740991 soft_limit : 9007199254740991 swap_hard_limit: 9007199254740991 Those numbers are supposed to be kilobytes, but the host box itself only has 12GB ram. Bug? Regards and best wishes, Justin Clift
On 01/06/2011 08:29 AM, Justin Clift wrote:
Hi all,
Getting strange numbers from the virsh memtune command on RHEL 6. (git head compiled, rather than RHEL 6 packages)
virsh # list Id Name State ---------------------------------- 4 Fedora_14_x64 running
virsh # memtune Fedora_14_x64 hard_limit : 9007199254740991 soft_limit : 9007199254740991 swap_hard_limit: 9007199254740991
Those numbers are supposed to be kilobytes, but the host box itself only has 12GB ram.
Bug?
Smells like a case of mistakenly treating either -1 or INT64_MAX as a valid value (that integer is 0x1f_ffff_ffff_ffff, which happens to be UINT64_MAX>>11 or INT64_MAX>>10), rather than recognizing it as meaning unlimited or undeterminable. If unlimited is the intended meaning, virsh can probably be taught to display it differently, perhaps by also referencing the host's limits. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/01/2011, at 2:48 AM, Eric Blake wrote:
On 01/06/2011 08:29 AM, Justin Clift wrote:
Hi all,
Getting strange numbers from the virsh memtune command on RHEL 6. (git head compiled, rather than RHEL 6 packages)
virsh # list Id Name State ---------------------------------- 4 Fedora_14_x64 running
virsh # memtune Fedora_14_x64 hard_limit : 9007199254740991 soft_limit : 9007199254740991 swap_hard_limit: 9007199254740991
Those numbers are supposed to be kilobytes, but the host box itself only has 12GB ram.
Bug?
Smells like a case of mistakenly treating either -1 or INT64_MAX as a valid value (that integer is 0x1f_ffff_ffff_ffff, which happens to be UINT64_MAX>>11 or INT64_MAX>>10), rather than recognizing it as meaning unlimited or undeterminable. If unlimited is the intended meaning, virsh can probably be taught to display it differently, perhaps by also referencing the host's limits.
Yeah, that sounds like it might be the cause. Nikunj, is this your kind of thing to look at bug wise? Regards and best wishes, Justin Clift
On Fri, 7 Jan 2011 03:16:56 +1100, Justin Clift <jclift@redhat.com> wrote:
On 07/01/2011, at 2:48 AM, Eric Blake wrote:
On 01/06/2011 08:29 AM, Justin Clift wrote:
Hi all,
Getting strange numbers from the virsh memtune command on RHEL 6. (git head compiled, rather than RHEL 6 packages)
virsh # list Id Name State ---------------------------------- 4 Fedora_14_x64 running
virsh # memtune Fedora_14_x64 hard_limit : 9007199254740991 soft_limit : 9007199254740991 swap_hard_limit: 9007199254740991
Those numbers are supposed to be kilobytes, but the host box itself only has 12GB ram.
Bug?
Smells like a case of mistakenly treating either -1 or INT64_MAX as a valid value (that integer is 0x1f_ffff_ffff_ffff, which happens to be UINT64_MAX>>11 or INT64_MAX>>10), rather than recognizing it as meaning unlimited or undeterminable. If unlimited is the intended meaning, virsh can probably be taught to display it differently, perhaps by also referencing the host's limits.
Yeah, that sounds like it might be the cause.
As Eric suggested, thats the value for unlimited, i.e. -1(64bit) and then (INT_MAX>>10) for getting kbytes. I will send a patch to display it as unlimited. Nikunj
From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Display unlimited when the memory cgroup settings says so. Unlimited is represented by INT64_MAX in memory cgroup. Signed-off-by: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Reported-by: Justin Clift <jclift@redhat.com> --- tools/virsh.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 55e2a68..bee875c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2987,9 +2987,15 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) params[i].value.l); break; case VIR_DOMAIN_MEMORY_PARAM_ULLONG: - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); + { + unsigned long long max_kbytes = INT64_MAX >> 10; + if (params[i].value.ul == max_kbytes) + vshPrint(ctl, "%-15s: unlimited\n", params[i].field); + else + vshPrint(ctl, "%-15s: %llu\n", params[i].field, + params[i].value.ul); break; + } case VIR_DOMAIN_MEMORY_PARAM_DOUBLE: vshPrint(ctl, "%-15s: %f\n", params[i].field, params[i].value.d);
On 01/07/2011 12:09 AM, Nikunj A. Dadhania wrote:
From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com>
Display unlimited when the memory cgroup settings says so. Unlimited is represented by INT64_MAX in memory cgroup.
Signed-off-by: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Reported-by: Justin Clift <jclift@redhat.com> --- tools/virsh.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 55e2a68..bee875c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2987,9 +2987,15 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) params[i].value.l); break; case VIR_DOMAIN_MEMORY_PARAM_ULLONG: - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); + { + unsigned long long max_kbytes = INT64_MAX >> 10;
Yuck - why do the clients have to know the magic value? Why not patch the source to actually return INT64_MAX rather than INT64_MAX>>10 when returning unlimited? Actually, due to the issue of cross-versioning between virsh and the actual libvirt running, we may have to check for both values. But even having a constant (VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) rather than making guests recompute things might be nice. But overall, I like the idea of this patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, 10 Jan 2011 12:02:36 -0700, Eric Blake <eblake@redhat.com> wrote:
On 01/07/2011 12:09 AM, Nikunj A. Dadhania wrote:
From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com>
Display unlimited when the memory cgroup settings says so. Unlimited is represented by INT64_MAX in memory cgroup.
Signed-off-by: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Reported-by: Justin Clift <jclift@redhat.com> --- tools/virsh.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 55e2a68..bee875c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2987,9 +2987,15 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) params[i].value.l); break; case VIR_DOMAIN_MEMORY_PARAM_ULLONG: - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); + { + unsigned long long max_kbytes = INT64_MAX >> 10;
Yuck - why do the clients have to know the magic value? Realised this but was late, and did not send a following patch :(
Why not patch the source to actually return INT64_MAX rather than INT64_MAX>>10 when returning unlimited?
Actually, due to the issue of cross-versioning between virsh and the actual libvirt running, we may have to check for both values. But even having a constant (VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) rather than making guests recompute things might be nice.
I like this option of having a constant and here is the patch. From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Display unlimited when the memory cgroup settings says so. Unlimited is represented by INT64_MAX in memory cgroup. Signed-off-by: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Reported-by: Justin Clift <jclift@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/util/cgroup.c | 27 +++++++++++++++++++++------ src/util/cgroup.h | 6 +++--- tools/virsh.c | 9 +++++++-- 6 files changed, 34 insertions(+), 13 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 3c6a54a..cb53f6b 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -696,6 +696,7 @@ typedef enum { */ #define VIR_DOMAIN_MEMORY_FIELD_LENGTH 80 +#define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED (UINT64_MAX) /** * VIR_DOMAIN_MEMORY_HARD_LIMIT: diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index eb58086..2db9954 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -815,7 +815,7 @@ static int lxcDomainGetMemoryParameters(virDomainPtr dom, int i; virCgroupPtr cgroup = NULL; virDomainObjPtr vm = NULL; - unsigned long val; + unsigned long long val; int ret = -1; int rc; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e915705..6648c6a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7077,7 +7077,7 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, int i; virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; - unsigned long val; + unsigned long long val; int ret = -1; int rc; diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 3ba6325..c81ebba 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -907,7 +907,7 @@ int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long long *kb) { long long unsigned int limit_in_bytes; int ret; @@ -915,7 +915,12 @@ int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + { + if (limit_in_bytes != INT64_MAX) + *kb = (unsigned long long) limit_in_bytes >> 10; + else + *kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + } return ret; } @@ -944,7 +949,7 @@ int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long long *kb) { long long unsigned int limit_in_bytes; int ret; @@ -952,7 +957,12 @@ int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.soft_limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + { + if (limit_in_bytes != INT64_MAX) + *kb = (unsigned long long) limit_in_bytes >> 10; + else + *kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + } return ret; } @@ -980,7 +990,7 @@ int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long long *kb) { long long unsigned int limit_in_bytes; int ret; @@ -988,7 +998,12 @@ int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.memsw.limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + { + if (limit_in_bytes != INT64_MAX) + *kb = (unsigned long long) limit_in_bytes >> 10; + else + *kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + } return ret; } diff --git a/src/util/cgroup.h b/src/util/cgroup.h index 9e1c61f..2769676 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -44,11 +44,11 @@ int virCgroupSetMemory(virCgroupPtr group, unsigned long kb); int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb); int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb); -int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb); +int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long long *kb); int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long kb); -int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb); +int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long long *kb); int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long kb); -int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long *kb); +int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long long *kb); int virCgroupDenyAllDevices(virCgroupPtr group); diff --git a/tools/virsh.c b/tools/virsh.c index 55e2a68..2fff9f4 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2987,9 +2987,14 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) params[i].value.l); break; case VIR_DOMAIN_MEMORY_PARAM_ULLONG: - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); + { + if (params[i].value.ul == VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + vshPrint(ctl, "%-15s: unlimited\n", params[i].field); + else + vshPrint(ctl, "%-15s: %llu\n", params[i].field, + params[i].value.ul); break; + } case VIR_DOMAIN_MEMORY_PARAM_DOUBLE: vshPrint(ctl, "%-15s: %f\n", params[i].field, params[i].value.d);
On 01/10/2011 10:18 PM, Nikunj A. Dadhania wrote:
Display unlimited when the memory cgroup settings says so. Unlimited is represented by INT64_MAX in memory cgroup.
Signed-off-by: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Reported-by: Justin Clift <jclift@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/util/cgroup.c | 27 +++++++++++++++++++++------ src/util/cgroup.h | 6 +++--- tools/virsh.c | 9 +++++++-- 6 files changed, 34 insertions(+), 13 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 3c6a54a..cb53f6b 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -696,6 +696,7 @@ typedef enum { */
#define VIR_DOMAIN_MEMORY_FIELD_LENGTH 80 +#define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED (UINT64_MAX)
() aren't strictly necessary here, but don't hurt, either.
@@ -907,7 +907,7 @@ int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long long *kb)
So why is Get changed to ull, but Set remains with just unsigned long? This patch is still incomplete. I think you need to touch both functions, and also virCgroupSetMemory. Also, in virCgroupSetMemory, you need to check for overflow, and fail if someone requests an impossible amount of kb.
{ long long unsigned int limit_in_bytes; int ret; @@ -915,7 +915,12 @@ int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + { + if (limit_in_bytes != INT64_MAX) + *kb = (unsigned long long) limit_in_bytes >> 10;
This cast is not necessary, since limit_in_bytes is already ull. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, 11 Jan 2011 08:19:30 -0700, Eric Blake <eblake@redhat.com> wrote:
On 01/10/2011 10:18 PM, Nikunj A. Dadhania wrote: [snip]
@@ -907,7 +907,7 @@ int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long long *kb)
So why is Get changed to ull, but Set remains with just unsigned long? This patch is still incomplete. I think you need to touch both functions, and also virCgroupSetMemory. Also, in virCgroupSetMemory, you need to check for overflow, and fail if someone requests an impossible amount of kb.
{ long long unsigned int limit_in_bytes; int ret; @@ -915,7 +915,12 @@ int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + { + if (limit_in_bytes != INT64_MAX) + *kb = (unsigned long long) limit_in_bytes >> 10;
This cast is not necessary, since limit_in_bytes is already ull.
Hi Eric, Thanks for reviewing. Here is the patch, now the set calls are also ull. Still virCgroupGetMemoryUsage is not changed, this will require changes in virDomainInfoPtr (info->memory). I am not sure if I should have them in this patch. From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Display unlimited when the memory cgroup settings says so. Unlimited is represented by INT64_MAX in memory cgroup. v3: Make virCgroupSet memory call ull Signed-off-by: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Reported-by: Justin Clift <jclift@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/util/cgroup.c | 45 +++++++++++++++++++++++++++++++++--------- src/util/cgroup.h | 14 +++++++------ tools/virsh.c | 9 +++++++- 6 files changed, 52 insertions(+), 21 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 3c6a54a..6475936 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -696,6 +696,7 @@ typedef enum { */ #define VIR_DOMAIN_MEMORY_FIELD_LENGTH 80 +#define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED UINT64_MAX /** * VIR_DOMAIN_MEMORY_HARD_LIMIT: diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index eb58086..2db9954 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -815,7 +815,7 @@ static int lxcDomainGetMemoryParameters(virDomainPtr dom, int i; virCgroupPtr cgroup = NULL; virDomainObjPtr vm = NULL; - unsigned long val; + unsigned long long val; int ret = -1; int rc; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e915705..6648c6a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7077,7 +7077,7 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, int i; virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; - unsigned long val; + unsigned long long val; int ret = -1; int rc; diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 3ba6325..f8991cf 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -858,8 +858,11 @@ int virCgroupForDomain(virCgroupPtr driver ATTRIBUTE_UNUSED, * * Returns: 0 on success */ -int virCgroupSetMemory(virCgroupPtr group, unsigned long kb) +int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb) { + if (kb > (VIR_DOMAIN_MEMORY_PARAM_UNLIMITED >> 10)) + return -EINVAL; + return virCgroupSetValueU64(group, VIR_CGROUP_CONTROLLER_MEMORY, "memory.limit_in_bytes", @@ -883,6 +886,7 @@ int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb) "memory.usage_in_bytes", &usage_in_bytes); if (ret == 0) *kb = (unsigned long) usage_in_bytes >> 10; + return ret; } @@ -894,7 +898,7 @@ int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb) * * Returns: 0 on success */ -int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb) +int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long long kb) { return virCgroupSetMemory(group, kb); } @@ -907,7 +911,7 @@ int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long long *kb) { long long unsigned int limit_in_bytes; int ret; @@ -915,7 +919,12 @@ int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + { + if (limit_in_bytes != INT64_MAX) + *kb = (unsigned long long) limit_in_bytes >> 10; + else + *kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + } return ret; } @@ -927,8 +936,11 @@ int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) * * Returns: 0 on success */ -int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long kb) +int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long long kb) { + if (kb > (VIR_DOMAIN_MEMORY_PARAM_UNLIMITED >> 10)) + return -EINVAL; + return virCgroupSetValueU64(group, VIR_CGROUP_CONTROLLER_MEMORY, "memory.soft_limit_in_bytes", @@ -944,7 +956,7 @@ int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long long *kb) { long long unsigned int limit_in_bytes; int ret; @@ -952,7 +964,12 @@ int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.soft_limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + { + if (limit_in_bytes != INT64_MAX) + *kb = (unsigned long long) limit_in_bytes >> 10; + else + *kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + } return ret; } @@ -964,8 +981,11 @@ int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb) * * Returns: 0 on success */ -int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long kb) +int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long long kb) { + if (kb > (VIR_DOMAIN_MEMORY_PARAM_UNLIMITED >> 10)) + return -EINVAL; + return virCgroupSetValueU64(group, VIR_CGROUP_CONTROLLER_MEMORY, "memory.memsw.limit_in_bytes", @@ -980,7 +1000,7 @@ int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long long *kb) { long long unsigned int limit_in_bytes; int ret; @@ -988,7 +1008,12 @@ int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.memsw.limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + { + if (limit_in_bytes != INT64_MAX) + *kb = (unsigned long long) limit_in_bytes >> 10; + else + *kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + } return ret; } diff --git a/src/util/cgroup.h b/src/util/cgroup.h index 9e1c61f..964da7a 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -40,15 +40,15 @@ int virCgroupForDomain(virCgroupPtr driver, int virCgroupAddTask(virCgroupPtr group, pid_t pid); -int virCgroupSetMemory(virCgroupPtr group, unsigned long kb); +int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb); int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb); -int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb); -int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb); -int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long kb); -int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb); -int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long kb); -int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long *kb); +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 virCgroupDenyAllDevices(virCgroupPtr group); diff --git a/tools/virsh.c b/tools/virsh.c index 55e2a68..2fff9f4 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2987,9 +2987,14 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) params[i].value.l); break; case VIR_DOMAIN_MEMORY_PARAM_ULLONG: - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); + { + if (params[i].value.ul == VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + vshPrint(ctl, "%-15s: unlimited\n", params[i].field); + else + vshPrint(ctl, "%-15s: %llu\n", params[i].field, + params[i].value.ul); break; + } case VIR_DOMAIN_MEMORY_PARAM_DOUBLE: vshPrint(ctl, "%-15s: %f\n", params[i].field, params[i].value.d);
On 01/12/2011 12:56 AM, Nikunj A. Dadhania wrote:
Here is the patch, now the set calls are also ull.
Still virCgroupGetMemoryUsage is not changed, this will require changes in virDomainInfoPtr (info->memory). I am not sure if I should have them in this patch.
It can be a separate patch, if desired, but it is probably still needed. virDomainGetInfo is inherently limited - we can't change that API due to backwards compatibility issues, so if we ever encounter a system with more than 4T memory (1k * 4G limit of unsigned long), then the maxMem and memory fields of virDomainInfoPtr have to be clamped at 4G on any system with 32-bit long, rather than wrapping around. We probably also ought to document this limitation, and point to getMemoryParameter as the way to find a more accurate memory limits if virDomainGetInfo returned clamped values.
From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com>
Display unlimited when the memory cgroup settings says so. Unlimited is represented by INT64_MAX in memory cgroup.
v3: Make virCgroupSet memory call ull
+++ b/src/util/cgroup.c @@ -858,8 +858,11 @@ int virCgroupForDomain(virCgroupPtr driver ATTRIBUTE_UNUSED, * * Returns: 0 on success */ -int virCgroupSetMemory(virCgroupPtr group, unsigned long kb) +int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb) { + if (kb > (VIR_DOMAIN_MEMORY_PARAM_UNLIMITED >> 10)) + return -EINVAL; + return virCgroupSetValueU64(group, VIR_CGROUP_CONTROLLER_MEMORY, "memory.limit_in_bytes",
Not quite right. What if I want to change from a finite limit back to unlimited? Then virCgroupSetMemory(group, UINT64_MAX) must recognize that kb == VIR_DOMAIN_MEMORY_PARAM_UNLIMITED, and call virCgroupSetValueU64(INT64_MAX) rathe than virCgroupSetValueU64(kb<<10).
@@ -883,6 +886,7 @@ int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb) "memory.usage_in_bytes", &usage_in_bytes); if (ret == 0) *kb = (unsigned long) usage_in_bytes >> 10; + return ret;
Why the whitespace change?
@@ -927,8 +936,11 @@ int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) * * Returns: 0 on success */ -int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long kb) +int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long long kb) { + if (kb > (VIR_DOMAIN_MEMORY_PARAM_UNLIMITED >> 10)) + return -EINVAL; +
Likewise on needing to allow the user to set the value back to unlimited.
+++ b/tools/virsh.c @@ -2987,9 +2987,14 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) params[i].value.l); break; case VIR_DOMAIN_MEMORY_PARAM_ULLONG: - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); + { + if (params[i].value.ul == VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + vshPrint(ctl, "%-15s: unlimited\n", params[i].field); + else + vshPrint(ctl, "%-15s: %llu\n", params[i].field, + params[i].value.ul);
Do we want any back-compat considerations? That is, if a newer virsh is talking to an older server, which still answered INT64_MAX>>10 instead of the new VIR_DOMAIN_MEMORY_PARAM_UNLIMITED, should we recognize that situation as another reason to print "unlimited"? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/1/12 Eric Blake <eblake@redhat.com>:
On 01/12/2011 12:56 AM, Nikunj A. Dadhania wrote:
Here is the patch, now the set calls are also ull.
+++ b/tools/virsh.c @@ -2987,9 +2987,14 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) params[i].value.l); break; case VIR_DOMAIN_MEMORY_PARAM_ULLONG: - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); + { + if (params[i].value.ul == VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + vshPrint(ctl, "%-15s: unlimited\n", params[i].field); + else + vshPrint(ctl, "%-15s: %llu\n", params[i].field, + params[i].value.ul);
Do we want any back-compat considerations? That is, if a newer virsh is talking to an older server, which still answered INT64_MAX>>10 instead of the new VIR_DOMAIN_MEMORY_PARAM_UNLIMITED, should we recognize that situation as another reason to print "unlimited"?
Why do we define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED to UINT64_MAX anyway when currently INT64_MAX >> 10 is supported to mean unlimited? Why do we want to complicate all applications that use the memory parameter API, because they effectively need to interpret two values as unlimited? Are there any really compelling reasons to change the value that means unlimited? Another question: How do I set a memory parameter back to unlimited? Do I need to specify INT64_MAX >> 10 as numeric value in virsh? Matthias
On Wed, 12 Jan 2011 10:21:04 -0700, Eric Blake <eblake@redhat.com> wrote:
On 01/12/2011 12:56 AM, Nikunj A. Dadhania wrote:
Here is the patch, now the set calls are also ull.
Still virCgroupGetMemoryUsage is not changed, this will require changes in virDomainInfoPtr (info->memory). I am not sure if I should have them in this patch.
It can be a separate patch, if desired, but it is probably still needed.
virCgroupGetMemoryUsage is called only from lxcDomainGetInfo, that is the reason I thought that this change may not be needed.
+++ b/tools/virsh.c @@ -2987,9 +2987,14 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) params[i].value.l); break; case VIR_DOMAIN_MEMORY_PARAM_ULLONG: - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); + { + if (params[i].value.ul == VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + vshPrint(ctl, "%-15s: unlimited\n", params[i].field); + else + vshPrint(ctl, "%-15s: %llu\n", params[i].field, + params[i].value.ul);
Do we want any back-compat considerations? That is, if a newer virsh is talking to an older server, which still answered INT64_MAX>>10 instead of the new VIR_DOMAIN_MEMORY_PARAM_UNLIMITED, should we recognize that situation as another reason to print "unlimited"?
As Mattias suggested in the other mail, this adds more complications. My take is to have VIR_DOMAIN_MEMORY_PARAM_UNLIMITED as the max value. Here is the patch which adds setting as well as displaying the "unlimited" values. Now in virsh to specify "unlimited" the user would need to pass "-1" for example: virsh # memtune lxcbb1 --hard-limit -1 From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Display and set unlimited when the memory cgroup settings. Unlimited is represented by INT64_MAX in memory cgroup. v4: Fix handling of setting unlimited values v3: Make virCgroupSet memory call ull Signed-off-by: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Reported-by: Justin Clift <jclift@redhat.com> --- include/libvirt/libvirt.h.in | 1 src/lxc/lxc_driver.c | 2 - src/qemu/qemu_driver.c | 2 - src/util/cgroup.c | 93 +++++++++++++++++++++++++++++++----------- src/util/cgroup.h | 14 +++--- tools/virsh.c | 13 +++++- 6 files changed, 90 insertions(+), 35 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 3c6a54a..3ee47b9 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -696,6 +696,7 @@ typedef enum { */ #define VIR_DOMAIN_MEMORY_FIELD_LENGTH 80 +#define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED INT64_MAX /** * VIR_DOMAIN_MEMORY_HARD_LIMIT: diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index eb58086..2db9954 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -815,7 +815,7 @@ static int lxcDomainGetMemoryParameters(virDomainPtr dom, int i; virCgroupPtr cgroup = NULL; virDomainObjPtr vm = NULL; - unsigned long val; + unsigned long long val; int ret = -1; int rc; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e915705..6648c6a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7077,7 +7077,7 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, int i; virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; - unsigned long val; + unsigned long long val; int ret = -1; int rc; diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 3ba6325..03a1263 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -355,8 +355,6 @@ static int virCgroupSetValueU64(virCgroupPtr group, } -#if 0 -/* This is included for completeness, but not yet used */ static int virCgroupSetValueI64(virCgroupPtr group, int controller, @@ -376,6 +374,8 @@ static int virCgroupSetValueI64(virCgroupPtr group, return rc; } +#if 0 +/* This is included for completeness, but not yet used */ static int virCgroupGetValueI64(virCgroupPtr group, int controller, const char *key, @@ -858,12 +858,22 @@ int virCgroupForDomain(virCgroupPtr driver ATTRIBUTE_UNUSED, * * Returns: 0 on success */ -int virCgroupSetMemory(virCgroupPtr group, unsigned long kb) +int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb) { - return virCgroupSetValueU64(group, - VIR_CGROUP_CONTROLLER_MEMORY, - "memory.limit_in_bytes", - kb << 10); + unsigned long long maxkb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED >> 10; + + if (kb > maxkb) + return -EINVAL; + else if (kb == maxkb) + return virCgroupSetValueI64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + "memory.limit_in_bytes", + -1); + else + return virCgroupSetValueU64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + "memory.limit_in_bytes", + kb << 10); } /** @@ -894,7 +904,7 @@ int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb) * * Returns: 0 on success */ -int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb) +int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long long kb) { return virCgroupSetMemory(group, kb); } @@ -907,7 +917,7 @@ int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long long *kb) { long long unsigned int limit_in_bytes; int ret; @@ -915,7 +925,12 @@ int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + { + if (limit_in_bytes != INT64_MAX) + *kb = (unsigned long long) limit_in_bytes >> 10; + else + *kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + } return ret; } @@ -927,12 +942,22 @@ int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) * * Returns: 0 on success */ -int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long kb) +int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long long kb) { - return virCgroupSetValueU64(group, - VIR_CGROUP_CONTROLLER_MEMORY, - "memory.soft_limit_in_bytes", - kb << 10); + unsigned long long maxkb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED >> 10; + + if (kb > maxkb) + return -EINVAL; + else if (kb == maxkb) + return virCgroupSetValueI64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + "memory.soft_limit_in_bytes", + -1); + else + return virCgroupSetValueU64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + "memory.soft_limit_in_bytes", + kb << 10); } @@ -944,7 +969,7 @@ int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long long *kb) { long long unsigned int limit_in_bytes; int ret; @@ -952,7 +977,12 @@ int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.soft_limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + { + if (limit_in_bytes != INT64_MAX) + *kb = (unsigned long long) limit_in_bytes >> 10; + else + *kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + } return ret; } @@ -964,12 +994,22 @@ int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb) * * Returns: 0 on success */ -int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long kb) +int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long long kb) { - return virCgroupSetValueU64(group, - VIR_CGROUP_CONTROLLER_MEMORY, - "memory.memsw.limit_in_bytes", - kb << 10); + unsigned long long maxkb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED >> 10; + + if (kb > maxkb) + return -EINVAL; + else if (kb == maxkb) + return virCgroupSetValueI64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + "memory.memsw.limit_in_bytes", + -1); + else + return virCgroupSetValueU64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + "memory.memsw.limit_in_bytes", + kb << 10); } /** @@ -980,7 +1020,7 @@ int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long long *kb) { long long unsigned int limit_in_bytes; int ret; @@ -988,7 +1028,12 @@ int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.memsw.limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + { + if (limit_in_bytes != INT64_MAX) + *kb = (unsigned long long) limit_in_bytes >> 10; + else + *kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + } return ret; } diff --git a/src/util/cgroup.h b/src/util/cgroup.h index 9e1c61f..964da7a 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -40,15 +40,15 @@ int virCgroupForDomain(virCgroupPtr driver, int virCgroupAddTask(virCgroupPtr group, pid_t pid); -int virCgroupSetMemory(virCgroupPtr group, unsigned long kb); +int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb); int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb); -int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb); -int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb); -int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long kb); -int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb); -int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long kb); -int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long *kb); +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 virCgroupDenyAllDevices(virCgroupPtr group); diff --git a/tools/virsh.c b/tools/virsh.c index 55e2a68..e855d1d 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2987,9 +2987,14 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) params[i].value.l); break; case VIR_DOMAIN_MEMORY_PARAM_ULLONG: - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); + { + if (params[i].value.ul == VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + vshPrint(ctl, "%-15s: unlimited\n", params[i].field); + else + vshPrint(ctl, "%-15s: %llu kB\n", params[i].field, + params[i].value.ul); break; + } case VIR_DOMAIN_MEMORY_PARAM_DOUBLE: vshPrint(ctl, "%-15s: %f\n", params[i].field, params[i].value.d); @@ -3039,6 +3044,10 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) sizeof(temp->field)); min_guarantee = 0; } + + /* If the user has passed -1, we interpret it as unlimited */ + if(temp->value.ul == -1) + temp->value.ul = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED >> 10; } if (virDomainSetMemoryParameters(dom, params, nparams, 0) != 0) vshError(ctl, "%s", _("Unable to change memory parameters"));
2011/1/13 Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com>:
On Wed, 12 Jan 2011 10:21:04 -0700, Eric Blake <eblake@redhat.com> wrote:
On 01/12/2011 12:56 AM, Nikunj A. Dadhania wrote:
Here is the patch, now the set calls are also ull.
Still virCgroupGetMemoryUsage is not changed, this will require changes in virDomainInfoPtr (info->memory). I am not sure if I should have them in this patch.
It can be a separate patch, if desired, but it is probably still needed.
virCgroupGetMemoryUsage is called only from lxcDomainGetInfo, that is the reason I thought that this change may not be needed.
+++ b/tools/virsh.c @@ -2987,9 +2987,14 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) params[i].value.l); break; case VIR_DOMAIN_MEMORY_PARAM_ULLONG: - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); + { + if (params[i].value.ul == VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + vshPrint(ctl, "%-15s: unlimited\n", params[i].field); + else + vshPrint(ctl, "%-15s: %llu\n", params[i].field, + params[i].value.ul);
Do we want any back-compat considerations? That is, if a newer virsh is talking to an older server, which still answered INT64_MAX>>10 instead of the new VIR_DOMAIN_MEMORY_PARAM_UNLIMITED, should we recognize that situation as another reason to print "unlimited"?
As Mattias suggested in the other mail, this adds more complications. My take is to have VIR_DOMAIN_MEMORY_PARAM_UNLIMITED as the max value.
Here is the patch which adds setting as well as displaying the "unlimited" values. Now in virsh to specify "unlimited" the user would need to pass "-1"
for example: virsh # memtune lxcbb1 --hard-limit -1
From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com>
Display and set unlimited when the memory cgroup settings. Unlimited is represented by INT64_MAX in memory cgroup.
v4: Fix handling of setting unlimited values v3: Make virCgroupSet memory call ull
Signed-off-by: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Reported-by: Justin Clift <jclift@redhat.com> --- include/libvirt/libvirt.h.in | 1 src/lxc/lxc_driver.c | 2 - src/qemu/qemu_driver.c | 2 - src/util/cgroup.c | 93 +++++++++++++++++++++++++++++++----------- src/util/cgroup.h | 14 +++--- tools/virsh.c | 13 +++++- 6 files changed, 90 insertions(+), 35 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 3c6a54a..3ee47b9 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -696,6 +696,7 @@ typedef enum { */
#define VIR_DOMAIN_MEMORY_FIELD_LENGTH 80 +#define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED INT64_MAX
If I understand it correctly we currently use INT64_MAX>>10 to indicate "unlimited". Therefore, we should define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED to INT64_MAX>>10 as this would preserve the existing behavior. I think this patch started to fix the problem that virsh memtune reports 9007199254740991 (== INT64_MAX>>10) when the value actually means unlimited. We want virsh to report "unlimited" in this case. As we cannot fix the problem in a better way (like using -1 for unlimited) anymore, we try to workaround it: First add a define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED for the magic value INT64_MAX>>10, but stick to this value, because changing it could break existing applications. Second make virsh memtune detect VIR_DOMAIN_MEMORY_PARAM_UNLIMITED and print "unlimited" in that case instead of the actual numeric value. Third make virsh memtune accept -1 as unlimited and translate it to VIR_DOMAIN_MEMORY_PARAM_UNLIMITED. You already addressed the second and third point in your patch so we're close to a proper workaround. Matthias
On Thu, 13 Jan 2011 09:24:09 +0100, Matthias Bolte <matthias.bolte@googlemail.com> wrote:
2011/1/13 Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com>: [snip]
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 3c6a54a..3ee47b9 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -696,6 +696,7 @@ typedef enum { */
#define VIR_DOMAIN_MEMORY_FIELD_LENGTH 80 +#define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED INT64_MAX [snip]
First add a define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED for the magic value INT64_MAX>>10, but stick to this value, because changing it could break existing applications.
Second make virsh memtune detect VIR_DOMAIN_MEMORY_PARAM_UNLIMITED and print "unlimited" in that case instead of the actual numeric value.
Third make virsh memtune accept -1 as unlimited and translate it to VIR_DOMAIN_MEMORY_PARAM_UNLIMITED.
You already addressed the second and third point in your patch so we're close to a proper workaround.
Here is one more spin and guess we would be finally there :) From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Display or set unlimited values for memory paramters. Unlimited is represented by INT64_MAX in memory cgroup. v5: return back to max as (INT64_MAX >> 10) for backward portablity. v4: Fix handling of setting unlimited values v3: Make virCgroupSet memory call ull Signed-off-by: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Reported-by: Justin Clift <jclift@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/lxc/lxc_driver.c | 2 + src/qemu/qemu_driver.c | 2 + src/util/cgroup.c | 78 +++++++++++++++++++++++++++++------------- src/util/cgroup.h | 14 ++++---- tools/virsh.c | 13 ++++++- 6 files changed, 75 insertions(+), 35 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 3c6a54a..055eb2e 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -696,6 +696,7 @@ typedef enum { */ #define VIR_DOMAIN_MEMORY_FIELD_LENGTH 80 +#define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED (INT64_MAX >> 10) /** * VIR_DOMAIN_MEMORY_HARD_LIMIT: diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index eb58086..2db9954 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -815,7 +815,7 @@ static int lxcDomainGetMemoryParameters(virDomainPtr dom, int i; virCgroupPtr cgroup = NULL; virDomainObjPtr vm = NULL; - unsigned long val; + unsigned long long val; int ret = -1; int rc; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e915705..6648c6a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7077,7 +7077,7 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, int i; virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; - unsigned long val; + unsigned long long val; int ret = -1; int rc; diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 3ba6325..cd9caba 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -355,8 +355,6 @@ static int virCgroupSetValueU64(virCgroupPtr group, } -#if 0 -/* This is included for completeness, but not yet used */ static int virCgroupSetValueI64(virCgroupPtr group, int controller, @@ -376,6 +374,8 @@ static int virCgroupSetValueI64(virCgroupPtr group, return rc; } +#if 0 +/* This is included for completeness, but not yet used */ static int virCgroupGetValueI64(virCgroupPtr group, int controller, const char *key, @@ -858,12 +858,22 @@ int virCgroupForDomain(virCgroupPtr driver ATTRIBUTE_UNUSED, * * Returns: 0 on success */ -int virCgroupSetMemory(virCgroupPtr group, unsigned long kb) +int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb) { - return virCgroupSetValueU64(group, - VIR_CGROUP_CONTROLLER_MEMORY, - "memory.limit_in_bytes", - kb << 10); + unsigned long long maxkb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + + if (kb > maxkb) + return -EINVAL; + else if (kb == maxkb) + return virCgroupSetValueI64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + "memory.limit_in_bytes", + -1); + else + return virCgroupSetValueU64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + "memory.limit_in_bytes", + kb << 10); } /** @@ -894,7 +904,7 @@ int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb) * * Returns: 0 on success */ -int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb) +int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long long kb) { return virCgroupSetMemory(group, kb); } @@ -907,7 +917,7 @@ int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long long *kb) { long long unsigned int limit_in_bytes; int ret; @@ -915,7 +925,7 @@ int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + *kb = limit_in_bytes >> 10; return ret; } @@ -927,12 +937,22 @@ int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb) * * Returns: 0 on success */ -int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long kb) +int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long long kb) { - return virCgroupSetValueU64(group, - VIR_CGROUP_CONTROLLER_MEMORY, - "memory.soft_limit_in_bytes", - kb << 10); + unsigned long long maxkb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + + if (kb > maxkb) + return -EINVAL; + else if (kb == maxkb) + return virCgroupSetValueI64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + "memory.soft_limit_in_bytes", + -1); + else + return virCgroupSetValueU64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + "memory.soft_limit_in_bytes", + kb << 10); } @@ -944,7 +964,7 @@ int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long long *kb) { long long unsigned int limit_in_bytes; int ret; @@ -952,7 +972,7 @@ int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.soft_limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + *kb = limit_in_bytes >> 10; return ret; } @@ -964,12 +984,22 @@ int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb) * * Returns: 0 on success */ -int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long kb) +int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long long kb) { - return virCgroupSetValueU64(group, - VIR_CGROUP_CONTROLLER_MEMORY, - "memory.memsw.limit_in_bytes", - kb << 10); + unsigned long long maxkb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + + if (kb > maxkb) + return -EINVAL; + else if (kb == maxkb) + return virCgroupSetValueI64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + "memory.memsw.limit_in_bytes", + -1); + else + return virCgroupSetValueU64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + "memory.memsw.limit_in_bytes", + kb << 10); } /** @@ -980,7 +1010,7 @@ int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long kb) * * Returns: 0 on success */ -int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long *kb) +int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long long *kb) { long long unsigned int limit_in_bytes; int ret; @@ -988,7 +1018,7 @@ int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long *kb) VIR_CGROUP_CONTROLLER_MEMORY, "memory.memsw.limit_in_bytes", &limit_in_bytes); if (ret == 0) - *kb = (unsigned long) limit_in_bytes >> 10; + *kb = limit_in_bytes >> 10; return ret; } diff --git a/src/util/cgroup.h b/src/util/cgroup.h index 9e1c61f..964da7a 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -40,15 +40,15 @@ int virCgroupForDomain(virCgroupPtr driver, int virCgroupAddTask(virCgroupPtr group, pid_t pid); -int virCgroupSetMemory(virCgroupPtr group, unsigned long kb); +int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb); int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb); -int virCgroupSetMemoryHardLimit(virCgroupPtr group, unsigned long kb); -int virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long *kb); -int virCgroupSetMemorySoftLimit(virCgroupPtr group, unsigned long kb); -int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long *kb); -int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long kb); -int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long *kb); +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 virCgroupDenyAllDevices(virCgroupPtr group); diff --git a/tools/virsh.c b/tools/virsh.c index 55e2a68..f4a0aea 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2987,9 +2987,14 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) params[i].value.l); break; case VIR_DOMAIN_MEMORY_PARAM_ULLONG: - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); + { + if (params[i].value.ul == VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + vshPrint(ctl, "%-15s: unlimited\n", params[i].field); + else + vshPrint(ctl, "%-15s: %llu kB\n", params[i].field, + params[i].value.ul); break; + } case VIR_DOMAIN_MEMORY_PARAM_DOUBLE: vshPrint(ctl, "%-15s: %f\n", params[i].field, params[i].value.d); @@ -3039,6 +3044,10 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) sizeof(temp->field)); min_guarantee = 0; } + + /* If the user has passed -1, we interpret it as unlimited */ + if(temp->value.ul == -1) + temp->value.ul = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; } if (virDomainSetMemoryParameters(dom, params, nparams, 0) != 0) vshError(ctl, "%s", _("Unable to change memory parameters"));
On 01/13/2011 02:18 AM, Nikunj A. Dadhania wrote:
Here is one more spin and guess we would be finally there :)
Here's hoping.
From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com>
Display or set unlimited values for memory paramters. Unlimited is
s/paramters/parameters/
represented by INT64_MAX in memory cgroup.
v5: return back to max as (INT64_MAX >> 10) for backward portablity.
s/portablity/portability/
+++ b/tools/virsh.c @@ -2987,9 +2987,14 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) params[i].value.l); break; case VIR_DOMAIN_MEMORY_PARAM_ULLONG: - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); + {
Not sure what the extra braces are for...
+ if (params[i].value.ul == VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + vshPrint(ctl, "%-15s: unlimited\n", params[i].field); + else + vshPrint(ctl, "%-15s: %llu kB\n", params[i].field, + params[i].value.ul); break; + }
since you didn't declare anything. So I nuked them.
case VIR_DOMAIN_MEMORY_PARAM_DOUBLE: vshPrint(ctl, "%-15s: %f\n", params[i].field, params[i].value.d); @@ -3039,6 +3044,10 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) sizeof(temp->field)); min_guarantee = 0; } + + /* If the user has passed -1, we interpret it as unlimited */ + if(temp->value.ul == -1)
space after if (keywords are different than function calls on preferred spacing). ACK with those nits fixed, so I pushed it. Thanks again for tackling this. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/01/2011, at 6:09 PM, Nikunj A. Dadhania wrote:
From: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com>
Display unlimited when the memory cgroup settings says so. Unlimited is represented by INT64_MAX in memory cgroup.
Signed-off-by: Nikunj A. Dadhania <nikunj@linux.vnet.ibm.com> Reported-by: Justin Clift <jclift@redhat.com>
As an aside, just created a Bugzilla ticket for this, so we can track it: https://bugzilla.redhat.com/show_bug.cgi?id=669069 I think this should be incorporated into our next release of RHEL if possible (when finally ACK'd). Regards and best wishes, Justin Clift
participants (4)
-
Eric Blake -
Justin Clift -
Matthias Bolte -
Nikunj A. Dadhania