Repository navigation
refactor(rocshmem/gda): refactor QueuePair class - #7217
Conversation
|
TODO:
|
5172b65 to
795eb8d
Compare
|
GIN will need to use |
62b6284 to
b5c1293
Compare
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
Pull request overview
Refactors rocSHMEM’s GDA QueuePair implementation by splitting the prior monolithic class into provider-specific queue-pair classes (IONIC/BNXT/MLX5) plus an optional runtime-dispatching mux, enabling cleaner separation of responsibilities and compile-time provider selection.
Changes:
- Introduces provider-specific
QueuePair*implementations and aQueuePairMuxruntime-dispatch wrapper, withQueuePairselected via a newqueue_pair_provider.hppalias. - Updates GDA backend initialization and device-side context code to use the new
QueuePairAPI (including new posting options usage). - Updates build system and bitcode compilation to support
GDA_MUXand provider-specific compilation/selection.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/rocshmem/src/gda/queue_pair.cpp | Removes the legacy monolithic QueuePair implementation (replaced by refactored classes). |
| projects/rocshmem/src/gda/queue_pair_provider.hpp | Adds QueuePair alias selection for provider/mux at compile time. |
| projects/rocshmem/src/gda/queue_pair_mux.hpp | Adds mux QueuePair type and device-side dispatch helpers. |
| projects/rocshmem/src/gda/queue_pair_mux.cpp | Implements mux union lifecycle and host-side dispatch helpers. |
| projects/rocshmem/src/gda/mlx5/queue_pair_mlx5.hpp | Adds MLX5 provider-specific queue pair class interface + templated posting. |
| projects/rocshmem/src/gda/mlx5/queue_pair_mlx5.cpp | Moves MLX5 queue-pair implementation out of legacy code into provider class. |
| projects/rocshmem/src/gda/mlx5/provider_gda_mlx5.hpp | Refactors MLX5 inline decision helper to a template form. |
| projects/rocshmem/src/gda/mlx5/CMakeLists.txt | Adjusts MLX5 sources inclusion based on provider build option. |
| projects/rocshmem/src/gda/mlx5/backend_gda_mlx5.cpp | Updates MLX5 GPU QP initialization to construct new provider QP object. |
| projects/rocshmem/src/gda/ionic/queue_pair_ionic.hpp | Adds IONIC provider-specific queue pair class interface + templated posting. |
| projects/rocshmem/src/gda/ionic/queue_pair_ionic.cpp | Moves IONIC queue-pair implementation out of legacy code into provider class. |
| projects/rocshmem/src/gda/ionic/provider_gda_ionic.hpp | Introduces device-queue structs and namespaces for IONIC provider internals. |
| projects/rocshmem/src/gda/ionic/CMakeLists.txt | Adjusts IONIC sources inclusion based on provider build option. |
| projects/rocshmem/src/gda/ionic/backend_gda_ionic.cpp | Updates IONIC GPU QP initialization to construct new provider QP object. |
| projects/rocshmem/src/gda/bnxt/queue_pair_bnxt.hpp | Adds BNXT provider-specific queue pair class interface + templated posting. |
| projects/rocshmem/src/gda/bnxt/queue_pair_bnxt.cpp | Moves BNXT queue-pair implementation out of legacy code into provider class. |
| projects/rocshmem/src/gda/bnxt/provider_gda_bnxt.hpp | Adds missing includes and constructors for BNXT device queue structs. |
| projects/rocshmem/src/gda/bnxt/CMakeLists.txt | Adjusts BNXT sources inclusion based on provider build option. |
| projects/rocshmem/src/gda/bnxt/backend_gda_bnxt.cpp | Updates BNXT GPU QP initialization to construct new provider QP object. |
| projects/rocshmem/src/gda/gda_enums.hpp | Changes provider enum to enum class and keeps GDAProvider alias. |
| projects/rocshmem/src/gda/debug_gda.hpp | Minor annotation update ([[maybe_unused]]). |
| projects/rocshmem/src/gda/context_gda_tmpl_device.hpp | Updates device-side calls to new QP AMO/RMA APIs and posting options. |
| projects/rocshmem/src/gda/context_gda_device.hpp | Switches include to queue_pair_provider.hpp. |
| projects/rocshmem/src/gda/backend_gda.hpp | Updates includes and provider-guarded declarations for backend init paths. |
| projects/rocshmem/src/gda/backend_gda.cpp | Updates buffer registration paths and GPU QP setup/teardown for refactor. |
| projects/rocshmem/src/gda/CMakeLists.txt | Removes legacy queue_pair.cpp and conditionally adds mux sources. |
| projects/rocshmem/CMakeLists.txt | Adds GDA_MUX option, provider-count checks, and backend string updates. |
| projects/rocshmem/cmake/rocshmem_config.h.in | Adds GDA_MUX config define. |
| projects/rocshmem/cmake/DeviceBitcode.cmake | Updates bitcode sources to compile mux and provider-specific QP sources. |
Comments suppressed due to low confidence (1)
projects/rocshmem/src/gda/backend_gda.cpp:587
- Host-side buffer_register/buffer_unregister calls QueuePair methods through gpu_qps, but gpu_qps is allocated with hipMalloc (device memory). Under standard HIP semantics the host cannot dereference/call methods on device allocations, so registration/destruction will be invalid (and was previously done via a host mirror + hipMemcpy). Consider restoring a host-resident QP array for host-only operations (MR map + buffer_info updates) and explicitly copying the device-visible portions to gpu_qps, or switching gpu_qps to a host-accessible allocation (e.g., hipMallocManaged/hipHostMalloc with the required coherency guarantees).
/* Register with QPs */
for (size_t i = 0; i < num_qps; i++) {
err = gpu_qps[i].buffer_register(addr, length);
if (ROCSHMEM_SUCCESS != err) {
qp_registration_failed = true;
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4ca9fab to
8ef949a
Compare
Refactor monolithic QueuePair class into:
* template class QueuePairSHMEM<typename Provider>
a CRTP mixin class implementing a high-level SHMEM-like interface
* template class QueuePairBase<typename Provider> : public QueuePairSHMEM<Provider>
a CRTP base class implementing data members and intialization
common to all QueuePair provider implementations
* QueuePairMLX5 : public QueuePairBase<QueuePairMLX5>
a concrete QueuePair implementation for mlx5
* QueuePairGeneric : public QueuePairSHMEM<QueuePairGeneric>
a variant (tagged union) class that dispatches
to an underlying concrete implementation at runtime
* QueuePairBNXT and QueuePairIONIC implementations will follow shortly
* extern "C" ::rocshmem_gin_init_constmem definition has been moved to constmem.cpp
Signed-off-by: Omri Mor <Omri.Mor@amd.com>
Signed-off-by: Omri Mor <Omri.Mor@amd.com>
Signed-off-by: Omri Mor <Omri.Mor@amd.com>
Signed-off-by: Omri Mor <Omri.Mor@amd.com>
Signed-off-by: Omri Mor <Omri.Mor@amd.com>
Signed-off-by: Omri Mor <Omri.Mor@amd.com>
There was a problem hiding this comment.
ARGUS
This PR changes 10,126 lines across 66 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: Nothing new worth raising; still open from earlier: queue_pair_host.cpp:108 and 4 more. Worth clearing before approval.
Comment @argus review to re-run, or @argus re-review after pushing.
Signed-off-by: Omri Mor <Omri.Mor@amd.com>
A PostOpt object could be constructed with arbitrary types. It wouldn't do anything, but was semantically nonsense, so reject such arguments. Signed-off-by: Omri Mor <Omri.Mor@amd.com>
Signed-off-by: Omri Mor <Omri.Mor@amd.com> Reported-by: amd-argus in #7217 review
Signed-off-by: Omri Mor <Omri.Mor@amd.com>
There was a problem hiding this comment.
ARGUS
This PR changes 10,153 lines across 66 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: Nothing new worth raising; still open from earlier: CMakeLists.txt:1110 and 8 more. Worth clearing before approval.
Comment @argus review to re-run, or @argus re-review after pushing.
|
@argus re-review full |
Signed-off-by: Omri Mor <Omri.Mor@amd.com>
Signed-off-by: Omri Mor <Omri.Mor@amd.com>
…aders Several headers derived from rdma-core or the Linux kernel contain copyright notices that refer to a COPYING file. Clarify that this file is located in the origin source tree, not our own. Signed-off-by: Omri Mor <Omri.Mor@amd.com>
There was a problem hiding this comment.
ARGUS
This PR changes 9,818 lines across 70 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: Nothing new worth raising; still open from earlier: CMakeLists.txt:1067 and 1 more. Worth clearing before approval.
Comment @argus review to re-run, or @argus re-review after pushing.
TheRock Submodule Bump ActivityNewest first
|
## Motivation
The QueuePair class in src/gda/queue_pair.{hpp,cpp} operates
as "three classes in a trenchcoat" that masquerade as a single class;
it simultaneously has functionality for:
- common device-side QP initialization and management,
including host functions for registering additional local buffers; and
- provider-specific send and completion queue management,
including WQE posting and CQ polling; and
- a multiplexing low-level interface that dispatches to the provider-specific
functions based on which is at use at execution time; and
- an intermediate-level interface that hides details such as opcodes,
implemented on top of the low-level interface.
Maintaining this complex class and its many interactions is complicated,
has led to changes in one GDA provider causing unintended impacts on others,
limits GDA provider performance, and hinders development of new features.
This PR refactors the single QueuePair class into several distinct classes,
which thereby improves code readability, maintainability, and modularity;
allows for independent unit testing of QueuePair functionality;
restores the ability to compile directly for specific GDA providers
without runtime dispatch; and paves the way for the decoupling
of the RCCL-GIN GDA backend from rocSHMEM.
## Technical Details
### rocSHMEM
A class hierarchy is implemented using CRTP:
- rocshmem::QueuePairInterface<typename Provider>
A common mid-level interface used directly by GIN GDA
and indirectly by rocSHMEM, via the high-level interface.
- rocshmem::QueuePairSHMEM<typename Provider> : QueuePairInterface<Provider>
A common high-level interface that is used directly by rocshmem::GDAContext.
- rocshmem::QueuePairDevice<typename Provider> : QueuePairSHMEM<Provider>
A common base class for concrete GDA provider QueuePair types.
- rocshmem::QueuePairTraits<typename Provider>
Used by the CRTP base classes to query provider-specific attributes.
- rocshmem::QueuePairHost
Used to manage the lifetime of memory used by QueuePairDevice instances.
These are used to implement the concrete GDA provider QueuePair classes:
- rocshmem::QueuePairIONIC
GDA provider for AMD Pensando RDMA networks using the 'ionic' driver.
- rocshmem::QueuePairBNXT
GDA provider for Broadcom RDMA networks using the 'bnxt' driver.
- rocshmem::QueuePairMLX5
GDA provider for Mellanox RDMA networks using the 'mlx5' driver.
There are two additional 'meta' provider QueuePair classes:
- rocshmem::QueuePairMux
Supports runtime GDA provider selection via a specialized tagged union.
- rocshmem::QueuePairMock
Mock QueuePair object for unit tests.
A compile-time mechanism allows callers of the QueuePair communication APIs
to selectively override certain internal safety and consistency mechanisms.
All QueuePair device code is header-only; this removes the necessity
of linking with the rocSHMEM QueuePair bitcode for GDA support in RCCL GIN.
### RCCL - GIN GDA
Since GIN GDA now requires access to the rocSHMEM QueuePair headers
from within the installed device API headers, when RCCL is configured
with support for GIN GDA (ENABLE_ROCSHMEM_GIN=ON) then the required headers
are copied out of the rocSHMEM source directory into the RCCL build directory
and thence to RCCL's installation prefix include directory. RCCL provides
a subset of these headers as internal replacements instead of rocSHMEM's.
## JIRA ID
JIRA ID: AIROCSHMEM-424
JIRA ID: AICOMRCCL-1457
## Test Plan
Use the rocSHMEM functional tests to validate correctness and
performance regression analysis against current develop HEAD.
Use the alltoall_perf GIN GDA implementation from rccl-tests
to validate correctness for the GDA backend in RCCL GIN.
## Test Result
- [x] Correctness: rocSHMEM functional tests
- [x] ionic
- [x] bnxt
- [x] mlx5
- [x] Performance regression analysis
- [x] ionic
- [x] bnxt
- [x] mlx5
- [x] Correctness: RCCL GIN rccl-tests/alltoall_perf
## Submission Checklist
- [x] Look over the contributing guidelines at
https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
---------
Signed-off-by: Omri Mor <Omri.Mor@amd.com>
## Motivation
The QueuePair class in src/gda/queue_pair.{hpp,cpp} operates
as "three classes in a trenchcoat" that masquerade as a single class;
it simultaneously has functionality for:
- common device-side QP initialization and management,
including host functions for registering additional local buffers; and
- provider-specific send and completion queue management,
including WQE posting and CQ polling; and
- a multiplexing low-level interface that dispatches to the provider-specific
functions based on which is at use at execution time; and
- an intermediate-level interface that hides details such as opcodes,
implemented on top of the low-level interface.
Maintaining this complex class and its many interactions is complicated,
has led to changes in one GDA provider causing unintended impacts on others,
limits GDA provider performance, and hinders development of new features.
This PR refactors the single QueuePair class into several distinct classes,
which thereby improves code readability, maintainability, and modularity;
allows for independent unit testing of QueuePair functionality;
restores the ability to compile directly for specific GDA providers
without runtime dispatch; and paves the way for the decoupling
of the RCCL-GIN GDA backend from rocSHMEM.
## Technical Details
### rocSHMEM
A class hierarchy is implemented using CRTP:
- rocshmem::QueuePairInterface<typename Provider>
A common mid-level interface used directly by GIN GDA
and indirectly by rocSHMEM, via the high-level interface.
- rocshmem::QueuePairSHMEM<typename Provider> : QueuePairInterface<Provider>
A common high-level interface that is used directly by rocshmem::GDAContext.
- rocshmem::QueuePairDevice<typename Provider> : QueuePairSHMEM<Provider>
A common base class for concrete GDA provider QueuePair types.
- rocshmem::QueuePairTraits<typename Provider>
Used by the CRTP base classes to query provider-specific attributes.
- rocshmem::QueuePairHost
Used to manage the lifetime of memory used by QueuePairDevice instances.
These are used to implement the concrete GDA provider QueuePair classes:
- rocshmem::QueuePairIONIC
GDA provider for AMD Pensando RDMA networks using the 'ionic' driver.
- rocshmem::QueuePairBNXT
GDA provider for Broadcom RDMA networks using the 'bnxt' driver.
- rocshmem::QueuePairMLX5
GDA provider for Mellanox RDMA networks using the 'mlx5' driver.
There are two additional 'meta' provider QueuePair classes:
- rocshmem::QueuePairMux
Supports runtime GDA provider selection via a specialized tagged union.
- rocshmem::QueuePairMock
Mock QueuePair object for unit tests.
A compile-time mechanism allows callers of the QueuePair communication APIs
to selectively override certain internal safety and consistency mechanisms.
All QueuePair device code is header-only; this removes the necessity
of linking with the rocSHMEM QueuePair bitcode for GDA support in RCCL GIN.
### RCCL - GIN GDA
Since GIN GDA now requires access to the rocSHMEM QueuePair headers
from within the installed device API headers, when RCCL is configured
with support for GIN GDA (ENABLE_ROCSHMEM_GIN=ON) then the required headers
are copied out of the rocSHMEM source directory into the RCCL build directory
and thence to RCCL's installation prefix include directory. RCCL provides
a subset of these headers as internal replacements instead of rocSHMEM's.
## JIRA ID
JIRA ID: AIROCSHMEM-424
JIRA ID: AICOMRCCL-1457
## Test Plan
Use the rocSHMEM functional tests to validate correctness and
performance regression analysis against current develop HEAD.
Use the alltoall_perf GIN GDA implementation from rccl-tests
to validate correctness for the GDA backend in RCCL GIN.
## Test Result
- [x] Correctness: rocSHMEM functional tests
- [x] ionic
- [x] bnxt
- [x] mlx5
- [x] Performance regression analysis
- [x] ionic
- [x] bnxt
- [x] mlx5
- [x] Correctness: RCCL GIN rccl-tests/alltoall_perf
## Submission Checklist
- [x] Look over the contributing guidelines at
https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
---------
Signed-off-by: Omri Mor <Omri.Mor@amd.com>
Motivation
The QueuePair class [gda/queue_pair.{hpp,cpp}] functions as three classes in a trenchcoat that masquerade as a single class; it simultaneously has functionality for:
Maintaining this complex class and its many interactions is complicated and has led to changes intended for one GDA provider causing unintended impacts on the others, as well making several types of optimizations far more difficult. The goal of this PR is to refactor this single class into several distinct classes that will help improve code readability and maintainability, as well as restoring the ability to compile directly for a specific GDA provider without runtime dispatch.
The end goal is to allow much of the QueuePair and GDA initialization code to be abstracted as a separate library that can be consumed by both rocSHMEM and other users of similar functionality, such as the RCCL-GIN GDA backend.
Technical Details
template <typename Provider> class QueuePairSHMEMProvidertemplate <typename Provider> class QueuePairBaseQueuePairSHMEM<Provider>class QueuePairIONICQueuePairBase<QueuePairIONIC>class QueuePairBNXTQueuePairBase<QueuePairBNXT>class QueuePairMLX5QueuePairBase<QueuePairMLX5>class QueuePairMuxQueuePairIONIC,QueuePairBNXT, orQueuePairMLX5QueuePairSHMEM<QueuePairMux>QueuePairJIRA ID
JIRA ID: AIROCSHMEM-424
Test Plan
Use the rocSHMEM functional tests to validate correctness and performance regression analysis against current develop HEAD.
Test Result
Submission Checklist