fix: avoid stale process slots after compaction - #242
Conversation
|
Welcome @AnatoliyKizyulya! It looks like this is your first PR to Project-HAMi/HAMi-core 🎉 |
📝 WalkthroughWalkthroughThe change validates cached process slots after shared-region compaction, updates memory and status paths to use valid slots, clears moved slots atomically, and gates readiness waiting on enabled per-device SM limits. ChangesProcess Slot Safety and Waiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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
|
@coderabbitai review |
|
|
This patch also resolves the issue of running models on vLLM 0.27.1 with TP=2. It was tested on an H200 GPU using driver version 580.178.04. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 932-937: Fix compaction ordering in
src/multiprocess/multiprocess_memory_limit.c#L932-L937 and `#L951-L956`: in both
cleanup branches, compute the last index without changing region->proc_num, copy
the last live slot, update region_info.my_slot when needed, publish the reduced
proc_num with a release store, then clear the source slot.
- Around line 1386-1390: Update wait_status_self so the no-slot path returns an
explicit result that ENSURE_RUNNING can distinguish from status mismatch, and
adjust the ENSURE_RUNNING loop to retry or handle that error rather than
treating -1 as success. Preserve the existing status comparison behavior when
get_current_proc_slot returns a valid slot.
🪄 Autofix
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: 27020614-18f7-4c03-8f29-de98c3e400ee
📒 Files selected for processing (1)
src/multiprocess/multiprocess_memory_limit.c
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kizyulya Anatoliy <tolik8621@list.ru>
87385ce to
48fd86b
Compare
|
@archlitchi could you please review this PR? |
|
I hit the same issue and used an LLM to fix it. Here are the changes — self-tested and working. FYI. Also: why need to move the slot? |
Good question: moving the slot is not fundamentally required, and this PR did not introduce that behavior. clear_proc_slot_nolock() already used swap-with-last compaction. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AnatoliyKizyulya, archlitchi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
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