pcp: correct the Linux memory classes and the used memory total - #2080
pcp: correct the Linux memory classes and the used memory total#2080ravi-arnan wants to merge 2 commits into
Conversation
pcp/Platform.c carries a copy of the Linux memory class table from linux/Platform.c, but the MEMORY_CLASS_* enum it is keyed on lists the classes in a different order: native used shared compressed buffers cache available pcp used shared buffers cache compressed available The table was copied entry for entry, so the designated initialisers attach the native sequence of labels and flags to the pcp sequence of names, and the last four labels come out rotated by one slot: the buffers figure is labelled compressed, the page cache is labelled buffers and the zswap pool is labelled cache. Against native htop on the same machine at the same instant: htop Mem: used:6.27G shared:940M compressed:0K buffers:289M cache:14.1G available:15.5G pcp-htop Mem: used:5.70G shared:940M compressed:289M buffers:14.1G cache:0K available:15.5G The flags rotate with the labels, which is the more serious half. The slot holding the buffers figure inherits countsAsUsed from the native compressed class, so 289M of buffers is drawn as used memory, while the page cache and zswap slots end up carrying neither countsAsUsed nor countsAsCache, so MemoryMeter_updateValues() sets them to NAN and 14.1G of page cache is not drawn at all. In bar mode that is 29 of 93 cells filled where native fills 82. Reorder the enum to match the native one and key each table entry to its own label, which leaves the two tables identical. Keeping .color in MEMORY_1..MEMORY_6 index order is not cosmetic: BarMeterMode_draw() colours segment i with MemoryMeter_attributes[i] and ignores .color, while MemoryMeter_display() uses .color, so the bar and the text would otherwise disagree about the colour of a class. Darwin shares the enum and keeps its own order, so its compressed class, which sits at a different index to the Linux one, needs a name of its own. Assisted-by: Claude:opus-5
PCPMachine_updateLinuxMemoryInfo() stores the display-adjusted page cache in memValue[MEMORY_CLASS_CACHE], which already has SReclaimable added and Shmem subtracted, then feeds that value into usedDiff and adds SReclaimable to it a second time. LinuxMachine_scanMemoryInfo() deliberately does not do this. It stores the adjusted figure in this->cachedMem but keeps the raw cachedMem in usedDiff, and the comment above it says why: Shmem is part of Cached, so it must not be subtracted from used twice. pcp's usedDiff is therefore larger than the native one by exactly SReclaimable - Shmem, and the used figure is smaller by the same amount. Measured against /proc/meminfo sampled with the screen: total 24339388 free 1704816 cached 14215516 sreclaim 1555620 shmem 963048 (kB) native usedDiff = 17772384 used = 6567004 kB = 6.27G screen said 6.27G pcp usedDiff = 18364956 used = 5974432 kB = 5.70G screen said 5.70G gap 592572 kB, which is SReclaimable - Shmem exactly On an ordinary desktop that is half a gigabyte of used memory going unreported, and it grows with the reclaimable slab. Keep the raw Cached in a local and use that for usedDiff, as native does. Assisted-by: Claude:opus-5
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change separates Darwin compressed memory from the generic compressed-memory class. It reorders Linux memory classes and corrects their labels, flags, and colors. Linux accounting now retains raw cached memory for used-memory calculations while adjusting only the displayed cache value for shared memory. Poem
Merge Risk: ⚪ Minimal · up to The changes correct Linux memory classification and used-memory accounting without any remaining actionable merge-blocking risk; the PR is merge-ready after normal checks and review. 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 |
2 participants
Fixes the two defects in
pcp/reported in #2075. Two commits, one per defect, since they are independent and the second survives a fix for the first.Closes #2075
@natoscott, taking you up on the enum reorder. All the measurements below are on PCP 7.2.0, using the tip you gave me on the issue: no
pmcd, thelinuxPMDA attached as a DSO through the local context.1. The memory classes were rotated by one slot
pcp/Platform.ccarries a copy of the Linux memory class table fromlinux/Platform.c, but theMEMORY_CLASS_*enum it is keyed on lists the classes in a different order:The table was copied entry for entry, so the designated initialisers attach the native sequence of labels and flags to the pcp sequence of names, and the last four come out rotated.
The flags rotate with the labels, which is the more serious half. The slot holding the buffers figure inherits
countsAsUsedfrom the native compressed class, so buffers is drawn as used memory; the page cache and zswap slots end up carrying neithercountsAsUsednorcountsAsCache, soMemoryMeter_updateValues()sets them toNANand the page cache is not drawn at all.The fix reorders the enum to match the native one and keys each table entry to its own label, which leaves the two tables identical. Darwin shares the enum and keeps its own order, so its compressed class needs a name of its own.
2.
SReclaimableis subtracted from used twicePCPMachine_updateLinuxMemoryInfo()stores the display-adjusted page cache inmemValue[MEMORY_CLASS_CACHE], which already hasSReclaimableadded andShmemsubtracted, then feeds that value intousedDiffand addsSReclaimablea second time.LinuxMachine_scanMemoryInfo()deliberately does not: it keeps the rawcachedMeminusedDiff, and the comment above it says why.So pcp's
usedDiffis larger than native's by exactlySReclaimable - Shmem, andusedis smaller by the same amount. This one is independent of the labels and would survive a fix that only re-keyed the table.Measured
Four binaries built from the same tree, started together, captured at one instant against
/proc/meminfosampled with the screen. Memory meter isolated, TEXT in the left column and BAR in the right.htop(native, reference)used:6.23G shared:1.05G compressed:0K buffers:290M cache:14.3G available:15.5Gpcp-htopon6f33dddused:5.79G shared:1.05G compressed:290M buffers:14.3G cache:0K available:15.5Gpcp-htopwith commit 1 onlyused:5.79G shared:1.05G compressed:0K buffers:290M cache:14.3G available:15.5Gpcp-htopwith bothused:6.23G shared:1.05G compressed:0K buffers:290M cache:14.3G available:15.5GThe third row is what makes the case for two commits: the labels and the bar are right, and
usedis still 5.79G.The arithmetic checks out against the same meminfo, so the screen and the source agree:
Same run with
show_cached_memory=0, since commit 1 changescountsAsCacheand that is the setting it feeds:htop(native)pcp-htopon6f33dddpcp-htopwith bothZero warnings under the default warning set on all three pcp builds.
What is not covered
The Darwin path is compiled but not run, since I have no macOS to point PCP at. The rename is mechanical:
MEMORY_CLASS_COMPRESSEDis replaced byMEMORY_CLASS_DARWIN_COMPRESSEDin the three places Darwin uses it, its index 4 is unchanged, and the Linux uses ofMEMORY_CLASS_COMPRESSED(the zswap pool inPlatform_setLinuxMemoryValues) keep the old name.One interaction worth flagging: if #2074 lands,
darwin/Platform.creorders its own memory classes, andpcp/Platform.c's Darwin table would then be out of step with it in the same way this PR fixes for Linux. Happy to send that as a follow-up in whichever order the two land.Assisted-by: Claude:opus-5