Add GpuIpcMem class #704
Add GpuIpcMem class #704
Conversation
Co-authored-by: Copilot <[email protected]>
- [x] Move hash specialization and equality operator from std/global namespace to custom namespace - [x] Update unordered_map to use custom hash and equality as template parameters - [x] Add noexcept to equality operator - [x] Verify the changes build correctly - [x] Run code review and security checks <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/microsoft/mscclpp/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Binyang2014 <[email protected]> Co-authored-by: Binyang Li <[email protected]>
chhwang
force-pushed
the
chhwang/new-ipc-mem
branch
from
5598d49 to
fcb1ab6
Compare
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new GpuIpcMem class to encapsulate GPU IPC (Inter-Process Communication) memory management, replacing the previous inline implementation in RegisteredMemory. The refactoring simplifies memory handle management by consolidating three different handle types (RuntimeIpc, PosixFd, and Fabric) into a unified abstraction layer.
Key changes include:
- Introduction of
GpuIpcMemHandlestruct andGpuIpcMemclass for managing GPU IPC memory handles - Significant simplification of
RegisteredMemoryimplementation by delegating IPC handle management to the new classes - Migration from old
debug.hlogging system to newlogger.hppsystem with GPU subsystem support - Test infrastructure improvements including MPI group caching and better resource cleanup
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/gpu_ipc_mem.cc | New file implementing GpuIpcMem class with support for RuntimeIpc, PosixFd, and Fabric handle types |
| src/include/gpu_ipc_mem.hpp | New header defining GpuIpcMemHandle struct and GpuIpcMem class interfaces |
| src/registered_memory.cc | Refactored to use GpuIpcMem abstraction, removing ~100 lines of inline IPC handle management code |
| src/include/registered_memory.hpp | Updated to use GpuIpcMemHandle in TransportInfo union and removed obsolete fields |
| src/include/logger.hpp | Added GPU subsystem to LogSubsys enum for GPU-related logging |
| test/mp_unit/executor_tests.cc | Moved test prerequisites check from test body to SetUp() method |
| python/test/mscclpp_mpi.py | Added MPI group caching and improved fixture cleanup with try-finally |
| python/test/conftest.py | New conftest for MPI initialization before test collection |
| python/mscclpp/utils.py | Added stdin=subprocess.DEVNULL to prevent subprocess hanging |
| include/mscclpp/gpu.hpp | Added CUDA_ERROR_NOT_SUPPORTED macro mapping for HIP compatibility |
| src/gpu_utils.cc | Removed unused nvlsCompatibleMemHandleType variable |
Comments suppressed due to low confidence (1)
src/registered_memory.cc:27
- The WARN macro call should use mscclpp::LogSubsys::P2P enum value instead of mscclpp::P2P. The logger.hpp defines LogSubsys enum with GPU, NET, CONN, EXEC, NCCL values. P2P is not defined in LogSubsys. Consider using GPU subsystem here since this is GPU IPC related code.
WARN("Call to " #cmd " failed, error is %s", errStr); \
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| namespace std { | ||
|
|
||
| std::string to_string(const mscclpp::GpuIpcMemHandle::TypeFlags& typeFlags) { | ||
| std::stringstream ss; | ||
| if (typeFlags & mscclpp::GpuIpcMemHandle::Type::RuntimeIpc) { | ||
| ss << "RuntimeIpc|"; | ||
| } | ||
| if (typeFlags & mscclpp::GpuIpcMemHandle::Type::PosixFd) { | ||
| ss << "PosixFd|"; | ||
| } | ||
| if (typeFlags & mscclpp::GpuIpcMemHandle::Type::Fabric) { | ||
| ss << "Fabric|"; | ||
| } | ||
| std::string ret = ss.str(); | ||
| if (!ret.empty()) { | ||
| ret.pop_back(); // Remove the trailing '|' | ||
| return ret; | ||
| } | ||
| return "None"; | ||
| } | ||
|
|
||
| } // namespace std |
There was a problem hiding this comment.
Adding a custom to_string function in the std namespace for a user-defined type (GpuIpcMemHandle::TypeFlags) is technically undefined behavior according to the C++ standard. While it may work in practice, consider moving this function to the mscclpp namespace as a free function or as a static member of GpuIpcMemHandle instead.
| GpuIpcMem::GpuIpcMem(const GpuIpcMemHandle& handle) | ||
| : handle_(handle), | ||
| isMulticast_(false), | ||
| multicastBindedAddr_(0), | ||
| type_(GpuIpcMemHandle::Type::None), | ||
| basePtr_(nullptr), | ||
| baseSize_(0), | ||
| dataPtr_(nullptr), | ||
| dataSize_(0) { |
There was a problem hiding this comment.
The allocHandle_ member variable is not initialized in the constructor initializer list. If the code reaches line 303 and throws before allocHandle_ is set (when neither Fabric nor PosixFd succeeds but RuntimeIpc also fails), allocHandle_ will contain an undefined value. While the destructor checks type_ before using allocHandle_, it's better to initialize allocHandle_ to a known invalid value (e.g., 0) in the initializer list to prevent potential issues.
| void GpuIpcMemHandle::deleter(GpuIpcMemHandle* handle) { | ||
| if (handle) { | ||
| if (handle->typeFlags & GpuIpcMemHandle::Type::PosixFd) { | ||
| ::close(handle->posixFd.fd); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The deleter function should also call 'delete handle' to avoid memory leak. Currently, it only closes the file descriptor but doesn't free the GpuIpcMemHandle object itself. Add 'delete handle;' at the end of the function.
No description provided.