freebsd: unifying buffer/cache memory usage for both ufs and zfs#2033
Conversation
|
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)
📝 WalkthroughWalkthroughThe PR replaces FreeBSD buffers and ARC accounting with a single Poem
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 |
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: fa32b47a-121c-4367-9db0-6061d621518c
📒 Files selected for processing (3)
freebsd/FreeBSDMachine.cfreebsd/FreeBSDMachine.hfreebsd/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)
freebsd/FreeBSDMachine.c (1)
319-321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the ZFS comment to match
cacheMem.This still refers to the ARC total and the old “buffer” class, but the code now uses a shrinkable ARC fallback for
cacheMem.Proposed wording
- // With ZFS, the ARC area is NOT counted in the 'buffers' class, but is still counted in the 'wired' - // class. The ARC total must thus be subtracted from the 'wired' class AND added to the 'buffer' class, - // so that the result is consistent with what ZFS users would expect. + // With ZFS, ARC is NOT counted in vfs.bufspace, but is still counted in the 'wired' + // class. Use the shrinkable ARC portion as a cache approximation when vfs.bufspace + // is unavailable, then subtract that cache value from 'wired'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b19c03eb-0ff6-450e-a158-ae22861e5ecd
📒 Files selected for processing (2)
freebsd/FreeBSDMachine.cfreebsd/Platform.c
|
@crocidb Thank you for this PR. On first glance the code looks okay. Please review that one cache size clamping issue that got flagged, as these kinds of edge cases tend to produce very annoying display issues. Apart from this, the code style asks for commits to be "final" without later commits in a PR reverting or fixing them. Please use git rebase to squash/fixup the commits in your PR so these fixes are integrated with where each issue was introduced. |
|
Hi @BenBE! Thanks for the quick reply. I applied the changes suggested and squashed into one single commit. |
Hi everyone! :)
This is more of a suggestion on how to improve readability of FreeBSD memory monitoring. Here I'm basically unifying
MEMORY_CLASS_BUFFERandMEMORY_CLASS_ARCintoMEMORY_CLASS_CACHE. This is because of a few reasons:MEMORY_CLASS_BUFFERis checking forvfs.bufspace, which reports the metadata buffer cache for UFS specifically. On ZFS only, it's always zero, because ARC bypasses the vfs system.MEMORY_CLASS_ARCis not being subtracted fromwiredmemory (although being part of it), and is being counted as used memory, althoughMEMORY_CLASS_BUFFERis not. And the behavior of disk cache on Linux is to not being counted as used.With the new
MEMORY_CLASS_CACHE, it will always report the current buffer/cache for the current filesystems. It's easier to understand and more accurate not considering ARC cache as used memory.EDIT: I did not consider, at first, that both UFS and ZFS could be active in the system, but it's not true. So I changed to effectively sum those two values.
Thanks.