linux: cgroup v2-aware Memory & Swap meters#2031
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds cgroup v2 parsing and scan code for Linux memory and swap metrics, wires the scan into Linux machine state, adds container-specific meter classes and platform setters, changes default labels when running in a container, and adds a unit test binary plus build wiring. Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)linux/Platform.c┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.18][ERROR]: unable to find a config; path linux/Platform.h┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.20][ERROR]: unable to find a config; path linux/LinuxMachine.h┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.33][ERROR]: unable to find a config; path Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b41e166e-6cba-4f24-8c47-d4f8ba2ccaca
📒 Files selected for processing (10)
DisplayOptionsPanel.cMakefile.amSettings.cSettings.hlinux/CGroupMem.clinux/CGroupMem.hlinux/CGroupMemTest.clinux/LinuxMachine.clinux/LinuxMachine.hlinux/Platform.c
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
linux/CGroupMemTest.c (1)
84-91: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail fast on fixture write errors in
writeFile().
writeFile()currently swallowsfopen/fputs/fclosefailures, which can cause misleading failures later intest_readNode()with no direct signal that fixture creation failed.Proposed fix
static void writeFile(const char* dir, const char* name, const char* body) { char path[1024]; snprintf(path, sizeof(path), "%s/%s", dir, name); FILE* fp = fopen(path, "w"); - if (fp) { - fputs(body, fp); - fclose(fp); - } + CHECK(fp != NULL); + if (!fp) + return; + + CHECK(fputs(body, fp) >= 0); + CHECK(fclose(fp) == 0); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 642a2b6f-95a8-46c8-a62b-e5ae2ad89181
📒 Files selected for processing (3)
DisplayOptionsPanel.clinux/CGroupMem.clinux/CGroupMemTest.c
BenBE
added
needs-discussion 🤔
BenBE
left a comment
BenBE
left a comment
There was a problem hiding this comment.
Using existing functions from Compat.h and XUtils.h is highly encouraged, as those functions are intended to handle common error cases. Also using the openat style APIs whereever available speeds up file access as it minimizes work for the kernel when parsing file paths. I'm sure, parsing CGroup data from a content block is probably implemented somewhere already too.
That said, please rebase your commits and squash them into units that are more aligned with what functionality you are implementing. Instead of 10+ commits, we probably can do with 5 or 6: Implement missing CGroup parsing stuff, implement collecting CGroup memory stats, implement the setting to switch to CGroup memory stats, update the memory/swap meter to use the new information+setting.
Finally, as your code is written with heavy AI assistance, please carefully review the style guide again, to better instruct the assistant to re-use existing functionality and avoiding much of the NIH created by it.
@coderabbitai Please pay more attention for patterns that reimplement functions from Compat.h or XUtil.h (like e.g. snprintf or strchrnul) in new code: Unless explicitly justified functions from these two modules should be used instead of duplicating their code patterns or calling their underlying functions directly.
| /* Read up to size-1 bytes of nodeDir/name into buf (NUL-terminated). Returns false on failure. */ | ||
| static bool CGroupMem_readFile(const char* nodeDir, const char* name, char* buf, size_t size) { | ||
| char path[PATH_MAX]; | ||
| int n = snprintf(path, sizeof(path), "%s/%s", nodeDir, name); | ||
| if (n < 0 || (size_t)n >= sizeof(path)) | ||
| return false; | ||
|
|
||
| FILE* fp = fopen(path, "r"); | ||
| if (!fp) | ||
| return false; | ||
|
|
||
| size_t got = fread(buf, 1, size - 1, fp); | ||
| buf[got] = '\0'; | ||
| fclose(fp); | ||
| return got > 0; | ||
| } |
There was a problem hiding this comment.
Duplicates code from Compat.h.
Should use Compat_readfileat.
Also, bare use of snprintf should use xSnprintf instead.
Furthermore avoid fopen/fread/fwrite/fclose API in memory-related code paths (might cause crashes in memory-constrained circumstances.
| if (strncmp(content, "max", 3) == 0) | ||
| return false; |
There was a problem hiding this comment.
Cf. XUtils.h function String_startsWith.
| return false; | ||
|
|
||
| char* end; | ||
| unsigned long long bytes = strtoull(content, &end, 10); |
There was a problem hiding this comment.
Prefer fast_strtoull_dec in hot paths.
Prefer types like uint64_t from stdint.h
| const char* eol = strchr(p, '\n'); | ||
| size_t lineLen = eol ? (size_t)(eol - p) : strlen(p); |
| size_t lineLen = eol ? (size_t)(eol - p) : strlen(p); | ||
|
|
||
| /* match "key " at the start of the line (exact token, space-delimited) */ | ||
| if (lineLen > keyLen && strncmp(p, key, keyLen) == 0 && p[keyLen] == ' ') { |
| fp = fopen("/proc/self/cgroup", "r"); | ||
| if (fp) { | ||
| size_t got = fread(fileBuf, 1, sizeof(fileBuf) - 1, fp); | ||
| fileBuf[got] = '\0'; | ||
| fclose(fp); |
| if (cgPath[0] == '/' && cgPath[1] == '\0') | ||
| n = snprintf(nodeDir, sizeof(nodeDir), "%s", mountBuf); | ||
| else | ||
| n = snprintf(nodeDir, sizeof(nodeDir), "%s%s", mountBuf, cgPath); |
| unsigned long long limit; /* memory.max, kB */ | ||
| unsigned long long current; /* memory.current, kB */ | ||
| unsigned long long file; /* memory.stat 'file', kB */ | ||
| unsigned long long swapLimit; /* memory.swap.max, kB (may be 0 = swap disabled) */ | ||
| unsigned long long swapCurrent; /* memory.swap.current, kB */ |
There was a problem hiding this comment.
There's usually no extra unit tests for code.
| check_PROGRAMS = linux/CGroupMemTest | ||
| TESTS = $(check_PROGRAMS) | ||
| linux_CGroupMemTest_SOURCES = linux/CGroupMemTest.c linux/CGroupMem.c linux/CGroupMem.h | ||
| linux_CGroupMemTest_CPPFLAGS = $(AM_CPPFLAGS) -D_DEFAULT_SOURCE | ||
|
|
|
With the code review out of the way, I don't quite like the way the default change for the new information is handled. Neither that it's active by default nor that it overrides the Memory/Swap meters entirely. Open to hear what others think on that subject. |
|
Hey @BenBE thanks for your thorough review and especially for your patience! It's my first contribution to the project and I've wished for this feature for a long time. I will take a moment to address everything, with a goal to minimize the changeset through better reuse of the existing functionality. Edit: draft PR until I'm ready for your next review. |
|
Regarding the presentation of the new information... My angle of attack here is that htop is currently lying to the unaware user. I imagine a data scientist without in-depth system understanding, working in a containerized workspace. They launch htop to understand how resource constrained they are, how much further they can push their use. But they have no idea about host vs cgroups resource limits. The actionable info for this class of user I would argue is the cgroups limit and not the host memory. |
Oblynx
force-pushed
the
feat/cgroup-v2-memory
branch
from
a65ea48 to
a57f8c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3f62de0f-a2e9-41b3-8970-c0999c559f1c
📒 Files selected for processing (10)
DisplayOptionsPanel.cMakefile.amSettings.cSettings.hlinux/CGroupMem.clinux/CGroupMem.hlinux/CGroupMemTest.clinux/LinuxMachine.clinux/LinuxMachine.hlinux/Platform.c
| char mountBuf[PATH_MAX]; | ||
| char cgPath[PATH_MAX]; | ||
| char fileBuf[8192]; | ||
|
|
||
| FILE* fp = fopen("/proc/self/mountinfo", "r"); | ||
| if (fp) { | ||
| size_t got = fread(fileBuf, 1, sizeof(fileBuf) - 1, fp); | ||
| fileBuf[got] = '\0'; | ||
| fclose(fp); | ||
| if (!CGroupMem_parseMountinfo(fileBuf, mountBuf, sizeof(mountBuf))) | ||
| mountBuf[0] = '\0'; | ||
| } else { | ||
| mountBuf[0] = '\0'; | ||
| } | ||
|
|
||
| if (mountBuf[0]) { | ||
| fp = fopen("/proc/self/cgroup", "r"); | ||
| if (fp) { | ||
| size_t got = fread(fileBuf, 1, sizeof(fileBuf) - 1, fp); | ||
| fileBuf[got] = '\0'; | ||
| fclose(fp); | ||
| if (CGroupMem_parseSelfCgroup(fileBuf, cgPath, sizeof(cgPath))) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid fixed-size snapshots for /proc/self/mountinfo.
fileBuf is capped at 8192 bytes and parsed after a single read. On systems with many mounts, /proc/self/mountinfo can exceed that, so the cgroup2 entry may be truncated out and available stays false. The new meters then fall back to host totals in containerized environments where this feature is supposed to apply. Read until EOF, or scan line-by-line until the cgroup2 record is found, instead of parsing a fixed-size prefix.
| /* must not match the "file_mapped" prefix when asked for "file" — exact token only */ | ||
| CHECK(CGroupMem_parseStat(stat, "slab", &kb) == false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the prefix-match case you describe.
This assertion does not test the "file" vs "file_mapped" boundary noted in the comment; it only checks that an unrelated missing key returns false. If CGroupMem_parseStat() regressed to prefix-match file_mapped, this test would still pass. Add a case where "file" is queried against input that contains only file_mapped.
Proposed fix
- /* must not match the "file_mapped" prefix when asked for "file" — exact token only */
- CHECK(CGroupMem_parseStat(stat, "slab", &kb) == false);
+ /* must not match the "file_mapped" prefix when asked for "file" — exact token only */
+ CHECK(CGroupMem_parseStat("file_mapped 100\n", "file", &kb) == false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* must not match the "file_mapped" prefix when asked for "file" — exact token only */ | |
| CHECK(CGroupMem_parseStat(stat, "slab", &kb) == false); | |
| /* must not match the "file_mapped" prefix when asked for "file" — exact token only */ | |
| CHECK(CGroupMem_parseStat("file_mapped 100\n", "file", &kb) == false); |
|
After some internal discussion, one idea that came up regarding the implementation of this PR was to keep the original Memory and Swap meters as-is, but create those CGroup-aware meters as new meters instead. When setting up htop (i.e. when there's no config) these new meters could then be used instead of the classical ones for the default configuration. That way there's not even a need to have the extra setting, while OTOH allowing the user to see both of these statistics at the same time. |
Oblynx
force-pushed
the
feat/cgroup-v2-memory
branch
from
a57f8c7 to
745dfba
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8eb306c9-2ba2-40f7-af64-6d078620ba1a
📒 Files selected for processing (10)
DisplayOptionsPanel.cMakefile.amSettings.cSettings.hlinux/CGroupMem.clinux/CGroupMem.hlinux/CGroupMemTest.clinux/LinuxMachine.clinux/LinuxMachine.hlinux/Platform.c
| bool CGroupMem_parseMountinfo(const char* content, char* mountBuf, size_t mountSize) { | ||
| if (!content || mountSize == 0) | ||
| return false; | ||
|
|
||
| const char* p = content; | ||
| while (*p) { | ||
| const char* eol = strchr(p, '\n'); | ||
|
|
||
| /* the filesystem type is the first token after the " - " separator */ | ||
| const char* sep = strstr(p, " - "); | ||
| if (sep && (!eol || sep < eol)) { | ||
| const char* fstype = sep + 3; | ||
| if (strncmp(fstype, "cgroup2 ", 8) == 0 || strncmp(fstype, "cgroup2\t", 8) == 0) { | ||
| /* mount point is the 5th space-separated field (index 4) of the pre-separator part */ | ||
| const char* q = p; | ||
| int field = 0; | ||
| while (field < 4 && q < sep) { | ||
| q = strchr(q, ' '); | ||
| if (!q || q >= sep) | ||
| break; | ||
| q++; | ||
| field++; | ||
| } | ||
| if (field == 4 && q < sep) { | ||
| const char* mpEnd = strchr(q, ' '); | ||
| if (mpEnd && mpEnd <= sep) { | ||
| size_t mpLen = (size_t)(mpEnd - q); | ||
| if (mpLen >= mountSize) | ||
| mpLen = mountSize - 1; | ||
| memcpy(mountBuf, q, mpLen); | ||
| mountBuf[mpLen] = '\0'; | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!eol) | ||
| break; | ||
| p = eol + 1; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the mount root when resolving the cgroup node.
/proc/self/mountinfo can legally expose multiple cgroup2 mounts, including bind-mounted subtrees. This parser keeps only the first mount point, and CGroupMem_resolveNodeDir() later appends the namespace-relative /proc/self/cgroup path directly to it. For a bind mount like root=/foo/bar, mountpoint=/mnt/cg with 0::/bar/baz, that resolves /mnt/cg/bar/baz instead of /mnt/cg/baz, so the scan reads the wrong node or falls back to host totals. Carry the mount root through resolution and normalize the cgroup path against it, then add a fixture that covers a bind-mounted cgroup2 subtree.
Also applies to: 199-216
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 116-116: The function 'CGroupMem_parseMountinfo' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
| bool CGroupMem_parseUsage(const char* content, unsigned long long* outKB) { | ||
| if (!content) | ||
| return false; | ||
|
|
||
| char* end; | ||
| unsigned long long bytes = strtoull(content, &end, 10); | ||
| if (end == content) | ||
| return false; | ||
|
|
||
| *outKB = bytes / 1024; | ||
| return true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject numeric prefixes with trailing garbage.
strtoull() succeeding on any leading digits makes inputs like "123junk\n" parse as valid usage. That violates the documented "false if not a number" contract here and in CGroupMem_parseLimit(), which delegates to this helper. Require the remainder to be only optional whitespace/newline before returning true, and add a regression case in linux/CGroupMemTest.c.
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 160-160: The function 'CGroupMem_parseUsage' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
| if (settings->cgroupAwareMeters && lhost->cgroupMem.active) { | ||
| /* cgroup v2 mode: total = limit; used = current - file (anon+kernel); cache = file. */ | ||
| unsigned long long limit = lhost->cgroupMem.limit; | ||
| unsigned long long current = lhost->cgroupMem.current; | ||
| unsigned long long file = lhost->cgroupMem.file; | ||
| unsigned long long used = (current > file) ? (current - file) : 0; | ||
|
|
||
| this->total = limit; | ||
| this->values[MEMORY_CLASS_USED] = used; | ||
| this->values[MEMORY_CLASS_SHARED] = 0; | ||
| this->values[MEMORY_CLASS_COMPRESSED] = 0; | ||
| this->values[MEMORY_CLASS_BUFFERS] = 0; | ||
| this->values[MEMORY_CLASS_CACHE] = file; | ||
| this->values[MEMORY_CLASS_AVAILABLE] = (limit > current) ? (limit - current) : 0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clamp cgroup usage to the configured limit before feeding the meter.
Linux allows memory.current to sit above memory.max temporarily, and memory.swap.current can remain above a lowered memory.swap.max while reclaim catches up. Passing those raw values straight into a meter whose total is the limit lets the bars exceed 100% in valid kernel states. Clamp the displayed segments to the corresponding limit before assigning them. (kernel.org)
Proposed fix
unsigned long long limit = lhost->cgroupMem.limit;
unsigned long long current = lhost->cgroupMem.current;
unsigned long long file = lhost->cgroupMem.file;
- unsigned long long used = (current > file) ? (current - file) : 0;
+ if (current > limit)
+ current = limit;
+ if (file > current)
+ file = current;
+ unsigned long long used = current - file;
@@
- this->values[SWAP_METER_USED] = lhost->cgroupMem.swapCurrent;
+ this->values[SWAP_METER_USED] = MINIMUM(lhost->cgroupMem.swapCurrent, lhost->cgroupMem.swapLimit);Also applies to: 499-503
Introduce linux/CGroupMem with pure, string-input parsers for the cgroup
v2 files htop needs to discover and read its own memory node:
- parseLimit — memory.max / memory.swap.max ("max" or bytes -> kB)
- parseUsage — memory.current / memory.swap.current (bytes -> kB)
- parseStat — a named field (e.g. "file") from memory.stat, exact-token
- parseSelfCgroup — the v2 ("0::") path from /proc/self/cgroup
- parseMountinfo — the cgroup2 mount point from /proc/self/mountinfo
Values use uint64_t and the parsers build on htop's String_startsWith,
String_strchrnul and String_safeStrncpy helpers rather than open-coding
prefix/scan/copy logic. They take strings rather than touching the
filesystem, so they are covered by a standalone unit test
(linux/CGroupMemTest) wired into check_PROGRAMS/TESTS. No behaviour
change yet: nothing calls them.
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Konstantinos Samaras-Tsakiris <5130925+Oblynx@users.noreply.github.com>
Build the collection layer on top of the parsers and feed it from the
Linux memory scan:
- CGroupMemData holds the effective limit/usage/file-cache and the swap
equivalents (all uint64_t kB), plus active/swapActive flags.
- CGroupMem_readNode opens the node directory once and reads
memory.{max,current,stat} and the swap files through Compat_openat /
Compat_readfileat, keeping stdio out of this memory-sensitive path. A
node is "active" only when a finite memory.max is below the host total;
swap is only entered when memory itself is limited, so the Memory and
Swap meters stay consistent.
- CGroupMem_scan resolves our own v2 node once (cgroup2 mount from
/proc/self/mountinfo + the "0::" path from /proc/self/cgroup, the
namespace root inside a container) and reads it on every refresh.
- LinuxMachine_scanMemoryInfo now calls CGroupMem_scan into a cgroupMem
field, gated on the host total.
readNode is unit-tested against a temp directory. Still no UI change:
the data is gathered but not yet displayed.
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Konstantinos Samaras-Tsakiris <5130925+Oblynx@users.noreply.github.com>
Add two new meters, ContainerMemory ("Cmem") and ContainerSwap ("Cswp"),
that present the container's cgroup v2 limits (collected in the previous
commit) and fall back to the host figures when no cgroup limit is in
effect. They are plain additional meters — the classic Memory/Swap meters
are untouched — so host and container figures can be shown side by side.
- linux/ContainerMeter.{c,h}: the two MeterClasses, reusing the classic
rendering (MemoryMeter_display / SwapMeter_display + attribute tables).
- MemoryMeter/SwapMeter: factor the value-update body into
MemoryMeter_updateValuesWith() / SwapMeter_updateValuesWith(), which take
the Platform value-setter as a parameter, so the Container meters reuse
it instead of duplicating it. The attribute arrays and display functions
are exported for that reuse.
- linux/Platform.c: Platform_setCGroupMemoryValues / Platform_setCGroupSwapValues
fill the meter from LinuxMachine.cgroupMem when a limit is active (memory:
total=limit, used=current-file, cache=file, available=limit-current;
swap: total=swapLimit, used=swapCurrent) and delegate to the host setters
otherwise. Both meters are registered in Platform_meterTypes[].
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Konstantinos Samaras-Tsakiris <5130925+Oblynx@users.noreply.github.com>
When htop starts without a configuration and detects it is running inside a container, seed the default header layout with the ContainerMemory and ContainerSwap meters instead of the classic Memory/Swap ones, so a containerized user sees the limits that actually apply with no setup and no extra setting. Containerization is exposed generically as Machine.containerized (set from the Linux Running_containerized probe in LinuxMachine_new; false on every other platform), so the cross-platform Settings_defaultMeters can pick the meter names without depending on Linux-only symbols — the Container* names are only ever selected where those meters are registered. The ordering is safe: Platform_init() detects containerization before Machine_new() copies the flag and Settings_new() seeds the defaults. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Konstantinos Samaras-Tsakiris <5130925+Oblynx@users.noreply.github.com>
Oblynx
force-pushed
the
feat/cgroup-v2-memory
branch
from
745dfba to
d19ae8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9290e9c6-86db-4771-91fb-98e36e2b229c
📒 Files selected for processing (17)
Machine.cMachine.hMakefile.amMemoryMeter.cMemoryMeter.hSettings.cSwapMeter.cSwapMeter.hlinux/CGroupMem.clinux/CGroupMem.hlinux/CGroupMemTest.clinux/ContainerMeter.clinux/ContainerMeter.hlinux/LinuxMachine.clinux/LinuxMachine.hlinux/Platform.clinux/Platform.h
| } while (0) | ||
|
|
||
|
|
||
| static void test_parseLimit(void) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Align the test helper names with the repo’s C naming convention.
test_parseLimit, test_parseUsage, test_parseStat, test_parseSelfCgroup, test_parseMountinfo, writeFile, and test_readNode do not follow the ModuleName_functionName() form used elsewhere in-tree. Renaming them to CGroupMemTest_* keeps the new test file consistent and easier to navigate.
As per coding guidelines: **/*.c: Use ModuleName_functionName() naming convention for functions (e.g., Process_compare()).
Also applies to: 43-43, 50-50, 65-65, 79-79, 91-91, 101-101
Source: Coding guidelines
| const MeterClass ContainerMemoryMeter_class = { | ||
| .super = { | ||
| .extends = Class(Meter), | ||
| .delete = Meter_delete, | ||
| .display = MemoryMeter_display, | ||
| }, | ||
| .updateValues = ContainerMemoryMeter_updateValues, | ||
| .defaultMode = BAR_METERMODE, | ||
| .supportedModes = METERMODE_DEFAULT_SUPPORTED, | ||
| .maxItems = 6, // maximum of MEMORY_N settings | ||
| .isPercentChart = true, | ||
| .total = 100.0, | ||
| .attributes = MemoryMeter_attributes, | ||
| .name = "ContainerMemory", | ||
| .uiName = "Container memory", | ||
| .caption = "Mem" | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the container meter label state-aware.
These are separate meter classes, but their captions stay Mem/Swp while the paired setters can either show cgroup values or fall back to host totals. That means the UI never signals when a bar is cgroup-scoped, and if users show both host and container meters together the labels are ambiguous. The fix needs to happen at the contract level: either only select these classes when the cgroup-backed path is active, or make the displayed caption vary with the active scope instead of hard-coding the host labels here.
Also applies to: 45-61
| check_PROGRAMS = linux/CGroupMemTest | ||
| TESTS = $(check_PROGRAMS) | ||
| linux_CGroupMemTest_SOURCES = linux/CGroupMemTest.c linux/CGroupMem.c linux/CGroupMem.h XUtils.c linux/Compat.c | ||
| linux_CGroupMemTest_CPPFLAGS = $(AM_CPPFLAGS) -D_DEFAULT_SOURCE |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the new unit test behind HTOP_LINUX.
check_PROGRAMS and TESTS are unconditional here, so make check on non-Linux targets will still try to build linux/CGroupMemTest and link Linux-only sources. Wrap this block in if HTOP_LINUX ... endif so the new test only participates in Linux builds.
Suggested fix
+if HTOP_LINUX
check_PROGRAMS = linux/CGroupMemTest
TESTS = $(check_PROGRAMS)
linux_CGroupMemTest_SOURCES = linux/CGroupMemTest.c linux/CGroupMem.c linux/CGroupMem.h XUtils.c linux/Compat.c
linux_CGroupMemTest_CPPFLAGS = $(AM_CPPFLAGS) -D_DEFAULT_SOURCE
+endifBased on the PR objective, this test target is Linux-specific.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| check_PROGRAMS = linux/CGroupMemTest | |
| TESTS = $(check_PROGRAMS) | |
| linux_CGroupMemTest_SOURCES = linux/CGroupMemTest.c linux/CGroupMem.c linux/CGroupMem.h XUtils.c linux/Compat.c | |
| linux_CGroupMemTest_CPPFLAGS = $(AM_CPPFLAGS) -D_DEFAULT_SOURCE | |
| if HTOP_LINUX | |
| check_PROGRAMS = linux/CGroupMemTest | |
| TESTS = $(check_PROGRAMS) | |
| linux_CGroupMemTest_SOURCES = linux/CGroupMemTest.c linux/CGroupMem.c linux/CGroupMem.h XUtils.c linux/Compat.c | |
| linux_CGroupMemTest_CPPFLAGS = $(AM_CPPFLAGS) -D_DEFAULT_SOURCE | |
| endif |
2 participants
cgroup v2-aware Memory & Swap meters
Inside a container (Kubernetes pod, Docker, LXC) htop's Memory/Swap meters read host-wide values from
/proc/meminfo, which the kernel does not namespace per-container — so a pod limited to 512 MiB on a large host shows the host's RAM as if it were available. Addresses the long-standing #1538 (the CPU half, #1020, is a planned follow-up).This adds two new meters, Container Memory and Container Swap, that show usage against the container's cgroup v2 limit (
memory.max/memory.swap.max) and fall back to host totals when no limit is in effect. When htop starts without a config and detects it is containerized, the default header layout uses these meters instead of the classic ones — so a containerized user sees the limits that actually apply, with no setup and no new setting. The classic Memory/Swap meters are unchanged, and both can be shown side by side.docker runon a 13.1 G host:--memory=512m:--memory=256m --memory-swap=384m(256 M RAM + 128 M swap):No limit (host fallback):
Design
linux/CGroupMem.c— libc-only parsers plus aCompat_openat/Compat_readfileatreader for our own cgroup v2 node. Inside a cgroup namespace/proc/self/cgroupis0::/and ancestors aren't visible, so we read the node we are in; the limit enforced there is what matters.max(unlimited) and cgroup v1 fall back to host totals cleanly.linux/ContainerMeter.c— the two meters, reusing the classicMemoryMeter/SwapMeterrendering through a shared*_updateValuesWith()helper (no duplicated draw/attribute code). Values:used = memory.current − file,cache = memory.stat:file, total =memory.max; swap analogously. Swap cgroup mode is only entered when memory is also limited, keeping the two meters consistent.Machine.containerizedflag (set from the existingRunning_containerizedprobe), so the cross-platformSettings_defaultMetersselects the meter names without depending on Linux-only symbols.Tests
make checktargetlinux/CGroupMemTestunit-tests the parsers and the node reader against synthetic fixtures (30 checks, incl.maxsentinel, namespaced0::/,swap.maxgating). This adds the firstcheck_PROGRAMS/TESTStarget — happy to relocate if you'd prefer.--memory/--memory-swapexactly). Clean-Wall -Wextrabuild, no new warnings.AI assistance
Developed with AI assistance (Claude); every commit carries an
Assisted-by:trailer per the project's AI-contributions policy, and the work has been reviewed and verified by me.