Re: [libvirt] [RFC PATCH] cgroup: Use system reported "unlimited" value for comparison

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&gt; >> --- 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.