Restore tracked device resources on their original GPU#3077
Restore tracked device resources on their original GPU#3077fallintoplace wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughBoth ChangesPer-device resource restoration
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
achirkin
left a comment
There was a problem hiding this comment.
Thank you for uncovering this! The memory tracking/statistics APIs are rather new and haven't been properly tested in multi-gpu environment yet.
In the context of raft::resources handle we already have access to a "device id" resource (core/resource/device_id.hpp) stored per resources handle. Therefore, there's no need for an extra member variable duplicating this information (see below). Please adjust both files to use this resource instead.
| { | ||
| mr::set_default_host_resource(old_host_); | ||
| rmm::mr::set_current_device_resource(std::move(old_device_)); | ||
| rmm::mr::set_per_device_resource(rmm::cuda_device_id{device_id_}, std::move(old_device_)); |
There was a problem hiding this comment.
core/resource/device_id.hpp is the source of truth for the GPU device associated with the handle:
| rmm::mr::set_per_device_resource(rmm::cuda_device_id{device_id_}, std::move(old_device_)); | |
| rmm::mr::set_per_device_resource(resource::get_device_id(*this), std::move(old_device_)); |
| : resources(existing), | ||
| device_id_(device_setter::get_current_device()), | ||
| old_host_(mr::get_default_host_resource()), | ||
| old_device_(rmm::mr::get_current_device_resource_ref()) |
There was a problem hiding this comment.
Calling resource::get_device_id() on the current handle ensures the device id resource is instantiated and won't mutate until the resource is destroyed.
| old_device_(rmm::mr::get_current_device_resource_ref()) | |
| old_device_(rmm::mr::get_per_device_resource_ref(resource::get_device_id(existing))) |
|
/ok to test b0ddd70 |
Summary
memory_stats_resourcesandmemory_tracking_resourcesdevice memory resources to the CUDA device they wrapped at construction timermm::mr::set_per_device_resource(...)using the captured device idWhy
Both wrappers used
rmm::mr::set_current_device_resource(...)during teardown. In multi-GPU flows that writes the saved resource into whichever CUDA device is current at destruction time, which can leave the construction device pointing at the tracking adaptor and install the wrong resource on another GPU.Impact
This keeps per-device RMM state stable across wrapper teardown and closes a nasty multi-GPU correctness hole that could otherwise lead to wrong-device allocators being reused.
Validation
git diff --checkPARALLEL_LEVEL=8 ./build.sh tests -n --limit-tests=CORE_TESTnvcc/CUDAToolkit_ROOT, so the new tests were not run here