linux/GPU: add per-process GPU memory column (GPU_MEMORY) - #2121
Open
rajasekharponakala wants to merge 2 commits into
Open
rajasekharponakala wants to merge 2 commits into
rajasekharponakala wants to merge 2 commits into
Conversation
btop shows per-process GPU memory usage, which htop lacked. htop already tracks per-process GPU engine time/utilization (GPU_TIME, GPU_PERCENT) by parsing the generic DRM fdinfo stats (drm-engine-*), so extend the same fdinfo parser to also sum drm-resident-<region>/drm-memory-<region> (the deprecated amdgpu-only alias for resident) values, deduplicated per client the same way engine time already is, and expose the result as a new GPU_MEMORY column. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Tgh778fwKS3MFYmr7wd3M
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthrough
Priority: ➖ Normal Change: Feature Merge Risk: 🟡 Moderate · up to GPU memory can be reported too high for amdgpu processes, affecting the new column and its sort order. Deduplicate the aliases before merging. 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. DRM numbers gather in flight 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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 34176738-a2ee-463c-a24b-f7fa025c7729
📒 Files selected for processing (4)
linux/GPU.clinux/LinuxProcess.clinux/LinuxProcess.hlinux/ProcessField.h
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Comment on lines
+216
to
+228
|
|
||
| unsigned long long int bytesValue; | ||
| if (errno != 0) | ||
| continue; | ||
| else if (String_startsWith(endptr, "KiB")) | ||
| bytesValue = value * 1024ULL; | ||
| else if (String_startsWith(endptr, "MiB")) | ||
| bytesValue = value * 1024ULL * 1024ULL; | ||
| else | ||
| continue; | ||
|
|
||
| if (sstate == SECST_UNKNOWN) { | ||
| if (client_id != INVALID_CLIENT_ID && !is_duplicate_client(parsed_ids, client_id, pdev)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '185,240p' linux/GPU.c
rg -n 'strtoull|UINT64_MAX|ULLONG_MAX|bytesValue|new_gpu_memory' linux/GPU.c linux tests | head -240Repository: htop-dev/htop
Length of output: 6673
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- repository guidance ---'
if command -v get_repo_knowledge >/dev/null 2>&1; then
get_repo_knowledge </dev/null
else
printf '%s\n' 'get_repo_knowledge utility unavailable in shell'
fi
printf '%s\n' '--- GPU.c function and callers ---'
sed -n '1,310p' linux/GPU.c
printf '%s\n' '--- GPU.c symbols and call sites ---'
rg -n -C 3 'GPU_readProcessData|gpu_memory|/proc/.*/fdinfo|fdinfo' --glob '*.{c,h}' .Repository: htop-dev/htop
Length of output: 14693
🤖 get_repo_knowledge executed:
get_repo_knowledge htop-dev/htop /tmp/coderabbit-repo-knowledge/htop-dev-htop-29b6682e
Length of output: 2126
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- GPU.c focused source ---'
sed -n '1,310p' linux/GPU.c
printf '%s\n' '--- GPU symbols and call sites ---'
rg -n -C 3 'GPU_readProcessData|gpu_memory|/proc/.*/fdinfo|fdinfo' --glob '*.{c,h}' .Repository: htop-dev/htop
Length of output: 14609
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- caller context ---'
sed -n '1725,1790p' linux/LinuxProcessTable.c
printf '%s\n' '--- numeric limits and relevant includes ---'
rg -n -C 2 '`#include` <limits.h>|ULLONG_MAX|UINT64_MAX|GPU_readProcessData' --glob '*.{c,h}' .
printf '%s\n' '--- local DRM references ---'
rg -n -C 3 'drm-memory|drm-resident|drm-usage-stats|KiB|MiB' --glob '*' . | head -240Repository: htop-dev/htop
Length of output: 17496
🌐 Web query:
official Linux DRM usage stats fdinfo drm-memory drm-resident units value contract
💡 Result:
<source_evidence>
Citations:
- 1: https://kernel.org/doc/html/latest/gpu/drm-usage-stats.html
- 2: https://docs.kernel.org/gpu/drm-usage-stats.html
- 3: https://docs.kernel.org/6.7/gpu/drm-usage-stats.html
- 4: https://lists.freedesktop.org/archives/dri-devel/2024-November/478070.html
- 5: https://lists.openwall.net/linux-kernel/2024/11/01/1500
- 6: https://lkml.indiana.edu/hypermail/linux/kernel/2305.1/08011.html
- 7: https://docs.kernel.org/6.11/gpu/drm-usage-stats.html
Reject values that overflow the byte conversion.
When GPU monitoring is active, GPU_readProcessData() can parse a valid DRM fdinfo memory entry. The parser accepts any in-range unsigned integer, but it does not check the unit multiplication. A value above ULLONG_MAX / 1024 for KiB, or above ULLONG_MAX / 1048576 for MiB, can wrap bytesValue. The wrapped value is added to new_gpu_memory and stored in lp->gpu_memory.
Suggested fix
`#include` <errno.h>
+#include <limits.h>
`#include` <sys/types.h>
@@
unsigned long long int bytesValue;
+ unsigned long long int multiplier;
if (errno != 0)
continue;
else if (String_startsWith(endptr, "KiB"))
- bytesValue = value * 1024ULL;
+ multiplier = 1024ULL;
else if (String_startsWith(endptr, "MiB"))
- bytesValue = value * 1024ULL * 1024ULL;
+ multiplier = 1024ULL * 1024ULL;
else
continue;
+ if (value > ULLONG_MAX / multiplier)
+ continue;
+ bytesValue = value * multiplier;
+
Member
There was a problem hiding this comment.
Also take a look at the style guide regarding braces in if/elseif chains …
Also, this fails for values given in GiB or larger units.
There was a problem hiding this comment.
Use this command on a human-authored review finding. CodeRabbit findings already use the standard resolution workflow.
| unsigned long long int bytesValue; | ||
| if (errno != 0) | ||
| continue; | ||
| else if (String_startsWith(endptr, "KiB")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '170,245p' linux/GPU.c
rg -n 'String_startsWith|drm-resident|drm-memory|fdinfo' linux tests . | head -240Repository: htop-dev/htop
Length of output: 21777
Require an exact unit suffix.
String_startsWith(endptr, "KiB") and the equivalent MiB branch accept values such as 1 KiBjunk. When the entry is otherwise associated with a new client, GPU_readProcessData converts and adds that value to new_gpu_memory. Reject trailing data so malformed fdinfo entries cannot affect gpu_memory.
Proposed fix
- else if (String_startsWith(endptr, "KiB"))
+ else if (String_eq(endptr, "KiB"))
bytesValue = value * 1024ULL;
- else if (String_startsWith(endptr, "MiB"))
+ else if (String_eq(endptr, "MiB"))
bytesValue = value * 1024ULL * 1024ULL;
Member
|
Please clean up the commits (no merge commits) |
BenBE
added
Linux 🐧
Address upstream review feedback on the GPU_MEMORY column (PR htop-dev#2121): - CodeRabbit: a value above ULLONG_MAX / 1024 (KiB) or ULLONG_MAX / 1048576 (MiB) could wrap bytesValue during multiplication; check for overflow before multiplying instead. - CodeRabbit: String_startsWith(endptr, "KiB") accepted malformed suffixes like "KiBjunk"; require an exact unit match via String_eq() instead. - BenBE: the parser only recognized KiB/MiB and failed for GiB/TiB values, which drivers reporting large VRAM sizes can emit; add those units. The unit dispatch is pulled out into parse_drm_memory_value() so the overflow check and exact-match comparisons don't turn the call site into a multi-statement if/else-if chain. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Tgh778fwKS3MFYmr7wd3M
rajasekharponakala
force-pushed
the
main
branch
from
September 24, 2026 04:35
cb61770 to
7825cd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 90f50246-35ca-4101-be2a-c51facd717c9
📒 Files selected for processing (1)
linux/GPU.c
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| /* | ||
| * "drm-resident-<region>" is the current key for backing-store size; | ||
| * "drm-memory-<region>" is its deprecated alias (amdgpu only). A given | ||
| * driver emits only one of the two per region, so summing both is safe. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Deduplicate the deprecated alias by region.
When amdgpu emits drm-resident-vram and drm-memory-vram in one fdinfo entry, this branch adds both values. The kernel emits both keys from the same resident statistic. Client-ID deduplication does not apply within one entry, so GPU_MEMORY can report twice the actual value. Prefer drm-resident-<region> and use drm-memory-<region> only when that region has no resident key. (github.com)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
btop shows per-process GPU memory usage, which htop lacked. htop already
tracks per-process GPU engine time/utilization (GPU_TIME, GPU_PERCENT) by
parsing the generic DRM fdinfo stats (drm-engine-*), so extend the same
fdinfo parser to also sum drm-resident-/drm-memory- (the
deprecated amdgpu-only alias for resident) values, deduplicated per client
the same way engine time already is, and expose the result as a new
GPU_MEMORY column.