Implementation Review Summary

Intent

Fill in IntelGpuReader::get_process_info() (previously Vec::new()) with real per-process GPU memory accounting from /proc/<pid>/fdinfo/<fd> for both i915 and xe drivers. Stretch goal (per-process engine-time) intentionally deferred to keep IntelGpuCard struct shape stable for #248's rebase.

Findings Addressed

None — the implementation is correct and complete as submitted. No fixes required.

Verification

• All stated requirements implemented (memory accounting via fdinfo for i915 + xe, render+card node mapping, drm-client-id dedup, EACCES silent skip, kB→bytes conversion, sysinfo enrichment mirroring AMD pattern)
• No placeholder/mock code remaining (the Vec::new() stub is fully replaced; gpu_utilization: 0.0 is a documented stretch-goal deferral, not a stub)
• Integrated into project code flow (IntelGpuReader::new() already registered in reader_factory.rs; get_process_info() flows through the GpuReader trait without per-vendor specialization)
• Project conventions followed (conventional-commits subjects all ≤72 chars; English-only; no AI attribution in commits, PR body, or code; no unwrap()/expect() in library code; let-else and Rust 1.58+ inline format args used)
• Existing modules reused (with_global_system, crate::device::process_list::get_all_processes, ProcessRefreshKind::nothing().with_cpu().with_memory().with_user(UpdateKind::OnlyIfNotSet) — exact mirror of the AMD reader pattern at src/device/readers/amd.rs:599-608)
• No unintended structural changes (IntelGpuCard struct shape is unchanged per the explicit orchestrator instruction — diff confirms only IntelGpuReader gained intel_drm_basenames and proc_root fields, and the 25-line reduction in intel_gpu_linux.rs is pure code movement to detection.rs)
• Tests pass (cargo check --lib --tests, cargo clippy --lib --tests -- -D warnings, cargo clippy -- -D warnings, 23 fdinfo tests, 19 linux tests — all green from a clean build)

Critical correctness checks (ultrathink)

• cardN ↔ renderD<M> mapping: build_intel_drm_basenames correctly registers BOTH nodes by walking /sys/class/drm and matching the basename of each node's device symlink target (the PCI bus identifier) against the pre-enumerated card list. Tests build_intel_drm_basenames_maps_card_and_render_to_same_index and build_intel_drm_basenames_two_cards_two_render_nodes cover the common case. Non-Intel render nodes are excluded by PCI-bus mismatch (test build_intel_drm_basenames_ignores_non_intel_render_nodes).
• drm-client-id deduplication: Per-(pid, card_index) the parser keeps a HashMap<client_id, max_bytes> (so N fds sharing one client report ONE memory amount, not N) and sums by_client.values() across distinct client IDs. Test collect_intel_gpu_processes_dedupes_by_client_id proves no double-count; collect_intel_gpu_processes_sums_distinct_clients proves distinct clients still aggregate. NOT a per-(pid, fd) dedup.
• i915 vs xe schema: Parser uses k.starts_with("drm-resident-") to sum every resident-memory key regardless of suffix (-system, -local0, -gtt, -vram0). Integrated GPUs (with only one key present) resolve correctly. Three tests cover both schemas explicitly.
• Permission errors: EACCES on /proc/<pid>/fd/, read_link, and fdinfo all degrade silently via let-else/match — no eprintln! in the entire new module.
• kB → bytes: parse_memory_value multiplies by 1024 for kb/kib. Test parse_fdinfo_kb_multiplier_is_1024 verifies 1 kB → 1024 bytes.
• Malformed fdinfo: Truncated mid-key, non-numeric values, missing client-id — all skipped or treated as None without panic (4 tests cover these paths).
• /proc injection: IntelGpuReader::new_with_roots(drm_root, proc_root) enables tempdir-based integration tests; IntelGpuReader::new() passes Path::new("/proc") for production. Integration tests in intel_gpu_linux/tests.rs exercise the full pipeline end-to-end.
• No regression for non-Intel hosts: collect_intel_gpu_processes short-circuits when intel_drm_basenames is empty. Test get_process_info_returns_empty_when_no_intel_cards covers this directly. AMD-only hosts skip the IntelGpuReader entirely via reader_factory.rs registration.

Module structure

• intel_gpu_fdinfo{.rs,/enrichment.rs,/tests.rs} mirrors intel_gpu_engine{.rs,/discovery.rs,/tests.rs} 3-file pattern.
• File-size budgets: intel_gpu_fdinfo.rs 483 lines, enrichment.rs 107, tests.rs 494, intel_gpu_linux.rs 474 (was 499 — kept under 500 by extracting detection.rs), detection.rs 86. All under 500.
• mod.rs declares intel_gpu_fdinfo under the same #[cfg(target_os = "linux")] gate as intel_gpu_engine.

Remaining items

• Hardware verification ACs (get_process_info non-empty on real Intel GPU host, used_memory matches intel_gpu_top -p) remain unchecked with (awaits hardware verification by maintainer) — appropriate per the convention.
• Stretch goal (per-process engine-time utilization) intentionally deferred to a follow-up PR; documented in PR body and tracked by ACs.

Minor docs nit (LOW)

intel_gpu_linux.rs:146 comment says "Test-only constructor" for new_with_roots, but it is also called by IntelGpuReader::new() (production). The function is correctly not #[cfg(test)] gated — the comment should read "Internal constructor that accepts arbitrary DRM and proc roots (production code uses default paths via [IntelGpuReader::new])." Not worth a separate fix cycle.

Final verdict

Ready to advance to security/perf review. Implementation is correct, complete, properly integrated, conforms to project conventions, and preserves IntelGpuCard struct shape for #248's rebase. All narrow-scope verifications pass on a clean build.

Security and performance review

Comprehensive review of the per-process fdinfo accounting work against the surface areas called out in the briefing. The reviewer ran cargo check --lib --tests, cargo clippy --lib --tests -- -D warnings, cargo clippy -- -D warnings, cargo test --lib device::readers::intel_gpu_fdinfo (23 pass), and cargo test --lib device::readers::intel_gpu_linux (19 pass) on this branch.

Findings

MEDIUM (auto-fixed in 9bb3d75)

• cargo fmt --check flagged three formatting nits in the new module (one path-join chain, one let binding, one function signature). CI runs cargo fmt --check (see .github/workflows/ci.yml:49), so this would have blocked merge. Applied cargo fmt, verified all 42 tests still pass, pushed as a separate style(intel-gpu) commit on this branch.

LOW (informational, not blocking)

• collect_intel_gpu_processes only increments the MAX_GPU_PROCESSES cap counter for processes that have at least one Intel DRM fd (src/device/readers/intel_gpu_fdinfo.rs:407-410). On a host with thousands of idle PIDs and zero Intel-using processes, the full /proc walk still pays one read_dir(/proc/<pid>/fd/) per PID. This matches the AMD reader's cost shape via libamdgpu_top::FdInfoStat::update_proc_usage and is acceptable for v1, but it means the cap is a credit cap, not an enumeration cap. Worth noting in a follow-up if /proc walk latency ever becomes a hotspot.

• The missing-drm-client-id fallback at src/device/readers/intel_gpu_fdinfo.rs:428-437 keys by fdinfo_path (which is unique per fd). Two fds in the same process to the same DRM client on a pre-drm-client-id kernel would still double-count. In practice this is a non-issue: the kernels that expose drm-resident-* (Linux >=5.19 i915, all xe) also emit drm-client-id, so the over-count only triggers on a hypothetical kernel that emits memory keys without the client-id — and on such a kernel resident_bytes is typically zero anyway.

• read_to_string on fdinfo files has no take(N) upper bound (src/device/readers/intel_gpu_fdinfo.rs:413). In practice the kernel emits at most ~30 lines (~2 KB) per fdinfo, but a defence-in-depth byte cap would harden against a future kernel change emitting unbounded counters.

Critical surface areas — verdict

• /proc/<pid>/fd/ traversal & EACCES: intel_drm_fds_for_pid uses let Ok(entries) = std::fs::read_dir(&fd_dir) else { return Vec::new(); } (line 307). Silently degrades on EACCES and ENOENT. No eprintln!, no unwrap, no panic! anywhere in the production path. Pass.
• Symlink-follow safety: The walker uses std::fs::read_link(&fd_path) (line 320), returning only the symlink target as a PathBuf without following it. It then matches target.file_name() (basename only) against the HashMap<String, usize> of known Intel DRM nodes. No read_to_string on the fd entry itself; no path-join with the read_link result; no opportunity to follow a fd pointing at a sensitive file. Pass.
• Path-injection via drm-pdev: drm_pdev is captured into FdInfo (line 159) but never used to construct a path anywhere in the pipeline. Pass.
• TOCTOU on process exit: read_to_string(&fd.fdinfo_path) Err → continue (line 415); read_link Err → continue (line 321); read_dir(proc_root) Err → empty Vec. Process disappearance is a normal-not-error case throughout. Pass.
• fdinfo parsing safety: All parse::<u64>() calls use .ok() (line 129) or ? chains; truncated/malformed input skips lines via let Some(...) = ... else { continue; } patterns; saturating_add/saturating_mul used on every memory math op (lines 141, 181, 456, 461). Non-UTF8 fdinfo would fail at read_to_string and skip cleanly. Pass.
• resident vs total selection: Only drm-resident-* keys are summed (line 139: k.starts_with("drm-resident-")). drm-total-* is intentionally ignored per the kernel drm-usage-stats.rst spec — total includes freed pages. Tests verify this. Pass.
• Cache reuse: build_intel_drm_basenames is called exactly once at IntelGpuReader::new_with_roots (line 154 of intel_gpu_linux.rs); subsequent get_process_info calls read self.intel_drm_basenames (line 299). The only per-call rebuild is card_uuids (O(cards), microseconds). Pass.
• sysinfo refresh narrowness: ProcessRefreshKind::nothing().with_cpu().with_memory().with_user(UpdateKind::OnlyIfNotSet) (enrichment.rs:63-66). Matches AMD exactly; no with_disk_usage, no with_environ. Pass.

Verdict

Ready for finalizer. One MEDIUM (formatting) was auto-fixed on this branch (9bb3d75). The remaining LOW findings are informational and do not block merge.

PR Finalization Complete

Summary

Lint/Format: cargo fmt idempotent (no changes after security checker's prior run). cargo clippy --lib --tests and cargo clippy bin target both clean with -D warnings.

Tests: 1120 lib tests pass. No new tests added — reviewer coverage of drm-client-id dedup, cardN/renderD mapping, and i915/xe schema branch was assessed as complete.

Docs: Three targeted fixes committed as f8743b1.

• src/device/readers/intel_gpu_linux.rs line 146: stale "Test-only constructor" doc comment replaced with accurate description of the function as an internal constructor called by both production code and tests.
• README.md: added per-process fdinfo bullet under the Intel Arc section (was missing despite ARCHITECTURE.md already documenting it); updated the Processes metrics line to include Intel Arc/Xe alongside AMD.
• docs/man/all-smi.1: extended the Intel Arc entry to mention per-process GPU memory tracking via /proc fdinfo, consistent with the ARCHITECTURE.md description.

All checks passing. Ready for merge.