fix: avoid stale process slots after compaction - #242
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AnatoliyKizyulya The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @AnatoliyKizyulya! It looks like this is your first PR to Project-HAMi/HAMi-core 🎉 |
|
Warning Review limit reached
Next review available in: 41 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesProcess Slot Safety and Waiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/multiprocess/multiprocess_memory_limit.c`:
- Around line 69-103: Synchronize slot lifetime with lock-free readers by
changing get_current_proc_slot to return a pinned/read-guarded slot handle
rather than a raw pointer, while synchronizing access to region_info.my_slot. In
src/multiprocess/multiprocess_memory_limit.c lines 105-109, 485-491, 560-566,
and 1386-1390, acquire the guard and hold it through the respective status
read/store or accounting update, then release it. At lines 725-742, 932-937, and
951-956, perform slot retirement and compaction under the same reader-drain
protocol, retiring source slots only after active readers have exited.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9350ed2-ba7e-45e5-89d8-5d7b5a55483e
📒 Files selected for processing (3)
src/include/memory_limit.hsrc/multiprocess/multiprocess_memory_limit.csrc/multiprocess/multiprocess_memory_limit.h
960cbe2 to
1641a52
Compare
1641a52 to
9d64a0b
Compare
Validate cached process slots against the active shared-region range and clear the source slot after it is moved during compaction. Avoid waiting on process status when SM limiting is disabled. Signed-off-by: Kizyulya Anatoliy <tolik8621@list.ru>
9d64a0b to
025f456
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/multiprocess/multiprocess_memory_limit.c (2)
69-103: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStale cache never gets refreshed after cross-process compaction.
When another process compacts this slot away, the fallback loop deliberately leaves
region_info.my_slotunrefreshed "Do not rewrite my_slot here: other threads may still be using the stale cache." That means every subsequent call from this process (a hot path used by memory accounting and status checks) pays anO(proc_num)linear scan for the remainder of the process's life, since nothing else ever repairs the cache for this process. Consider refreshing the cache with an atomic pointer store (atomic_store_explicitonregion_info.my_slotas_Atomic(shrreg_proc_slot_t*)) instead of leaving it permanently stale — that resolves both the thread-safety concern noted in the comment and the perpetual fallback cost.🤖 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 `@src/multiprocess/multiprocess_memory_limit.c` around lines 69 - 103, Update get_current_proc_slot so that, after the fallback scan finds the current process slot, it refreshes region_info.my_slot via an atomic pointer store using the field’s _Atomic(shrreg_proc_slot_t*) type. Preserve the existing validation and return behavior while ensuring subsequent calls avoid repeatedly scanning proc_num entries.
932-937: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated compaction block — extract a helper.
The 5-line sequence (
copy_proc_slot_atomic+region_info.my_slotfixup +clear_proc_slot_atomic) is repeated verbatim for the PID=0 branch and the dead-proc branch. Extracting it reduces the risk of the two copies drifting apart on a future edit (e.g., the seqlock-protocol fix above would otherwise need to be applied twice).♻️ Suggested helper
+static inline void compact_last_slot_into(shared_region_t* region, int target_slot) { + shrreg_proc_slot_t* last_slot = ®ion->procs[region->proc_num]; + copy_proc_slot_atomic(®ion->procs[target_slot], last_slot); + if (region_info.my_slot != NULL && region_info.my_slot == last_slot) { + region_info.my_slot = ®ion->procs[target_slot]; + } + clear_proc_slot_atomic(last_slot); +}Then both branches reduce to
region->proc_num--; compact_last_slot_into(region, slot); __sync_synchronize();.Also applies to: 951-956
🤖 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 `@src/multiprocess/multiprocess_memory_limit.c` around lines 932 - 937, Extract the duplicated compaction sequence into a helper such as compact_last_slot_into, preserving the copy_proc_slot_atomic call, region_info.my_slot fixup, and clear_proc_slot_atomic call. Replace the corresponding PID=0 and dead-proc branches with region->proc_num--, the helper call, and the existing synchronization, ensuring both paths share identical compaction behavior.
🤖 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 `@src/multiprocess/multiprocess_memory_limit.c`:
- Around line 725-743: Update clear_proc_slot_atomic to follow the seqlock
odd/even protocol used by add_gpu_device_memory_usage and
rm_gpu_device_memory_usage: mark slot->seqlock odd before clearing pid, status,
used[], device_util[], and related fields, then publish an even value only after
all clears complete. Do not expose the slot as stable while its fields are being
reset.
---
Nitpick comments:
In `@src/multiprocess/multiprocess_memory_limit.c`:
- Around line 69-103: Update get_current_proc_slot so that, after the fallback
scan finds the current process slot, it refreshes region_info.my_slot via an
atomic pointer store using the field’s _Atomic(shrreg_proc_slot_t*) type.
Preserve the existing validation and return behavior while ensuring subsequent
calls avoid repeatedly scanning proc_num entries.
- Around line 932-937: Extract the duplicated compaction sequence into a helper
such as compact_last_slot_into, preserving the copy_proc_slot_atomic call,
region_info.my_slot fixup, and clear_proc_slot_atomic call. Replace the
corresponding PID=0 and dead-proc branches with region->proc_num--, the helper
call, and the existing synchronization, ensuring both paths share identical
compaction behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 414a2d0b-c072-4a4f-915d-7f1f2e90831f
📒 Files selected for processing (3)
src/include/memory_limit.hsrc/multiprocess/multiprocess_memory_limit.csrc/multiprocess/multiprocess_memory_limit.h
| static inline void clear_proc_slot_atomic(shrreg_proc_slot_t* slot) { | ||
| atomic_store_explicit(&slot->seqlock, 0, memory_order_relaxed); | ||
| atomic_store_explicit(&slot->pid, 0, memory_order_release); | ||
| atomic_store_explicit(&slot->hostpid, 0, memory_order_relaxed); | ||
| atomic_store_explicit(&slot->status, 0, memory_order_release); | ||
|
|
||
| for (int dev = 0; dev < CUDA_DEVICE_MAX_COUNT; dev++) { | ||
| atomic_store_explicit(&slot->used[dev].total, 0, memory_order_relaxed); | ||
| atomic_store_explicit(&slot->used[dev].context_size, 0, memory_order_relaxed); | ||
| atomic_store_explicit(&slot->used[dev].module_size, 0, memory_order_relaxed); | ||
| atomic_store_explicit(&slot->used[dev].data_size, 0, memory_order_relaxed); | ||
| atomic_store_explicit(&slot->used[dev].offset, 0, memory_order_relaxed); | ||
| atomic_store_explicit(&slot->device_util[dev].dec_util, 0, memory_order_relaxed); | ||
| atomic_store_explicit(&slot->device_util[dev].enc_util, 0, memory_order_relaxed); | ||
| atomic_store_explicit(&slot->device_util[dev].sm_util, 0, memory_order_relaxed); | ||
| atomic_store_explicit(&slot->monitorused[dev], 0, memory_order_relaxed); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
clear_proc_slot_atomic breaks the seqlock invariant used elsewhere.
add_gpu_device_memory_usage/rm_gpu_device_memory_usage bump seqlock to odd before mutating fields and back to even after, so readers can detect torn reads. Here seqlock is set to 0 (an even/"stable" value) before the field clears run, so a concurrent seqlock reader sees a stable-looking counter while pid, status, and used[]/device_util[] are being zeroed underneath it — a torn read with no way to detect it.
🔒 Follow the same odd/even protocol during clear
static inline void clear_proc_slot_atomic(shrreg_proc_slot_t* slot) {
- atomic_store_explicit(&slot->seqlock, 0, memory_order_relaxed);
+ atomic_fetch_add_explicit(&slot->seqlock, 1, memory_order_release); // odd: write in progress
atomic_store_explicit(&slot->pid, 0, memory_order_release);
atomic_store_explicit(&slot->hostpid, 0, memory_order_relaxed);
atomic_store_explicit(&slot->status, 0, memory_order_release);
@@
}
+ atomic_fetch_add_explicit(&slot->seqlock, 1, memory_order_release); // even: write complete
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static inline void clear_proc_slot_atomic(shrreg_proc_slot_t* slot) { | |
| atomic_store_explicit(&slot->seqlock, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->pid, 0, memory_order_release); | |
| atomic_store_explicit(&slot->hostpid, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->status, 0, memory_order_release); | |
| for (int dev = 0; dev < CUDA_DEVICE_MAX_COUNT; dev++) { | |
| atomic_store_explicit(&slot->used[dev].total, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->used[dev].context_size, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->used[dev].module_size, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->used[dev].data_size, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->used[dev].offset, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->device_util[dev].dec_util, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->device_util[dev].enc_util, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->device_util[dev].sm_util, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->monitorused[dev], 0, memory_order_relaxed); | |
| } | |
| } | |
| static inline void clear_proc_slot_atomic(shrreg_proc_slot_t* slot) { | |
| atomic_fetch_add_explicit(&slot->seqlock, 1, memory_order_release); // odd: write in progress | |
| atomic_store_explicit(&slot->pid, 0, memory_order_release); | |
| atomic_store_explicit(&slot->hostpid, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->status, 0, memory_order_release); | |
| for (int dev = 0; dev < CUDA_DEVICE_MAX_COUNT; dev++) { | |
| atomic_store_explicit(&slot->used[dev].total, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->used[dev].context_size, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->used[dev].module_size, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->used[dev].data_size, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->used[dev].offset, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->device_util[dev].dec_util, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->device_util[dev].enc_util, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->device_util[dev].sm_util, 0, memory_order_relaxed); | |
| atomic_store_explicit(&slot->monitorused[dev], 0, memory_order_relaxed); | |
| } | |
| atomic_fetch_add_explicit(&slot->seqlock, 1, memory_order_release); // even: write complete | |
| } |
🤖 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 `@src/multiprocess/multiprocess_memory_limit.c` around lines 725 - 743, Update
clear_proc_slot_atomic to follow the seqlock odd/even protocol used by
add_gpu_device_memory_usage and rm_gpu_device_memory_usage: mark slot->seqlock
odd before clearing pid, status, used[], device_util[], and related fields, then
publish an even value only after all clears complete. Do not expose the slot as
stable while its fields are being reset.
1 participant
Fixes #241
Summary
Fix stale cached process slots after shared-region compaction and avoid
unnecessary status waiting when SM limiting is disabled.
Changes
Validation
driver 580.159.03.
CUDA_DEVICE_SM_LIMIT=0andCUDA_DEVICE_SM_LIMIT=50.AI assistance was used for drafting; the change and validation results were reviewed by the author.
Summary by CodeRabbit