fix: use atomic load/store for device limit/sm_limit in shared region - #238
fix: use atomic load/store for device limit/sm_limit in shared region#238om7057 wants to merge 3 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: om7057 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 @om7057! 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)
📝 WalkthroughWalkthroughShared-region memory and SM limit arrays now use atomic fields. Initialization and runtime access use atomic operations. Limit APIs and ChangesShared-region limit synchronization
Estimated code review effort: 2 (Simple) | ~10 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 |
453ca60 to
b0ea170
Compare
DeviceMemoryLimit, SetDeviceMemoryLimit and SetDeviceSmLimit in
pkg/monitor/nvidia/{v0,v1} read and write limit/smLimit in
sharedRegionT with plain Go assignments. These are the same
shared-memory words HAMi-core's shared_region_t.limit/sm_limit map
onto, and the C side already guarantees every other field in that
struct is atomic (see the companion fix in
Project-HAMi/HAMi-core#238). These two were the only fields left as
plain reads/writes on either side.
Switches all three methods to sync/atomic.LoadUint64/StoreUint64 on
the same words. No change to sharedRegionT's field types or layout,
so the wire format is unaffected. Existing unit tests in
pkg/monitor/nvidia/{v0,v1} pass unmodified under -race.
![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`:
- Line 1255: Update the device-limit accessor functions around the
invalid-device checks at lines 1238, 1250, 1258, and 1268 to return their
documented error or sentinel immediately when dev is negative or dev >=
CUDA_DEVICE_MAX_COUNT. Ensure each invalid path exits before accessing
shared_region->sm_limit[dev] or limit[dev], while preserving existing logging
and valid-device 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: 788ec018-c7d0-4e90-8d55-30d8890677cf
📒 Files selected for processing (2)
src/multiprocess/multiprocess_memory_limit.csrc/multiprocess/multiprocess_memory_limit.h
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 1124-1127: Update the shared-region initialization around
do_init_device_memory_limits and do_init_device_sm_limits so region->limit and
region->sm_limit are written using atomic stores rather than casts to uint64_t*.
Preserve the existing helpers for local copies, and add or use atomic-capable
initialization helpers for the shared arrays.
🪄 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: 62d96b9e-b29a-4d96-96a1-710756f392dd
📒 Files selected for processing (2)
src/multiprocess/multiprocess_memory_limit.csrc/multiprocess/multiprocess_memory_limit.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/multiprocess/multiprocess_memory_limit.h
b0ea170 to
b83f7c7
Compare
DeviceMemoryLimit, SetDeviceMemoryLimit and SetDeviceSmLimit in
pkg/monitor/nvidia/{v0,v1} read and write limit/smLimit in
sharedRegionT with plain Go assignments. These are the same
shared-memory words HAMi-core's shared_region_t.limit/sm_limit map
onto, and the C side already guarantees every other field in that
struct is atomic (see the companion fix in
Project-HAMi/HAMi-core#238). These two were the only fields left as
plain reads/writes on either side.
Switches all three methods to sync/atomic.LoadUint64/StoreUint64 on
the same words. No change to sharedRegionT's field types or layout,
so the wire format is unaffected. Existing unit tests in
pkg/monitor/nvidia/{v0,v1} pass unmodified under -race.
Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
|
Fixed, added early returns for the illegal-dev path in all four functions. |
context_size, hostpid, proc_num, status and last_kernel_time were all made _Atomic in 24a0f49, but limit and sm_limit were missed. Both are read on the CUDA allocation hot path (get_current_device_memory_limit, called from every hooked cuMemAlloc) and are exposed through public, tested setters on both this side and the Go monitor side, so a plain uint64_t here does not match the atomicity already guaranteed for the rest of shared_region_t. _Atomic uint64_t has the same size and representation as uint64_t on the supported targets, so this does not change the struct layout: I verified sizeof(shared_region_t) and offsetof() for every field are identical before and after (2008952 / 1600 / 1728 / 1856 / 8), so the on-disk cudevshr.cache format and the Go-side mirror struct are unaffected. The two callers that fill the arrays at shared-region creation (do_init_device_memory_limits/do_init_device_sm_limits) still take a plain uint64_t*, so a cast is added there; that path already runs under the region's file lock before the region is published, so no new synchronization is needed there. Also fixes four sites in the same functions where an illegal dev falls through to the array access instead of returning after the LOG_ERROR (set_current_device_sm_limit_scale, get_current_device_sm_limit, set_current_device_memory_limit, get_current_device_memory_limit). The int-returning functions now return -1 on an out-of-range dev, matching the -1 convention already used elsewhere in this file. get_current_device_memory_limit returns 0 instead, since it feeds the allocation limit check directly and UINT64_MAX would silently remove the limit for an invalid dev where 0 fails closed. do_init_device_memory_limits/do_init_device_sm_limits still fill their array via plain non-atomic stores (arr[i] = ...), so writing region->limit/sm_limit through them via a uint64_t* cast performed a non-atomic store into an _Atomic-qualified object, which is a type violation independent of whether it happens to be safe on current targets. Reusing the helper into a local uint64_t array and copying that into the region with atomic_store_explicit avoids the cast while keeping do_init_device_memory_limits/do_init_device_sm_limits themselves unchanged, so nothing else that calls them (the already-initialized comparison branch just below) is affected. memory_order_relaxed is enough here: the atomic_thread_fence(release) a few lines down already publishes everything written before it, the same reasoning already used for sm_init_flag/utilization_switch/ recent_kernel/proc_num in this same block. Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
b83f7c7 to
0435096
Compare
|
Fixed. Kept do_init_device_memory_limits/do_init_device_sm_limits as-is for the local-array case (used by the already-initialized comparison branch below), and now compute into a local array then copy into the region with atomic_store_explicit, so nothing writes through a plain uint64_t* into the _Atomic fields anymore. |
|
@archlitchi @chaunceyjiang could you please check this PR? |
|
@om7057 these returns fix more than the atomics gap: before, an illegal dev logged and then indexed the 16 entry arrays anyway, so this also closes an OOB, worth a body sentence since the generated summary says input validation and return behavior are unchanged. also cuDeviceTotalMem_v2() just does *bytes = get_current_device_memory_limt(dev), so the new 0 sentinel comes back as zero bytes with CUDA_SUCCESS for a bogus dev, memory safe but not error equivalent |
|
also #232 rewrites and renames the same 'set_current_device_sm_limit_scale' hunk with the identical return -1 guard, and it already has lgtm, so whichever lands second takes a conflict, worth agreeing order with @KaminariOS. also #239's abi_check asserts these exact sizeof/offsetof values, running it against this head and pasting the output would make the layout section machine checked |
|
These two fields ( That said, making them |
… dev get_current_device_memory_limit() returns 0 for an out-of-range dev to avoid the out-of-bounds array read this PR is fixing elsewhere, but 0 is also the existing sentinel this codebase uses for "no memory limit configured" (do_init_device_memory_limits leaves an entry at 0 when neither the per-device nor fallback env var is set, and oom_check, cuMemGetInfo_v2, and the NVML hook.c all treat limit==0 as unlimited). cuDeviceTotalMem_v2 forwarded that value straight through as *bytes with CUDA_SUCCESS, so an out-of-range dev looked like a real device reporting zero total memory instead of an invalid-device error. Check the range before calling get_current_device_memory_limit and return CUDA_ERROR_INVALID_DEVICE directly, matching the real driver's behavior for an invalid device ordinal. This only touches the one call site that is CUDA-API-facing; oom_check, cuMemGetInfo_v2, and the NVML hook already fail closed in the only case that matters in practice, an unconfigured real device reads as unlimited by design, same as before this PR. Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com> Signed-off-by: om7057 <kulkarniom7057@gmail.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@iemAnshuman fixed all four:
|
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 1248-1251: Move the dev bounds validation in the relevant
initialization function before the sm_init_flag fast path, so invalid IDs log
the error and return -1 even when initialization is complete. Preserve the
existing return 0 behavior for valid devices when shared_region->sm_init_flag is
1.
🪄 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: f4d63c2a-1d81-4a28-903d-240c1c681080
📒 Files selected for processing (3)
src/cuda/device.csrc/multiprocess/multiprocess_memory_limit.csrc/multiprocess/multiprocess_memory_limit.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/multiprocess/multiprocess_memory_limit.h
CodeRabbit caught this on PR Project-HAMi#238: set_current_device_sm_limit_scale checked sm_init_flag==1 and returned 0 before checking dev's bounds, so an out-of-range dev with sm_init_flag already set to 1 reported success instead of the -1 this same PR just added for every other illegal-dev case in this file. Swapped the order so the bounds check runs first regardless of sm_init_flag's state. Also fixed pre-existing spacing on the lines this moves past (sm_init_flag==1, the LOG_INFO argument list, the sm_limit[dev] assignment), since they were touched by the reorder anyway. Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com> Signed-off-by: om7057 <kulkarniom7057@gmail.com>
4 participants
Closes part of Project-HAMi/HAMi#2127.
context_size,hostpid,proc_num,statusandlast_kernel_timeinshared_region_twere all made_Atomicin 24a0f49, butlimitandsm_limitwere missed.Both are read on the CUDA allocation hot path (
get_current_device_memory_limit, called from every hookedcuMemAlloc) and are exposed through public, tested setters on this side (set_current_device_memory_limit) and on the Go monitor side (SetDeviceMemoryLimit,SetDeviceSmLimitinpkg/monitor/nvidia), so a plainuint64_there does not match the atomicity already guaranteed for the rest of the struct.I checked and there is currently only one writer of these two fields in the whole system:
do_init_device_memory_limits/do_init_device_sm_limitsat shared-region creation, serialized by the region's file lock before the region is published. So this is not fixing an observed bug, it is closing the one gap left from 24a0f49 in a struct where every other field already carries this guarantee, and in tested API surface that a future caller could otherwise reintroduce the same race through.Compatibility
_Atomic uint64_thas the same size and representation asuint64_ton the supported targets, so this does not changeshared_region_t's layout. I verified withsizeof/offsetofbefore and after the change:All identical, so the on-disk
cudevshr.cacheformat and the Go-side mirror struct are unaffected.Re-verified against the current head (
3b56dde) withbench/abi_checkfrom #239:Changes
limit/sm_limitinshared_region_tare now_Atomic uint64_t[CUDA_DEVICE_MAX_COUNT].get_current_device_memory_limit,get_current_device_sm_limit,set_current_device_memory_limituseatomic_load_explicit/atomic_store_explicitwith acquire/release, matching the style already used elsewhere in this file forproc_num,status,last_kernel_time.do_init_device_memory_limits/do_init_device_sm_limitsstill take a plainuint64_t*and are otherwise unchanged, including the already-initialized comparison branch that calls them.try_create_shrregnow fills a localuint64_tarray through them and copies that into the region withatomic_store_explicit(memory_order_relaxed, since the existingatomic_thread_fence(release)a few lines down already publishes it), instead of writing through a plain-uint64_t*cast into the_Atomic-qualified fields, which would have been a type violation independent of whether it's safe on current targets.set_current_device_sm_limit_scale,get_current_device_sm_limit,set_current_device_memory_limit, andget_current_device_memory_limitnowreturnafterLOG_ERROR("Illegal device id...")instead of falling through to index the array anyway, which was an out-of-bounds read for an invaliddev.cuDeviceTotalMem_v2(src/cuda/device.c) additionally now returnsCUDA_ERROR_INVALID_DEVICEdirectly for an out-of-rangedev, rather than forwarding whateverget_current_device_memory_limit's 0 return produces: that 0 means "no limit configured" everywhere else it's read (oom_check,cuMemGetInfo_v2, the NVML hook), so it does not fail closed, and this PR doesn't claim it does for those three; thecuDeviceTotalMem_v2check is the one place that needed and got an explicit fix instead.Testing
libvgpu.sobuilds clean with no new warnings.libvgpu.so,LD_PRELOAD-interposed) before and after this change as a functional regression check; numbers are unchanged.A companion PR updates the Go side of this same shared struct: Project-HAMi/HAMi#2179.
Note on the CodeRabbit summary below: "kept existing input validation and return behavior unchanged" was accurate for the version it was generated against, but is no longer accurate as of the illegal-
devfix described above; return behavior does change for that case now, on purpose.Summary by CodeRabbit