fix: make SM limiter step linear in SM count - #317
maymust-jonghyunlee wants to merge 1 commit into
Conversation
Remove the extra sm_num factor so the step-to-pool ratio stays diff / 81920 across device sizes Signed-off-by: jhlee <jonghyun.lee@maymust.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: maymust-jonghyunlee 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe utilization watcher now calculates the CUDA-core increment with one SM-count factor instead of two. ChangesUtilization correction
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to The utilization limiter now adjusts token allocation proportionally to SM count, preventing oversized correction steps on high-SM GPUs. The change is ready to merge. 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 |
|
Not like #286 LLM helped posting my replies. |
| abs(up_limit - user_current) < 5 ? 5 : abs(up_limit - user_current); | ||
| int64_t increment = | ||
| (int64_t)g_sm_num[device_id] * (int64_t)g_sm_num[device_id] * | ||
| (int64_t)g_sm_num[device_id] * |
There was a problem hiding this comment.
this also shrinks the step by sm_num on small cards. did convergence still hold on something like a 40-sm gpu?
There was a problem hiding this comment.
Since I don't have 40 sm gpu (maybe like 4080?) i tested with gtx 1080.
Measurements were taken on a gtx 1080 with 20 sm (Pascal, max_threads_per_sm=2048, pool size 1,310,720), using driver 580.173.02,CUDA_DEVICE_SM_LIMIT=50,GPU_CORE_UTILIZATION_POLICY=FORCE. Ran continuous matmul for 240 seconds using standalone LD_PRELOAD. Both were built from the same source tree with the same toolchain: main versus main with this patch applied. Watcher samples were collected approximately every 20 seconds.
On main, userutil was 46, 62, 9, 100, 17, 83, 95, 100, 100, 100, 100, 47, unstable. Later in the run, it stayed at 100 for roughly 80 seconds, while share alternated between 0 and approximately 95000. Average device utilization, sampled every 5 seconds with nvidia-smi, was 81%, well above the 50% target.
With the patch, userutil was 40, 41, 43, 55, 56, 68, 51, 67, 50, 43, 42. It was already within the target band at the first sample, 20 seconds into the run, and share remained stable between 95000 and 102000 Average device utilization was 49%, close to the 50% target.
First, English is not my native language, so I used AI to help with the translation.
Fixes #314
Removes one
sm_numfactor from the correction step indelta().The token pool is:
The correction step previously contained:
This caused the step-to-pool ratio to increase with the SM count. The ratio is now diff / 81920 regardless of device size.
This PR does not change g_total_cuda_cores in setspec(). Its int64_t conversion is covered by #303, and the two changes can be merged in either order.
On a 188-SM GPU, the previous correction step could refill the entire pool in a single 120 ms tick. This caused the limiter to alternate between saturation and throttling instead of settling at the configured cap. See #314 for logs and further analysis.
Test environment:
Two processes shared one GPU with caps of 30 and 70, running 4096 FP32 matrix multiplication for 240 seconds:
With one process capped at 10, main averaged 26.9% device utilization while userutil oscillated between 0 and 83. With this change, userutil converged within approximately 20 seconds and remained between 9 and 13, averaging 10.9%.
Summary by CodeRabbit