feat: add CARDWIRE_FORCE_DGPU to cardwire - #116
Conversation
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBy wizardry of explicit GPU mode values, application analysis now selects iGPU or dGPU behavior, daemon blocking APIs store those values, and smart-mode eBPF filtering uses process and inode maps to control GPU visibility. ChangesGPU mode visibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant spawn_exec_analyzer
participant pid_map
participant eBPF
participant cw_blocked_ino
Application->>spawn_exec_analyzer: start with environment
spawn_exec_analyzer->>pid_map: store selected GPU mode
eBPF->>cw_blocked_ino: read inode GPU mode
eBPF->>pid_map: read process GPU mode
eBPF-->>Application: allow, hide, or expose GPU directory entries
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/cardwire-daemon/src/interface/mode.rs (1)
124-140: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winA stale enchantment lingers and blinds the iGPU.
The default GPU only gets touched here when
mode == Modes::Smart(block_gpu(0)). Nothing ever callsunblock_gpu()on the default GPU for the other branches (Integrated/Hybrid). Peris_blocked_device(helpers.h) andis_inode_blocked(lib.rs), Integrated/Manual mode blocks purely on presence incw_blocked_ino, ignoring the stored value. So: switch to Smart mode (tags default GPU with value0), then switch to Integrated mode — the stale0entry is never cleared, and the default GPU (often the only usable one) gets fully denied.🔮 Banish the stale tracking entry on mode change
for gpu in gpu_list.values_mut() { if !gpu.device.is_default() { if mode == Modes::Integrated || mode == Modes::Smart { // Here we block the dGPU gpu.block_gpu(1).await?; } else { gpu.unblock_gpu().await?; } - }; - - // If we in smart mode and is the default gpu, push it to the blocked inode map - // but it wont get blocked, trust - if mode == Modes::Smart && gpu.device.is_default() { - gpu.block_gpu(0).await?; + } else if mode == Modes::Smart { + // push default gpu (iGPU) into the blocked inode map for tracking only + gpu.block_gpu(0).await?; + } else { + // clear any stale smart-mode tracking entry left from a prior session + gpu.unblock_gpu().await?; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cardwire-daemon/src/interface/mode.rs` around lines 124 - 140, Update the default-GPU handling in the mode loop so switching away from Smart removes its stale blocked-inode tracking entry by calling the appropriate unblock operation for non-Smart modes. Preserve the existing Smart behavior that records the default GPU with block value 0, and keep non-default GPU handling unchanged.crates/cardwire-daemon/src/interface/debug.rs (1)
134-157: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe dGPU-forcing charm fades after a hotplug refresh.
should_blockforModes::Smartonly covers the non-default GPU (!gpu.device.is_default()), sorefresh_gpunever re-castsblock_gpu(0)on the default GPU here. If a PCI list change (hotplug/eGPU) rebuildsGpuInterfaces with new inodes while in Smart mode, the default GPU's tracking entry fromset_modeis stale/missing, silently breakingCARDWIRE_FORCE_DGPU's iGPU-hiding logic until the mode is manually re-applied.🧙 Suggested reapplication of the tracking rune
if should_block { info!( "GPU {} should be blocked, re-applying block on hotplug", gpu.device.name() ); if let Err(e) = gpu.block_gpu(1).await { warn!( "failed to automatically re-block {}: {}", gpu.device.name(), e ); } + } else if mode == Modes::Smart && gpu.device.is_default() { + if let Err(e) = gpu.block_gpu(0).await { + warn!( + "failed to re-track default gpu {}: {}", + gpu.device.name(), + e + ); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cardwire-daemon/src/interface/debug.rs` around lines 134 - 157, Update the refresh logic around `should_block` and `gpu.block_gpu` so `Modes::Smart` also reapplies the default GPU’s required `block_gpu(0)` tracking state after hotplug rebuilds, while preserving non-default GPU blocking and existing Integrated, Hybrid, and Manual behavior. Ensure the Smart-mode path restores the force-dGPU/iGPU-hiding state for newly recreated interfaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cardwire-daemon/src/analyzer/dynamic_analysis.rs`:
- Around line 93-105: Refactor check_cardwire_force_dgpu and
check_cardwire_allow to use a shared environment-variable parsing helper
parameterized by the variable prefix, deriving the value offset from that prefix
instead of hardcoding 20. Preserve the existing Option<bool> behavior, and add
focused unit tests covering enabled, disabled, missing, and offset-sensitive
values.
In `@crates/cardwire-ebpf/src/c/bpf.c`:
- Around line 152-161: Introduce a shared always-inline helper, such as
should_skip_getdents_patch, that combines the is_pid_allowed check with the
cw_allowed_pid lookups and returns whether patching should be skipped. Replace
the duplicated allow_val logic in both getdents64 hook call sites with this
helper while preserving the existing is_smart guard and return behavior.
In `@crates/cardwire-ebpf/src/c/helpers.h`:
- Around line 216-260: Update the iGPU/dGPU visibility logic in the shown
getdents handling block to look up cw_allowed_pid by pid and, when absent, retry
with ppid. Use the resulting allow_map_value for the existing ALLOW/FORCE_DGPU
branches, preserving their current visibility behavior for both direct processes
and forked children.
---
Outside diff comments:
In `@crates/cardwire-daemon/src/interface/debug.rs`:
- Around line 134-157: Update the refresh logic around `should_block` and
`gpu.block_gpu` so `Modes::Smart` also reapplies the default GPU’s required
`block_gpu(0)` tracking state after hotplug rebuilds, while preserving
non-default GPU blocking and existing Integrated, Hybrid, and Manual behavior.
Ensure the Smart-mode path restores the force-dGPU/iGPU-hiding state for newly
recreated interfaces.
In `@crates/cardwire-daemon/src/interface/mode.rs`:
- Around line 124-140: Update the default-GPU handling in the mode loop so
switching away from Smart removes its stale blocked-inode tracking entry by
calling the appropriate unblock operation for non-Smart modes. Preserve the
existing Smart behavior that records the default GPU with block value 0, and
keep non-default GPU handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d919546d-9719-4d45-b0f7-15902bf7e842
📒 Files selected for processing (8)
crates/cardwire-daemon/src/analyzer/dynamic_analysis.rscrates/cardwire-daemon/src/analyzer/models.rscrates/cardwire-daemon/src/interface/debug.rscrates/cardwire-daemon/src/interface/gpu.rscrates/cardwire-daemon/src/interface/mode.rscrates/cardwire-ebpf/src/c/bpf.ccrates/cardwire-ebpf/src/c/helpers.hcrates/cardwire-ebpf/src/lib.rs
| __u8 *allow_val = | ||
| bpf_map_lookup_elem(&cw_allowed_pid, &pid); | ||
| if (!allow_val) { | ||
| allow_val = bpf_map_lookup_elem(&cw_allowed_pid, | ||
| &ppid); | ||
| } | ||
| // If force_dgpu is NOT enabled, we can safely skip patching | ||
| if (allow_val && *allow_val != 0) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Casting the same lookup spell twice, in two scrolls.
Both getdents64 hooks repeat: is_pid_allowed(pid,ppid) (which itself looks up cw_allowed_pid by pid/ppid) followed by an independent re-lookup of the same map to fetch the value. On a syscall hot path (getdents64 fires on every directory read), that's redundant map work performed twice per call site, copy-pasted verbatim across both functions.
♻️ Suggested shared helper
static __always_inline int should_skip_getdents_patch(__u32 pid, __u32 ppid)
{
if (!is_pid_allowed(pid, ppid))
return 0;
__u8 *allow_val = bpf_map_lookup_elem(&cw_allowed_pid, &pid);
if (!allow_val)
allow_val = bpf_map_lookup_elem(&cw_allowed_pid, &ppid);
// If force_dgpu is NOT enabled, we can safely skip patching
return allow_val && *allow_val != 0;
}Then both call sites collapse to:
if (is_smart() && should_skip_getdents_patch(pid, ppid)) {
return 0;
}Also applies to: 196-205
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/cardwire-ebpf/src/c/bpf.c` around lines 152 - 161, Introduce a shared
always-inline helper, such as should_skip_getdents_patch, that combines the
is_pid_allowed check with the cw_allowed_pid lookups and returns whether
patching should be skipped. Replace the duplicated allow_val logic in both
getdents64 hook call sites with this helper while preserving the existing
is_smart guard and return behavior.
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 16 minutes. |
1 participant
Description
Add a new ENV var named
CARWIRE_FORCE_DGPU, setting this env to 1 before launching a process would hide the iGPU from the process, if the program only sees the dGPU, it should always select itThis is only for smart mode, porting to manual mode would be a task for >0.12.0 and would require rewriting some parts of the ebpf code, i'm also considering rewriting it to rust.
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes