test: add cuInit contention benchmark for issue #1662 - #247
Conversation
Fork N processes that each call cuInit() under LD_PRELOAD, synchronized with a process-shared barrier so they all hit sem_postinit at once. Reports per-process cuInit() latency and blocked time for N=1..256. Validated on Tesla K80, driver 470.256.02, commit 52f33fc. Signed-off-by: chidwipak kuppani <chidwipak@gmail.com>
|
@chidwipak: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chidwipak 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 @chidwipak! It looks like this is your first PR to Project-HAMi/HAMi-core 🎉 |
📝 WalkthroughWalkthroughAdds a Linux benchmark for concurrent ChangesInitialization contention benchmark
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BenchmarkMain
participant SharedMemory
participant ChildProcess
participant CUDARuntime
BenchmarkMain->>SharedMemory: initialize barrier and result storage
BenchmarkMain->>ChildProcess: fork synchronized children
ChildProcess->>SharedMemory: wait at the process-shared barrier
ChildProcess->>CUDARuntime: call cuInit()
CUDARuntime-->>ChildProcess: return status
ChildProcess->>SharedMemory: record timing and resource metrics
BenchmarkMain->>SharedMemory: collect child results
BenchmarkMain-->>BenchmarkMain: print statistics and CSV output
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: 3
🤖 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/benchmark/bench_init_contention.c`:
- Around line 110-115: Update the benchmark startup synchronization around
shared->barrier so children first signal readiness, then the parent records
wall_start between synchronization phases, and finally the existing barrier
releases all children together. Reuse the same barrier for both readiness and
start phases, ensuring no child can enter cuInit() before wall-clock timing
begins.
- Around line 93-100: Update the percentile function’s rank calculation to use
nearest-rank semantics, computing the zero-based index as ceil(p/100 * n) minus
one. Preserve the existing empty-input handling and bounds clamping for
percentile values outside the valid range.
- Around line 205-214: Update the process-count parsing in the benchmark’s main
argument handling to use strtol() instead of atoi(), validate that the entire
argument is consumed and represents a positive integer within MAX_PROC_NUM, and
only then assign proc_num; retain the existing error message and failure return
for invalid input.
🪄 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: 19c5e8bf-cf04-47a4-be2d-a62a266922ad
📒 Files selected for processing (2)
test/benchmark/README.mdtest/benchmark/bench_init_contention.c
Fix three issues flagged by CodeRabbit: - Percentile: use ceil(p/100*n)-1 index, not floor; fixes off-by-one for p50 when N is even (the common case in our sweep) - Wall timer: record wall_start before releasing children via a second barrier phase, so no child can enter cuInit() before timing begins - Replace atoi() with strtol() to avoid undefined behaviour on overflow Signed-off-by: chidwipak kuppani <chidwipak@gmail.com>
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 `@test/benchmark/bench_init_contention.c`:
- Line 210: Update the process-count parsing near strtol in the benchmark
initialization code to use int32_t instead of long, while retaining the
end-pointer validation, range checks, and errno == ERANGE handling; use the
existing strtol/strtol_l flow or an equivalent fixed-width conversion and ensure
the parsed value is safely range-checked before assignment.
🪄 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: 0e3f891e-5bf6-4492-a187-2ec86854edc2
📒 Files selected for processing (1)
test/benchmark/bench_init_contention.c
|
|
||
| if (argc >= 2) { | ||
| char *end; | ||
| long val = strtol(argv[1], &end, 10); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file:"
fd -a 'bench_init_contention\.c$' . || true
file="$(fd 'bench_init_contention\.c$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo
echo "File: $file"
wc -l "$file"
echo
echo "Relevant lines 180-230:"
sed -n '180,230p' "$file" | cat -n -v
echo
echo "Search MAX_PROC_NUM / cpplint / lint config:"
rg -n "MAX_PROC_NUM|MAX\(|cpplint|lint|STRTO|strtol|strtoll" "$file" -C 2 || true
echo
echo "Repository files mentioning cpplint:"
rg -n "cpplint|lint" -S --glob '!vendor/**' --glob '!third_party/**' --glob '!build/**' . | head -n 80 || true
fi
echo
echo "Check repo local cpplint availability/options:"
(command -v cpplint >/dev/null 2>&1 && cpplint --version; true)Repository: Project-HAMi/HAMi-core
Length of output: 4169
🌐 Web query:
google style cpplint runtime/int64 long cpplint error
💡 Result:
The cpplint error "runtime/int" (often triggered by the use of long) occurs because the Google C++ Style Guide mandates the use of fixed-width integer types from the C++ standard library, such as int32_t or int64_t, rather than built-in C types like short, long, or long long [1][2]. This rule is enforced to ensure code portability, as the size of built-in types (specifically long) can vary significantly across different compilers and operating systems (e.g., 32-bit on Windows vs. 64-bit on many Unix-like systems) [3]. To resolve or handle this error, you have three primary options: 1. Replace the types: Refactor your code to use the appropriate sized integer types from (e.g., replace long with int64_t or int32_t as appropriate for your application) [2][4]. 2. Suppress the warning for specific lines: You can add a comment to the offending line to instruct cpplint to ignore it [5]: // NOLINTNEXTLINE(runtime/int) long my_variable; 3. Filter out the check: If you decide that this rule does not apply to your project, you can disable the warning globally by running cpplint with a filter flag [2][5]: cpplint --filter=-runtime/int your_file.cpp
Citations:
- 1: https://clang.llvm.org/extra/clang-tidy/checks/google/runtime-int.html
- 2: https://qedevelopment.co.uk/?p=459
- 3: https://stackoverflow.com/questions/18971732/what-is-the-difference-between-long-long-long-long-int-and-long-long-i
- 4: ignore cpplint runtime/int error ament/ament_lint#33
- 5: https://manpages.debian.org/unstable/cpplint/cpplint.1.en.html
Use a fixed-width integer for the process count parsing.
cpplint rejects long in runtime/int. Use int32_t with strtol/strtol_l or another fixed-width conversion; keep the end-pointer, range, and errno == ERANGE checks.
🧰 Tools
🪛 GitHub Check: cpplint
[failure] 210-210:
[cpplint] reported by reviewdog 🐶
Use int16_t/int64_t/etc, rather than the C type long [runtime/int] [4]
Raw Output:
test/benchmark/bench_init_contention.c:210: Use int16_t/int64_t/etc, rather than the C type long [runtime/int] [4]
🤖 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/benchmark/bench_init_contention.c` at line 210, Update the process-count
parsing near strtol in the benchmark initialization code to use int32_t instead
of long, while retaining the end-pointer validation, range checks, and errno ==
ERANGE handling; use the existing strtol/strtol_l flow or an equivalent
fixed-width conversion and ensure the parsed value is safely range-checked
before assignment.
Source: Linters/SAST tools
1 participant
What type of PR is this?
New test infrastructure (benchmark tool)
What this PR does / why we need it:
Adds
test/benchmark/bench_init_contention.candtest/benchmark/README.md— a measurement tool for the initialization contention described in Project-HAMi/HAMi#1662.The program forks N processes that each call
cuInit()underLD_PRELOAD=libvgpu.so, synchronized with a two-phase process-shared barrier. Phase 1 collects all processes as ready; the parent recordswall_startbetween the phases; Phase 2 releases all processes together so no child can entercuInit()before timing begins. It reports per-process latency (min, p50, mean, p95, p99, max), wall time, CPU time per process, and blocked time. A CSV line per run makes it easy to sweep over different process counts.cuInit()is timed because it callsensure_post_init()at its tail (src/libvgpu.c:945), which runspostInit()once per process viapthread_once.postInit()acquiressem_postinitthroughlock_postinit()and callsset_task_pid()under it.set_task_pid()runsnvmlInit(), scans running processes twice, and retains a primary CUDA context. Becausesem_postinitserializes this across every process on the node, timingcuInit()end-to-end captures the full contention cost.The file is picked up automatically by
test/CMakeLists.txt(itsGLOB_RECURSEcovers all*.cfiles undertest/). No build-system changes are needed.Which issue(s) this PR fixes:
Related to Project-HAMi/HAMi#1662
Special notes for your reviewer:
Tested on a 4-GPU node, Tesla K80, driver 470.256.02, CUDA 11.4 kernel driver, HAMi-core commit 52f33fc,
CUDA_VISIBLE_DEVICES=1:Wall time scales linearly with N. Per-process CPU stays flat at ~150 ms. Blocked time grows linearly. For N=256, the last process waits ~43 seconds, matching the "~60 s startup" report in the issue.
Note: PRs #228 and #229 (currently open) fix
set_task_pid()to use the correct device forcuDevicePrimaryCtxRetain. Neither changes thesem_postinitserialization that this benchmark measures.Note: PR #239 (also open) adds a broader benchmark suite under
bench/with its own Makefile. This PR adds a single focused tool undertest/benchmark/, which integrates with the existingtest/CMakeLists.txtand needs no separate build configuration.AI assistance: I used GitHub Copilot for repository navigation and review assistance while building and analyzing this benchmark. The program was designed, built, and run by me on the K80 machine above. Every number in this PR comes from an actual run on that hardware — nothing is estimated. I can explain every part of the implementation and take responsibility for addressing review feedback.
Does this PR introduce a user-facing change?:
No. This adds only
test/benchmark/files. Nosrc/changes.Summary by CodeRabbit
New Features
Documentation