add gpu free shared region concurrency regression test - #244
Conversation
Signed-off-by: iemAnshuman <asquare567@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: iemAnshuman 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 @iemAnshuman! It looks like this is your first PR to Project-HAMi/HAMi-core 🎉 |
📝 WalkthroughWalkthroughAdds optional semaphore sequence-point callbacks and a fork-based shared-region concurrency test. CMake builds the test with system libraries, registers it with CTest, sets a 90-second timeout, and enables testing. ChangesShared-region concurrency testing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Test as test_shared_region_concurrency
participant Workers as worker processes
participant Lock as lock_shrreg
participant Cache as shared-region cache
Test->>Workers: fork and release synchronization gates
Workers->>Lock: register shared region
Lock->>Workers: record semaphore acquisition sequence point
Workers->>Cache: write registration entries
Test->>Cache: validate live PID slots
Test->>Workers: reap processes and report metrics
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (2)
test/CMakeLists.txt (1)
15-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated
test_shared_region_concurrency.cpredicate.The same
MATCHEScheck is evaluated at line 15 and again at line 32. Folding the link step into the first branch keeps the special case in one place.🤖 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 `@test/CMakeLists.txt` around lines 15 - 37, Consolidate the duplicated test_shared_region_concurrency.c condition in the test target setup by moving its special target_link_libraries call into the existing first branch. Remove the later repeated predicate and retain the current CUDA-library linking in the else path.test/test_shared_region_concurrency.c (1)
69-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSequence marks are overwritten on every lock retry, so reported wait latency understates contention.
lock_shrregemitsSEQ_BEFORE_ACQUIRE_SEMLOCKandSEQ_ACQUIRE_SEMLOCK_OKon each loop iteration, including timed-out attempts. With 128+ workers contending, a worker that times out repeatedly reports only the final successful attempt, sowait_*_msinprint_resultscan look near-zero while the worker actually waited tens of seconds. Consider accumulating instead of overwriting (e.g., keep the firstacquire_begin_msand count attempts).♻️ Sketch: record first attempt + attempt count
case SEQ_BEFORE_ACQUIRE_SEMLOCK: - result->acquire_begin_ms = timestamp; + if (result->acquire_begin_ms <= 0.0) { + result->acquire_begin_ms = timestamp; + } + result->acquire_attempts++; break;🤖 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 `@test/test_shared_region_concurrency.c` around lines 69 - 84, Update the sequence handling around SEQ_BEFORE_ACQUIRE_SEMLOCK and SEQ_ACQUIRE_SEMLOCK_OK to preserve the worker’s initial acquisition timestamp across retries instead of overwriting it; track the number of acquisition attempts if the existing result structure and print_results reporting support it, so wait latency includes the full contention period while release and unlock marks retain their current 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 `@test/CMakeLists.txt`:
- Around line 34-37: Update the test CMake configuration around the CUDA link
branch to gate NVIDIA/CUDA libraries on successful CUDA discovery. Ensure
non-CUDA tests can configure without CUDA, either by applying these link options
only to CUDA-capable targets or by skipping the test subdirectory when CUDA is
unavailable, while preserving existing linking for CUDA-enabled tests.
- Around line 46-48: Increase the CTest TIMEOUT for shared_region_concurrency
above the test’s maximum internal wait budget, accounting for both timeout_ms
wait paths under the default SHRREG_TEST_TIMEOUT_MS value, so the harness can
report its diagnostic before CTest terminates it.
---
Nitpick comments:
In `@test/CMakeLists.txt`:
- Around line 15-37: Consolidate the duplicated test_shared_region_concurrency.c
condition in the test target setup by moving its special target_link_libraries
call into the existing first branch. Remove the later repeated predicate and
retain the current CUDA-library linking in the else path.
In `@test/test_shared_region_concurrency.c`:
- Around line 69-84: Update the sequence handling around
SEQ_BEFORE_ACQUIRE_SEMLOCK and SEQ_ACQUIRE_SEMLOCK_OK to preserve the worker’s
initial acquisition timestamp across retries instead of overwriting it; track
the number of acquisition attempts if the existing result structure and
print_results reporting support it, so wait latency includes the full contention
period while release and unlock marks retain their current 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: cd9c8f7e-7a3f-4dd2-a82d-3b5380d50ec4
📒 Files selected for processing (5)
CMakeLists.txtsrc/multiprocess/multiprocess_memory_limit.csrc/multiprocess/multiprocess_memory_limit.htest/CMakeLists.txttest/test_shared_region_concurrency.c
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
1 participant
What this PR does:
lock_shrreg()serializes every process that registers a slot in the shared region, andtry_create_shrreg()andinit_proc_slot_withlock()run behind it on every process'scuInit()path. Nothing in the tree exercises that under concurrency, so a regression in the semaphore protocol or in slot allocation only shows up on a machine with GPUs and a driver.test/test_shared_region_concurrency.cforks N workers (128 by default, 256 max viaSHRREG_TEST_WORKERS), releases them from a pipe barrier intoensure_initialized()together, and keeps them alive while the parent maps the real cache file. The parent then checks thatproc_numequals the worker count, that every worker PID sits in exactly one slot, and that each worker's sequence-point timestamps are present and in order, which shows it went throughlock_shrreg()rather than skipping it. Non-zero exit on deadlock (20s deadline, 90s ctest timeout outside it), a lost or duplicate slot, or a worker that dies.It links only
-lrtand-lpthread, no-lcuda,-lnvidia-mlor-lcudart, and calls no CUDA or NVML entry point, so it needs no device node, driver or GPU at runtime.multiprocess_memory_limit.hstill includescuda.h, so the CUDA headers the build already requires are still needed to compile. It compiles the productionmultiprocess_memory_limit.c, not a copy.Timings reuse the existing
SEQ_POINT_MARKhook: one new sequence point beforesem_timedwait(), plus a callback form of the macro that is only selected whenSHRREG_SEQUENCE_POINT_CALLBACKis defined. The default build is unchanged,SEQ_POINT_MARKstill expands to nothing, andlibvgpu.sois not affected.The latencies it prints are not assertions. No wall-time ceiling applies unless
SHRREG_TEST_MAX_WALL_MSis set, so a slow runner cannot fail it.Not a fix. It does not change initialization behavior and does not address the contention in Project-HAMi/HAMi#1662, and it passes before and after any change there. #239 by @om7057 measures that on real hardware; this is the GPU-free half.
Which issue(s) this PR fixes:
Refs Project-HAMi/HAMi#1662
Special notes for your reviewer:
Built and run on Rostam cluster, 128 CPUs, GCC 11.5.0, CUDA 12.9, driver 580.65.06. The node has two A100 80GB cards, which the test does not touch.
ctest found one matching test and passed it, 1/1 in 0.12 s. Ten consecutive 128-worker runs passed, 92.243 ms to 107.744 ms wall and 89.046 ms to 103.318 ms total p99. The 256-worker run passed at 355.600 ms wall, 348.926 ms total p99 and 335.880 ms semaphore-wait p99.
SHRREG_TEST_MAX_WALL_MS=1exits non-zero and reports the measured wall time, 100.088 ms on that run. Worker counts of 0 and 257 both exit 2.lddshows only libc and the ELF loader, and a traced 32-worker run touched no/dev/nvidia,libcuda,libnvidia-mlorlibcudart.One linker fix is included: the inherited
-rdynamickept unused GPU-facing functions alive despite section garbage collection, so the test target adds--no-export-dynamic.I used Claude Code while writing this test.
Does this PR introduce a user-facing change?: none
Summary by CodeRabbit
New Features
Tests