[libvirt] [PATCH 0/4] lxc: fix 'free' usage on fedora 23
We need to add a few bits to our /proc/meminfo virtualization to make 'free' work correctly on Fedora 23. https://bugzilla.redhat.com/show_bug.cgi?id=1300781 Cole Robinson (4): lxc: fuse: Unindent meminfo logic lxc: fuse: Fix /proc/meminfo size calculation lxc: fuse: Fill in MemAvailable for /proc/meminfo lxc: fuse: Stub out Slab bits in /proc/meminfo src/lxc/lxc_fuse.c | 141 +++++++++++++++++++++++++++++------------------------ 1 file changed, 76 insertions(+), 65 deletions(-) -- 2.5.0
Reverse the conditional at the start so we aren't stuffing all the logic in an 'if' block --- src/lxc/lxc_fuse.c | 122 ++++++++++++++++++++++++++--------------------------- 1 file changed, 61 insertions(+), 61 deletions(-) diff --git a/src/lxc/lxc_fuse.c b/src/lxc/lxc_fuse.c index 862dca3..86c0d10 100644 --- a/src/lxc/lxc_fuse.c +++ b/src/lxc/lxc_fuse.c @@ -161,68 +161,68 @@ static int lxcProcReadMeminfo(char *hostpath, virDomainDefPtr def, res = -1; while (copied < size && getline(&line, &n, fd) > 0) { char *ptr = strchr(line, ':'); - if (ptr) { - *ptr = '\0'; - - if (STREQ(line, "MemTotal") && - (virMemoryLimitIsSet(def->mem.hard_limit) || - virDomainDefGetMemoryActual(def))) { - virBufferAsprintf(new_meminfo, "MemTotal: %8llu kB\n", - meminfo.memtotal); - } else if (STREQ(line, "MemFree") && - (virMemoryLimitIsSet(def->mem.hard_limit) || - virDomainDefGetMemoryActual(def))) { - virBufferAsprintf(new_meminfo, "MemFree: %8llu kB\n", - (meminfo.memtotal - meminfo.memusage)); - } else if (STREQ(line, "Buffers")) { - virBufferAsprintf(new_meminfo, "Buffers: %8d kB\n", 0); - } else if (STREQ(line, "Cached")) { - virBufferAsprintf(new_meminfo, "Cached: %8llu kB\n", - meminfo.cached); - } else if (STREQ(line, "Active")) { - virBufferAsprintf(new_meminfo, "Active: %8llu kB\n", - (meminfo.active_anon + meminfo.active_file)); - } else if (STREQ(line, "Inactive")) { - virBufferAsprintf(new_meminfo, "Inactive: %8llu kB\n", - (meminfo.inactive_anon + meminfo.inactive_file)); - } else if (STREQ(line, "Active(anon)")) { - virBufferAsprintf(new_meminfo, "Active(anon): %8llu kB\n", - meminfo.active_anon); - } else if (STREQ(line, "Inactive(anon)")) { - virBufferAsprintf(new_meminfo, "Inactive(anon): %8llu kB\n", - meminfo.inactive_anon); - } else if (STREQ(line, "Active(file)")) { - virBufferAsprintf(new_meminfo, "Active(file): %8llu kB\n", - meminfo.active_file); - } else if (STREQ(line, "Inactive(file)")) { - virBufferAsprintf(new_meminfo, "Inactive(file): %8llu kB\n", - meminfo.inactive_file); - } else if (STREQ(line, "Unevictable")) { - virBufferAsprintf(new_meminfo, "Unevictable: %8llu kB\n", - meminfo.unevictable); - } else if (STREQ(line, "SwapTotal") && - virMemoryLimitIsSet(def->mem.swap_hard_limit)) { - virBufferAsprintf(new_meminfo, "SwapTotal: %8llu kB\n", - (meminfo.swaptotal - meminfo.memtotal)); - } else if (STREQ(line, "SwapFree") && - virMemoryLimitIsSet(def->mem.swap_hard_limit)) { - virBufferAsprintf(new_meminfo, "SwapFree: %8llu kB\n", - (meminfo.swaptotal - meminfo.memtotal - - meminfo.swapusage + meminfo.memusage)); - } else { - *ptr = ':'; - virBufferAdd(new_meminfo, line, -1); - } - - if (virBufferCheckError(new_meminfo) < 0) { - res = -errno; - goto cleanup; - } - - copied += strlen(line); - if (copied > size) - copied = size; + if (!ptr) + continue; + *ptr = '\0'; + + if (STREQ(line, "MemTotal") && + (virMemoryLimitIsSet(def->mem.hard_limit) || + virDomainDefGetMemoryActual(def))) { + virBufferAsprintf(new_meminfo, "MemTotal: %8llu kB\n", + meminfo.memtotal); + } else if (STREQ(line, "MemFree") && + (virMemoryLimitIsSet(def->mem.hard_limit) || + virDomainDefGetMemoryActual(def))) { + virBufferAsprintf(new_meminfo, "MemFree: %8llu kB\n", + (meminfo.memtotal - meminfo.memusage)); + } else if (STREQ(line, "Buffers")) { + virBufferAsprintf(new_meminfo, "Buffers: %8d kB\n", 0); + } else if (STREQ(line, "Cached")) { + virBufferAsprintf(new_meminfo, "Cached: %8llu kB\n", + meminfo.cached); + } else if (STREQ(line, "Active")) { + virBufferAsprintf(new_meminfo, "Active: %8llu kB\n", + (meminfo.active_anon + meminfo.active_file)); + } else if (STREQ(line, "Inactive")) { + virBufferAsprintf(new_meminfo, "Inactive: %8llu kB\n", + (meminfo.inactive_anon + meminfo.inactive_file)); + } else if (STREQ(line, "Active(anon)")) { + virBufferAsprintf(new_meminfo, "Active(anon): %8llu kB\n", + meminfo.active_anon); + } else if (STREQ(line, "Inactive(anon)")) { + virBufferAsprintf(new_meminfo, "Inactive(anon): %8llu kB\n", + meminfo.inactive_anon); + } else if (STREQ(line, "Active(file)")) { + virBufferAsprintf(new_meminfo, "Active(file): %8llu kB\n", + meminfo.active_file); + } else if (STREQ(line, "Inactive(file)")) { + virBufferAsprintf(new_meminfo, "Inactive(file): %8llu kB\n", + meminfo.inactive_file); + } else if (STREQ(line, "Unevictable")) { + virBufferAsprintf(new_meminfo, "Unevictable: %8llu kB\n", + meminfo.unevictable); + } else if (STREQ(line, "SwapTotal") && + virMemoryLimitIsSet(def->mem.swap_hard_limit)) { + virBufferAsprintf(new_meminfo, "SwapTotal: %8llu kB\n", + (meminfo.swaptotal - meminfo.memtotal)); + } else if (STREQ(line, "SwapFree") && + virMemoryLimitIsSet(def->mem.swap_hard_limit)) { + virBufferAsprintf(new_meminfo, "SwapFree: %8llu kB\n", + (meminfo.swaptotal - meminfo.memtotal - + meminfo.swapusage + meminfo.memusage)); + } else { + *ptr = ':'; + virBufferAdd(new_meminfo, line, -1); } + + if (virBufferCheckError(new_meminfo) < 0) { + res = -errno; + goto cleanup; + } + + copied += strlen(line); + if (copied > size) + copied = size; } res = copied; memcpy(buf, virBufferCurrentContent(new_meminfo), copied); -- 2.5.0
We virtualize bits of /proc/meminfo by replacing host values with values specific to the container. However for calculating the final size of the returned data, we are using the size of the original file and not the altered copy, which could give garbelled output. --- src/lxc/lxc_fuse.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/lxc/lxc_fuse.c b/src/lxc/lxc_fuse.c index 86c0d10..ffa6889 100644 --- a/src/lxc/lxc_fuse.c +++ b/src/lxc/lxc_fuse.c @@ -131,7 +131,6 @@ static int lxcProcHostRead(char *path, char *buf, size_t size, off_t offset) static int lxcProcReadMeminfo(char *hostpath, virDomainDefPtr def, char *buf, size_t size, off_t offset) { - int copied = 0; int res; FILE *fd = NULL; char *line = NULL; @@ -159,7 +158,7 @@ static int lxcProcReadMeminfo(char *hostpath, virDomainDefPtr def, } res = -1; - while (copied < size && getline(&line, &n, fd) > 0) { + while (getline(&line, &n, fd) > 0) { char *ptr = strchr(line, ':'); if (!ptr) continue; @@ -219,13 +218,11 @@ static int lxcProcReadMeminfo(char *hostpath, virDomainDefPtr def, res = -errno; goto cleanup; } - - copied += strlen(line); - if (copied > size) - copied = size; } - res = copied; - memcpy(buf, virBufferCurrentContent(new_meminfo), copied); + res = strlen(virBufferCurrentContent(new_meminfo)); + if (res > size) + res = size; + memcpy(buf, virBufferCurrentContent(new_meminfo), res); cleanup: VIR_FREE(line); -- 2.5.0
'free' on Fedora 23 will use MemAvailable to calculate its 'available' field, but we are passing through the host's value. Set it to match MemFree, which is what 'free' will do for older linux that don't have MemAvailable https://bugzilla.redhat.com/show_bug.cgi?id=1300781 --- src/lxc/lxc_fuse.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/lxc/lxc_fuse.c b/src/lxc/lxc_fuse.c index ffa6889..0a1c7dc 100644 --- a/src/lxc/lxc_fuse.c +++ b/src/lxc/lxc_fuse.c @@ -174,6 +174,14 @@ static int lxcProcReadMeminfo(char *hostpath, virDomainDefPtr def, virDomainDefGetMemoryActual(def))) { virBufferAsprintf(new_meminfo, "MemFree: %8llu kB\n", (meminfo.memtotal - meminfo.memusage)); + } else if (STREQ(line, "MemAvailable") && + (virMemoryLimitIsSet(def->mem.hard_limit) || + virDomainDefGetMemoryActual(def))) { + /* MemAvailable is actually MemFree + SRReclaimable + + some other bits, but MemFree is the closest approximation + we have */ + virBufferAsprintf(new_meminfo, "MemAvailable: %8llu kB\n", + (meminfo.memtotal - meminfo.memusage)); } else if (STREQ(line, "Buffers")) { virBufferAsprintf(new_meminfo, "Buffers: %8d kB\n", 0); } else if (STREQ(line, "Cached")) { -- 2.5.0
'free' on fedora23 wants to use the Slab field for calculated used memory. The equation is: used = MemTotal - MemFree - (Cached + Slab) - Buffers We already set Cached and Buffers to 0, do the same for Slab and its related values https://bugzilla.redhat.com/show_bug.cgi?id=1300781 --- src/lxc/lxc_fuse.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/lxc/lxc_fuse.c b/src/lxc/lxc_fuse.c index 0a1c7dc..1988c19 100644 --- a/src/lxc/lxc_fuse.c +++ b/src/lxc/lxc_fuse.c @@ -217,6 +217,12 @@ static int lxcProcReadMeminfo(char *hostpath, virDomainDefPtr def, virBufferAsprintf(new_meminfo, "SwapFree: %8llu kB\n", (meminfo.swaptotal - meminfo.memtotal - meminfo.swapusage + meminfo.memusage)); + } else if (STREQ(line, "Slab")) { + virBufferAsprintf(new_meminfo, "Slab: %8d kB\n", 0); + } else if (STREQ(line, "SReclaimable")) { + virBufferAsprintf(new_meminfo, "SReclaimable: %8d kB\n", 0); + } else if (STREQ(line, "SUnreclaim")) { + virBufferAsprintf(new_meminfo, "SUnreclaim: %8d kB\n", 0); } else { *ptr = ':'; virBufferAdd(new_meminfo, line, -1); -- 2.5.0
On Thu, Jan 21, 2016 at 01:42:56PM -0500, Cole Robinson wrote:
We need to add a few bits to our /proc/meminfo virtualization to make 'free' work correctly on Fedora 23.
ACK to all 4 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 01/22/2016 05:02 AM, Daniel P. Berrange wrote:
On Thu, Jan 21, 2016 at 01:42:56PM -0500, Cole Robinson wrote:
We need to add a few bits to our /proc/meminfo virtualization to make 'free' work correctly on Fedora 23.
ACK to all 4
Regards, Daniel
Thanks, pushed - Cole
participants (2)
-
Cole Robinson -
Daniel P. Berrange