fix: sweep dead process slots only when slots are scarce - #253
fix: sweep dead process slots only when slots are scarce#253keshav9926 wants to merge 3 commits into
Conversation
init_proc_slot_withlock() swept every occupied slot for liveness on every join. The sweep reads /proc/<pid>/stat once per slot and runs with the region lock held, so N processes starting together perform O(N^2) serialised filesystem work. On the harness from Project-HAMi#252, init p50 grows from 0.35ms to 6.06ms going from 1 to 64 concurrent processes. Reclaiming a slot whose process already died is not needed for a join to be correct: oom_check() sweeps before it reports OOM, which is where a stale slot actually changes an outcome. Sweep on join only once occupancy reaches three quarters of the table, and do it before the capacity check so a table filled with dead slots is recovered instead of being fatal. Slots that exit cleanup already marked with PID 0 are still compacted on every join; that path reads no files. With this change init p50 at 64 concurrent processes is 1.11ms. Signed-off-by: keshav9926 <kkakani160@gmail.com>
Covers both halves of the join-path contract: below the sweep threshold a join leaves slots held by dead processes in place, and at the threshold a join reclaims them, so repeated join-and-die cycles cannot grow the table without bound. The test is GPU-free and builds against the production shared-region sources the same way test_postinit_owner_death does, with a small SHARED_REGION_SWEEP_THRESHOLD so the reclaim path is reachable without spawning 768 processes. Signed-off-by: keshav9926 <kkakani160@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: keshav9926 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 @keshav9926! It looks like this is your first PR to Project-HAMi/HAMi-core 🎉 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe shared-region process-slot logic now performs configurable threshold-based dead-slot sweeping. A GPU-free regression test validates slot retention below the threshold and reclamation at the threshold. ChangesProcess-slot reclamation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 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.h`:
- Around line 43-48: Validate SHARED_REGION_SWEEP_THRESHOLD after its default or
override is defined, rejecting values below 1 or above
SHARED_REGION_MAX_PROCESS_NUM while preserving the valid range. Ensure the
existing zero-threshold behavior is handled according to the intended
sweep-on-every-join semantics in init_proc_slot_withlock().
🪄 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: 4ec2fc95-c803-4727-8fee-dbf8e39df331
📒 Files selected for processing (4)
src/multiprocess/multiprocess_memory_limit.csrc/multiprocess/multiprocess_memory_limit.htest/CMakeLists.txttest/test_proc_slot_reclaim.c
| // Slot-table occupancy at which joining a process performs a full liveness | ||
| // sweep. See init_proc_slot_withlock(). Overridable at build time so the | ||
| // regression test can reach the sweep without spawning 768 processes. | ||
| #ifndef SHARED_REGION_SWEEP_THRESHOLD | ||
| #define SHARED_REGION_SWEEP_THRESHOLD ((SHARED_REGION_MAX_PROCESS_NUM * 3) / 4) | ||
| #endif |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the sweep-threshold range.
Reject overrides below 1 or above SHARED_REGION_MAX_PROCESS_NUM. A value of 0 restores a sweep on every join. A value above capacity makes the full-table check exit before dead slots are reclaimed.
Proposed fix
`#ifndef` SHARED_REGION_SWEEP_THRESHOLD
`#define` SHARED_REGION_SWEEP_THRESHOLD ((SHARED_REGION_MAX_PROCESS_NUM * 3) / 4)
`#endif`
+
+#if SHARED_REGION_SWEEP_THRESHOLD < 1 || \
+ SHARED_REGION_SWEEP_THRESHOLD > SHARED_REGION_MAX_PROCESS_NUM
+ `#error` "SHARED_REGION_SWEEP_THRESHOLD must be within process-slot capacity"
+#endif📝 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.
| // Slot-table occupancy at which joining a process performs a full liveness | |
| // sweep. See init_proc_slot_withlock(). Overridable at build time so the | |
| // regression test can reach the sweep without spawning 768 processes. | |
| #ifndef SHARED_REGION_SWEEP_THRESHOLD | |
| #define SHARED_REGION_SWEEP_THRESHOLD ((SHARED_REGION_MAX_PROCESS_NUM * 3) / 4) | |
| #endif | |
| // Slot-table occupancy at which joining a process performs a full liveness | |
| // sweep. See init_proc_slot_withlock(). Overridable at build time so the | |
| // regression test can reach the sweep without spawning 768 processes. | |
| `#ifndef` SHARED_REGION_SWEEP_THRESHOLD | |
| `#define` SHARED_REGION_SWEEP_THRESHOLD ((SHARED_REGION_MAX_PROCESS_NUM * 3) / 4) | |
| `#endif` | |
| `#if` SHARED_REGION_SWEEP_THRESHOLD < 1 || \ | |
| SHARED_REGION_SWEEP_THRESHOLD > SHARED_REGION_MAX_PROCESS_NUM | |
| `#error` "SHARED_REGION_SWEEP_THRESHOLD must be within process-slot capacity" | |
| `#endif` |
🤖 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.h` around lines 43 - 48, Validate
SHARED_REGION_SWEEP_THRESHOLD after its default or override is defined,
rejecting values below 1 or above SHARED_REGION_MAX_PROCESS_NUM while preserving
the valid range. Ensure the existing zero-threshold behavior is handled
according to the intended sweep-on-every-join semantics in
init_proc_slot_withlock().
An override above SHARED_REGION_MAX_PROCESS_NUM would keep the sweep from ever running, so a table full of dead slots would reach the capacity check and exit -- the case this branch set out to make recoverable. Zero would sweep on every join and bring back the cost this branch removes. Catch both at compile time, since the override only exists for the regression test. Signed-off-by: keshav9926 <kkakani160@gmail.com>
1 participant
Fixes the O(N²) join path measured in #252.
What the problem is
init_proc_slot_withlock()callsclear_proc_slot_nolock(1)on every join. That sweep walks every occupied slot and callsproc_alive(), which is anfopen("/proc/<pid>/stat")+ read + close per slot (src/include/process_utils.h:17). All of it runs underlock_shrreg(), the single region-wide semaphore.So the Nth process to join does O(N) filesystem syscalls while holding a lock every other joining process needs. Across N processes starting together that is O(N²) serialised work — the shape a TP=N inference job or a pod whose containers all start at once produces.
What this changes
A join now sweeps for liveness only once occupancy reaches three quarters of the table.
Why that is safe: reclaiming a slot whose process died is not needed for a join to be correct. It matters in two places, and both are still covered.
get_gpu_memory_usage(), which could cause a spurious OOM.oom_check()already callsclear_proc_slot_nolock(1)and retries before it reports OOM (src/allocator/allocator.c:54), so the reclaim still happens exactly where a stale slot changes an outcome.proc_num >= SHARED_REGION_MAX_PROCESS_NUMcapacity check instead of after the insert, so a table filled with dead slots is recovered rather than hittingexit_withlock(-1). That case used to be fatal.Slots that exit cleanup already marked with PID 0 are still compacted on every join. That path reads no files, so it costs nothing.
Measurements
Harness from #252 — fork N children, hold them at a shared start barrier, release them together, time exactly one
ensure_initialized()per child; the region is deleted before every round so each round measures cold first-touch: https://gist.github.com/keshav9926/9e8f4c29eebd104ba02891e4733b9dfbBoth arms were built from this tree, one with the patch and one without, and run interleaved — base, fix, base, fix — for 6 iterations of 10 repeats each, so thermal drift and background load hit both arms equally. Values below are the median across the 6 iterations.
Before, p50 grows 17× from 1 to 64 processes. After, it grows 4×, and the gap widens with process count — which is what you would expect if the removed term was the quadratic one. The remaining growth is the
lockfintry_create_shrreg()and the semaphore itself; this PR does not touch either.wall_ms,init_p95andinit_maxare dominated by fork/exit scheduling noise on this hardware and I would not read anything into them.Test
test/test_proc_slot_reclaim.c— GPU-free, built against the production shared-region sources the same waytest_postinit_owner_deathis, and registered with ctest.It pins both halves of the new contract:
The target is compiled with
SHARED_REGION_SWEEP_THRESHOLD=8so the reclaim path is reachable without spawning 768 processes. The production constant is unchanged; it is now#ifndef-guarded only so the test can override it.Verified red/green: against
mainwithout the fix the test fails (join below the sweep threshold changed the table: expected 4 occupied slots, saw 2); with the fix it passes 5/5 consecutive runs.What was tested, and what wasn't
/usr/lib/wsl/lib./procand file-lock behaviour under WSL2 may differ from bare metal. Happy to re-run anywhere if someone has a node available.ensure_initialized()is timed. No CUDA context creation, no real workload.Trade-off worth flagging
Between sweeps, slots held by dead processes stay in the table, so
proc_numand the usage totals derived from it can be stale for longer than before.oom_check()covers the case where that changes an allocation outcome. If you would rather also bound the staleness in wall-clock terms, a "sweep if the last one was more than N seconds ago" rule would do it, but that needs a timestamp in the shared region and therefore a layout change — happy to do it that way instead if you prefer.(Disclosure: I use AI assistance in my workflow. The measurements, the code reading and the reasoning above are my own, and I'm happy to walk through any part of it.)
Summary by CodeRabbit
Bug Fixes
Tests