11:03 a.m.
With kernel 3.18 (since commit 3e32cb2e0a12b6915056ff04601cf1bb9b44f967)
the "unlimited" value for cgroup memory limits has changed once again as
its byte value is now computed from a page counter.
The new "unlimited" value reported by the cgroup fs is therefore 2**51-1
pages which is (VIR_DOMAIN_MEMORY_PARAM_UNLIMITED - 3072). This results
e.g. in virsh memtune displaying 9007199254740988 instead of unlimited
for the limits.
This patch uses the value of memory.limit_in_bytes from the cgroup
memory root which is the system's "real" unlimited value for comparison.
See also libvirt commit 231656bbeb9e4d3bedc44362784c35eee21cf0f4 for the
history for kernel 3.12 and before.
I've tested this on F24 with the following configurations:
- no memory cgroup controller mounted
- memory cgroup controller mounted but not configured for libvirt
- memory cgroup controller mounted and configured
The first two fail as expected (and as before), the third case
works as expected.
Testing on other kernel versions highly welcome!
Not perfect yet in that we still provide a fallback to the old value.
We might consider failing right away if we can't get the system
value. I'd be inclined to do that, since we're probably facing
principal cgroup issues in this case.
Further, it's not the most efficient implementation. Obviously, the
unlimited value can be read once and cached. However, I'd like to see
the question above resolved first.
Signed-off-by: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com>
---
src/util/vircgroup.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 55 insertions(+), 6 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index f151193..969dca5 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -2452,6 +2452,40 @@ virCgroupGetBlkioDeviceWeight(virCgroupPtr group,
}
+/*
+ * Retrieve the "memory.limit_in_bytes" value from the memory controller
+ * root dir. This value cannot be modified by userspace and therefore
+ * is the maximum limit value supported by cgroups on the local system.
+ */
+static int
+virCgroupGetMemoryUnlimited(unsigned long long int * mem_unlimited)
+{
+ int ret = -1;
+ virCgroupPtr group;
+
+ if (VIR_ALLOC(group))
+ goto cleanup;
+
+ if (virCgroupDetectMounts(group))
+ goto cleanup;
+
+ if (!group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].mountPoint)
+ goto cleanup;
+
+ if (VIR_STRDUP(group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].placement,
+ "/.") < 0)
+ goto cleanup;
+
+ ret = virCgroupGetValueU64(group,
+ VIR_CGROUP_CONTROLLER_MEMORY,
+ "memory.limit_in_bytes",
+ mem_unlimited);
+ cleanup:
+ virCgroupFree(&group);
+ return ret;
+}
+
+
/**
* virCgroupSetMemory:
*
@@ -2534,6 +2568,7 @@ int
virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long long *kb)
{
long long unsigned int limit_in_bytes;
+ long long unsigned int unlimited_in_bytes;
int ret = -1;
if (virCgroupGetValueU64(group,
@@ -2541,9 +2576,13 @@ virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long long
*kb)
"memory.limit_in_bytes", &limit_in_bytes) <
0)
goto cleanup;
- *kb = limit_in_bytes >> 10;
- if (*kb > VIR_DOMAIN_MEMORY_PARAM_UNLIMITED)
+ if (virCgroupGetMemoryUnlimited(&unlimited_in_bytes) < 0)
+ unlimited_in_bytes = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED << 10;
+
+ if (limit_in_bytes == unlimited_in_bytes)
*kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
+ else
+ *kb = limit_in_bytes >> 10;
ret = 0;
cleanup:
@@ -2596,6 +2635,7 @@ int
virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long long *kb)
{
long long unsigned int limit_in_bytes;
+ long long unsigned int unlimited_in_bytes;
int ret = -1;
if (virCgroupGetValueU64(group,
@@ -2603,9 +2643,13 @@ virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long long
*kb)
"memory.soft_limit_in_bytes", &limit_in_bytes)
< 0)
goto cleanup;
- *kb = limit_in_bytes >> 10;
- if (*kb > VIR_DOMAIN_MEMORY_PARAM_UNLIMITED)
+ if (virCgroupGetMemoryUnlimited(&unlimited_in_bytes) < 0)
+ unlimited_in_bytes = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED << 10;
+
+ if (limit_in_bytes == unlimited_in_bytes)
*kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
+ else
+ *kb = limit_in_bytes >> 10;
ret = 0;
cleanup:
@@ -2658,6 +2702,7 @@ int
virCgroupGetMemSwapHardLimit(virCgroupPtr group, unsigned long long *kb)
{
long long unsigned int limit_in_bytes;
+ long long unsigned int unlimited_in_bytes;
int ret = -1;
if (virCgroupGetValueU64(group,
@@ -2665,9 +2710,13 @@ virCgroupGetMemSwapHardLimit(virCgroupPtr group, unsigned long long
*kb)
"memory.memsw.limit_in_bytes",
&limit_in_bytes) < 0)
goto cleanup;
- *kb = limit_in_bytes >> 10;
- if (*kb > VIR_DOMAIN_MEMORY_PARAM_UNLIMITED)
+ if (virCgroupGetMemoryUnlimited(&unlimited_in_bytes) < 0)
+ unlimited_in_bytes = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED << 10;
+
+ if (limit_in_bytes == unlimited_in_bytes)
*kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
+ else
+ *kb = limit_in_bytes >> 10;
ret = 0;
cleanup:
--
1.9.1
6:20 a.m.
On Mon, Nov 28, 2016 at 06:03:36PM +0100, Viktor Mihajlovski wrote:
With kernel 3.18 (since commit
3e32cb2e0a12b6915056ff04601cf1bb9b44f967)
the "unlimited" value for cgroup memory limits has changed once again as
its byte value is now computed from a page counter.
The new "unlimited" value reported by the cgroup fs is therefore 2**51-1
pages which is (VIR_DOMAIN_MEMORY_PARAM_UNLIMITED - 3072). This results
e.g. in virsh memtune displaying 9007199254740988 instead of unlimited
for the limits.
This patch uses the value of memory.limit_in_bytes from the cgroup
memory root which is the system's "real" unlimited value for comparison.
See also libvirt commit 231656bbeb9e4d3bedc44362784c35eee21cf0f4 for the
history for kernel 3.12 and before.
I've tested this on F24 with the following configurations:
- no memory cgroup controller mounted
- memory cgroup controller mounted but not configured for libvirt
- memory cgroup controller mounted and configured
The first two fail as expected (and as before), the third case
works as expected.
Testing on other kernel versions highly welcome!
Not perfect yet in that we still provide a fallback to the old value.
We might consider failing right away if we can't get the system
value. I'd be inclined to do that, since we're probably facing
principal cgroup issues in this case.
Since the code is called only after reading another value worked, it
_should not_ fail =) But I'm OK with both failing and falling back to
the old value. Mostly because I don't think it will make any
(significant) difference.
Further, it's not the most efficient implementation. Obviously,
the
unlimited value can be read once and cached. However, I'd like to see
the question above resolved first.
But I would really like to cache the value in a global variable. You
can use VIR_ONCE_GLOBAL_INIT for that, but maybe it's too much,
especially if you init the value before any other thread could access
it.
Signed-off-by: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com>
---
src/util/vircgroup.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 55 insertions(+), 6 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index f151193..969dca5 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -2452,6 +2452,40 @@ virCgroupGetBlkioDeviceWeight(virCgroupPtr group,
}
+/*
+ * Retrieve the "memory.limit_in_bytes" value from the memory controller
+ * root dir. This value cannot be modified by userspace and therefore
+ * is the maximum limit value supported by cgroups on the local system.
+ */
+static int
+virCgroupGetMemoryUnlimited(unsigned long long int * mem_unlimited)
+{
+ int ret = -1;
+ virCgroupPtr group;
+
Also, all this ↓
+ if (VIR_ALLOC(group))
+ goto cleanup;
+
+ if (virCgroupDetectMounts(group))
+ goto cleanup;
+
+ if (!group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].mountPoint)
+ goto cleanup;
+
+ if (VIR_STRDUP(group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].placement,
+ "/.") < 0)
+ goto cleanup;
+
↑ would be cleaner this way:
if (virCgroupNew(-1, "/", NULL, NULL, &group) < 0)
return -1;
if (!virCgroupHasController(cgroup, VIR_CGROUP_CONTROLLER_MEMORY))
goto cleanup;
I'm not passing VIR_CGROUP_CONTROLLER_MEMORY to virCgroupNew() so that
it doesn't fail with a cryptic message.
+ ret = virCgroupGetValueU64(group,
+ VIR_CGROUP_CONTROLLER_MEMORY,
+ "memory.limit_in_bytes",
+ mem_unlimited);
+ cleanup:
+ virCgroupFree(&group);
+ return ret;
+}
+
+
/**
* virCgroupSetMemory:
*
@@ -2534,6 +2568,7 @@ int
virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long long *kb)
{
long long unsigned int limit_in_bytes;
+ long long unsigned int unlimited_in_bytes;
int ret = -1;
if (virCgroupGetValueU64(group,
@@ -2541,9 +2576,13 @@ virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long long
*kb)
"memory.limit_in_bytes", &limit_in_bytes) <
0)
goto cleanup;
- *kb = limit_in_bytes >> 10;
- if (*kb > VIR_DOMAIN_MEMORY_PARAM_UNLIMITED)
+ if (virCgroupGetMemoryUnlimited(&unlimited_in_bytes) < 0)
+ unlimited_in_bytes = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED << 10;
+
+ if (limit_in_bytes == unlimited_in_bytes)
I don't know why, but I would feel more comfortable with '>=' there,
although it doesn't make any difference (or sense).
*kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
+ else
+ *kb = limit_in_bytes >> 10;
ret = 0;
cleanup:
As a nit, helper function for these would be nice.
Otherwise, I like it.
7:22 a.m.
On 30.11.2016 13:20, Martin Kletzander wrote:
On Mon, Nov 28, 2016 at 06:03:36PM +0100, Viktor Mihajlovski
wrote:
> With kernel 3.18 (since commit
> 3e32cb2e0a12b6915056ff04601cf1bb9b44f967) the "unlimited" value
> for cgroup memory limits has changed once again as its byte
> value is now computed from a page counter. The new "unlimited"
> value reported by the cgroup fs is therefore 2**51-1 pages which
> is (VIR_DOMAIN_MEMORY_PARAM_UNLIMITED - 3072). This results e.g.
> in virsh memtune displaying 9007199254740988 instead of unlimited
> for the limits.
>
> This patch uses the value of memory.limit_in_bytes from the
> cgroup memory root which is the system's "real" unlimited value
> for comparison.
>
> See also libvirt commit 231656bbeb9e4d3bedc44362784c35eee21cf0f4
> for the history for kernel 3.12 and before.
>
> I've tested this on F24 with the following configurations: - no
> memory cgroup controller mounted - memory cgroup controller
> mounted but not configured for libvirt - memory cgroup
> controller mounted and configured The first two fail as expected
> (and as before), the third case works as expected.
>
> Testing on other kernel versions highly welcome!
>
> Not perfect yet in that we still provide a fallback to the old
> value. We might consider failing right away if we can't get the
> system value. I'd be inclined to do that, since we're probably
> facing principal cgroup issues in this case.
>
Since the code is called only after reading another value worked,
it _should not_ fail =) But I'm OK with both failing and falling
back to the old value. Mostly because I don't think it will make
any (significant) difference.
OK, this tips me towards the no fallback.
> Further, it's not the most efficient implementation.
Obviously,
> the unlimited value can be read once and cached. However, I'd
> like to see the question above resolved first.
>
But I would really like to cache the value in a global variable.
You can use VIR_ONCE_GLOBAL_INIT for that, but maybe it's too much,
especially if you init the value before any other thread could
access it.
Sure, even if the initialization was racy, I see no way the global
long long value could be corrupted.
> Signed-off-by: Viktor Mihajlovski
<mihajlov(a)linux.vnet.ibm.com>
> --- src/util/vircgroup.c | 61
> ++++++++++++++++++++++++++++++++++++++++++++++------ 1 file
> changed, 55 insertions(+), 6 deletions(-)
>
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index
> f151193..969dca5 100644 --- a/src/util/vircgroup.c +++
> b/src/util/vircgroup.c @@ -2452,6 +2452,40 @@
> virCgroupGetBlkioDeviceWeight(virCgroupPtr group, }
>
>
> +/* + * Retrieve the "memory.limit_in_bytes" value from the
> memory controller + * root dir. This value cannot be modified by
> userspace and therefore + * is the maximum limit value supported
> by cgroups on the local system. + */ +static int
> +virCgroupGetMemoryUnlimited(unsigned long long int *
> mem_unlimited) +{ + int ret = -1; + virCgroupPtr group; +
Also, all this ↓
> + if (VIR_ALLOC(group)) + goto cleanup; + + if
> (virCgroupDetectMounts(group)) + goto cleanup; + + if
> (!group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].mountPoint) +
> goto cleanup; + + if
> (VIR_STRDUP(group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].placement,
>
>
>
+ "/.") < 0)
> + goto cleanup; +
↑ would be cleaner this way:
if (virCgroupNew(-1, "/", NULL, NULL, &group) < 0) return -1;
if (!virCgroupHasController(cgroup, VIR_CGROUP_CONTROLLER_MEMORY))
goto cleanup;
I'm not passing VIR_CGROUP_CONTROLLER_MEMORY to virCgroupNew() so
that it doesn't fail with a cryptic message.
Looks cleaner indeed ... I'll give it a try.
> + ret = virCgroupGetValueU64(group, +
> VIR_CGROUP_CONTROLLER_MEMORY, + "memory.limit_in_bytes", +
> mem_unlimited); + cleanup: + virCgroupFree(&group); + return
> ret; +} + + /** * virCgroupSetMemory: * @@ -2534,6 +2568,7 @@ int
> virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long
> long *kb) { long long unsigned int limit_in_bytes; + long long
> unsigned int unlimited_in_bytes; int ret = -1;
>
> if (virCgroupGetValueU64(group, @@ -2541,9 +2576,13 @@
> virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long
> long *kb) "memory.limit_in_bytes", &limit_in_bytes) < 0) goto
> cleanup;
>
> - *kb = limit_in_bytes >> 10; - if (*kb >
> VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + if
> (virCgroupGetMemoryUnlimited(&unlimited_in_bytes) < 0) +
> unlimited_in_bytes = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED << 10; +
> + if (limit_in_bytes == unlimited_in_bytes)
I don't know why, but I would feel more comfortable with '>='
there, although it doesn't make any difference (or sense).
> *kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + else + *kb
> = limit_in_bytes >> 10;
>
> ret = 0; cleanup:
As a nit, helper function for these would be nice.
Otherwise, I like it.
Thanks for the feedback.
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
6:11 a.m.
On 30.11.2016 14:22, Viktor Mihajlovski wrote:
On 30.11.2016 13:20, Martin Kletzander wrote:
> On Mon, Nov 28, 2016 at 06:03:36PM +0100, Viktor Mihajlovski
> wrote:
>> With kernel 3.18 (since commit
>> 3e32cb2e0a12b6915056ff04601cf1bb9b44f967) the "unlimited" value
>> for cgroup memory limits has changed once again as its byte
>> value is now computed from a page counter. The new "unlimited"
>> value reported by the cgroup fs is therefore 2**51-1 pages which
>> is (VIR_DOMAIN_MEMORY_PARAM_UNLIMITED - 3072). This results e.g.
>> in virsh memtune displaying 9007199254740988 instead of unlimited
>> for the limits.
>>
>> This patch uses the value of memory.limit_in_bytes from the
>> cgroup memory root which is the system's "real" unlimited value
>> for comparison.
>>
>> See also libvirt commit 231656bbeb9e4d3bedc44362784c35eee21cf0f4
>> for the history for kernel 3.12 and before.
>>
>> I've tested this on F24 with the following configurations: - no
>> memory cgroup controller mounted - memory cgroup controller
>> mounted but not configured for libvirt - memory cgroup
>> controller mounted and configured The first two fail as expected
>> (and as before), the third case works as expected.
>>
>> Testing on other kernel versions highly welcome!
>>
>> Not perfect yet in that we still provide a fallback to the old
>> value. We might consider failing right away if we can't get the
>> system value. I'd be inclined to do that, since we're probably
>> facing principal cgroup issues in this case.
>>
>
> Since the code is called only after reading another value worked,
> it _should not_ fail =) But I'm OK with both failing and falling
> back to the old value. Mostly because I don't think it will make
> any (significant) difference.
>
OK, this tips me towards the no fallback.
>> Further, it's not the most efficient implementation. Obviously,
>> the unlimited value can be read once and cached. However, I'd
>> like to see the question above resolved first.
>>
>
> But I would really like to cache the value in a global variable.
> You can use VIR_ONCE_GLOBAL_INIT for that, but maybe it's too much,
> especially if you init the value before any other thread could
> access it.
>
Sure, even if the initialization was racy, I see no way the global
long long value could be corrupted.
>> Signed-off-by: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com>
>> --- src/util/vircgroup.c | 61
>> ++++++++++++++++++++++++++++++++++++++++++++++------ 1 file
>> changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index
>> f151193..969dca5 100644 --- a/src/util/vircgroup.c +++
>> b/src/util/vircgroup.c @@ -2452,6 +2452,40 @@
>> virCgroupGetBlkioDeviceWeight(virCgroupPtr group, }
>>
>>
>> +/* + * Retrieve the "memory.limit_in_bytes" value from the
>> memory controller + * root dir. This value cannot be modified by
>> userspace and therefore + * is the maximum limit value supported
>> by cgroups on the local system. + */ +static int
>> +virCgroupGetMemoryUnlimited(unsigned long long int *
>> mem_unlimited) +{ + int ret = -1; + virCgroupPtr group; +
>
> Also, all this ↓
>
>> + if (VIR_ALLOC(group)) + goto cleanup; + + if
>> (virCgroupDetectMounts(group)) + goto cleanup; + + if
>> (!group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].mountPoint) +
>> goto cleanup; + + if
>> (VIR_STRDUP(group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].placement,
>>
>>
>>
+ "/.") < 0)
>> + goto cleanup; +
>
> ↑ would be cleaner this way:
>
> if (virCgroupNew(-1, "/", NULL, NULL, &group) < 0) return -1;
>
> if (!virCgroupHasController(cgroup, VIR_CGROUP_CONTROLLER_MEMORY))
> goto cleanup;
>
> I'm not passing VIR_CGROUP_CONTROLLER_MEMORY to virCgroupNew() so
> that it doesn't fail with a cryptic message.
>
Looks cleaner indeed ... I'll give it a try.
>> + ret = virCgroupGetValueU64(group, +
>> VIR_CGROUP_CONTROLLER_MEMORY, + "memory.limit_in_bytes", +
>> mem_unlimited); + cleanup: + virCgroupFree(&group); + return
>> ret; +} + + /** * virCgroupSetMemory: * @@ -2534,6 +2568,7 @@ int
>> virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long
>> long *kb) { long long unsigned int limit_in_bytes; + long long
>> unsigned int unlimited_in_bytes; int ret = -1;
>>
>> if (virCgroupGetValueU64(group, @@ -2541,9 +2576,13 @@
>> virCgroupGetMemoryHardLimit(virCgroupPtr group, unsigned long
>> long *kb) "memory.limit_in_bytes", &limit_in_bytes) < 0) goto
>> cleanup;
>>
>> - *kb = limit_in_bytes >> 10; - if (*kb >
>> VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) + if
>> (virCgroupGetMemoryUnlimited(&unlimited_in_bytes) < 0) +
>> unlimited_in_bytes = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED << 10; +
>> + if (limit_in_bytes == unlimited_in_bytes)
>
> I don't know why, but I would feel more comfortable with '>='
> there, although it doesn't make any difference (or sense).
>
>> *kb = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + else + *kb
>> = limit_in_bytes >> 10;
>>
>> ret = 0; cleanup:
>
> As a nit, helper function for these would be nice.
>
> Otherwise, I like it.
Thanks for the feedback.
FYI, I've posted new version that uses thread-safe initialization
https://www.redhat.com/archives/libvir-list/2016-December/msg00176.html
According to some documents I've read it seems that on ARM 64-bit memory
access may be interrupted, so I didn't want to take a chance.
The checking code is not really performance critical, as we have to walk
the cgroup filesyste anyway in order to get the process' limits.
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
2908
days inactive
2916
days old
3 comments
2 participants
participants (2)
-
Martin Kletzander -
Viktor Mihajlovski