Repository navigation
fix(rccl): critical stability fixes to the 2.30.7 release branch - #9381
JhaShweta1 wants to merge 5 commits into
Conversation
…rdown The TearDown safety-net cancelled a nonblocking collective (MPI_Ibarrier) request on timeout, which the MPI standard forbids. OpenMPI raises MPI_ERR_REQUEST and, because MPI_COMM_WORLD uses MPI_ERRORS_ARE_FATAL, that MPI_Cancel call itself aborts the job with a misleading error before the intended MPI_Abort runs. Remove the MPI_Cancel/MPI_Request_free and call MPI_Abort directly (it tears the job down anyway). Also raise the teardown-barrier timeout 3s -> 60s (overridable via RCCL_TEST_TEARDOWN_TIMEOUT_SEC) so benign rank skew in heavy multi-phase MPI tests (GrowMPITest, NetIbMPITest CtsDepthStress) no longer trips a false 'out of sync' abort. Test-harness only; no librccl behavior change. Recovers Grow_MultiRankJump and NetIB_CtsDepthStress (ib-cast) from false failures. Co-authored-by: Cursor <cursoragent@cursor.com>
The CAST connect path sizes each CQ as 3 * NET_IB_MAX_REQUESTS * QpsPerConn (= 768 * nqps). On AMD AI NIC (ionic) max_cqe is 65435, so for nqps >= ~86 ibv_create_cq fails, tearing down the connection handshake and leaving NULL send/recv comms that later segfault (e.g. Cast_MaxQPCount128). mlx5 reports a huge max_cqe so is unaffected. Store max_cqe per device and clamp the requested CQ size to it where the CQ size is computed (IbCastConnect / ibCastAccept).
✅ 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
Backports two stability fixes to the RCCL 2.30.7 branch: (1) make MPI test teardown more robust by avoiding an illegal MPI_Cancel on an MPI_Ibarrier request and by increasing/parameterizing the teardown timeout, and (2) prevent net_ib_cast completion queue (CQ) allocation failures by clamping requested CQ sizes to the device-reported max_cqe.
Changes:
- RCCL MPI test harness: remove illegal cancel path by directly calling
MPI_Aborton teardown-barrier timeout; raise default timeout to 60s and make it env-overridable. - net_ib_cast: record per-device
max_cqeand clamp CQ sizes in CAST connect/accept paths to avoidibv_create_cqfailure at high QP counts. - Extend
ncclIbDevto storemaxCqe.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| projects/rccl/test/common/MPIEnvironment.cpp | Makes teardown barrier timeout more tolerant/overridable and avoids illegal MPI request cancellation by aborting directly on timeout. |
| projects/rccl/src/transport/net_ib_cast/init.cc | Stores the device-reported CQE limit (max_cqe) into the device table for later clamping. |
| projects/rccl/src/transport/net_ib_cast/connect.cc | Clamps requested CQ size to the device maxCqe in both connect and accept paths before CQ creation. |
| projects/rccl/src/transport/net_ib_cast/common_cast.h | Adds maxCqe field to ncclIbDev to carry the per-device CQE limit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int timeout_sec = 60; | ||
| if(const char* env = std::getenv("RCCL_TEST_TEARDOWN_TIMEOUT_SEC")) | ||
| { | ||
| int v = std::atoi(env); | ||
| if(v > 0) timeout_sec = v; |
JhaShweta1
changed the title
|
@JhaShweta1 is it still relevant? I believe that we should get these changes via sync branch integration, right? |
|
@ddurnov That PR was created while the SYNC PR was still open. At that time, we wanted to integrate these fixes to get the tests passing on origin/develop, because we didn't know when the SYNC branch would be aligned with origin/develop. |
|
@JhaShweta1 Is this PR still relevant? |
|
Per discussion w/ @JhaShweta1, this is not relevant anymore. Closing it out. |
Motivation
Backport critical stability fixes to the 2.30.7 release branch: resolve an illegal MPI_Cancel on Ibarrier during RCCL test teardown, and prevent CQ-size overflow in net_ib_cast by clamping to the device's max_cqe. Together these eliminate a test-teardown crash and a potential IB completion-queue allocation failure.
Technical Details
This backports two 2.30.7 fixes. PR #9322 (RCCL test harness): the MPIEnvironment::TearDown safety-net illegally called MPI_Cancel on a nonblocking collective (MPI_Ibarrier) request, which OpenMPI rejects with MPI_ERR_REQUEST under MPI_ERRORS_ARE_FATAL, aborting the job with a misleading error; it now calls MPI_Abort directly and raises the teardown-barrier timeout 3s→60s (overridable via RCCL_TEST_TEARDOWN_TIMEOUT_SEC) to stop false "out of sync" failures in heavy multi-phase tests. PR #9335 (net_ib_cast): the CAST connect path sized each CQ as 3 * NET_IB_MAX_REQUESTS * QpsPerConn, which exceeds the AMD AI NIC (ionic) max_cqe of 65435 for ~86+ QPs, failing ibv_create_cq and leaving NULL comms that later segfault; it now stores each device's max_cqe and clamps the requested CQ size to it in IbCastConnect/ibCastAccept. Together they recover Grow_MultiRankJump, NetIB_CtsDepthStress, and Cast_MaxQPCount128 from false failures/crashes, with no librccl runtime behavior change.
Issue Tracking
Test Plan
Build librccl + RCCL test suite with MPI_TESTS_ENABLED.
Run the previously-failing MPI tests on an AMD AI NIC (ionic) node:
GrowMPITest.Grow_MultiRankJump — exercises the teardown-barrier rank-skew path (PR [RCCL][test] Fix illegal MPI_Cancel on Ibarrier in MPIEnvironment tea… #9322).
NetIbMPITest.NetIB_CtsDepthStress (ib-cast) — heavy multi-phase teardown timing (PR [RCCL][test] Fix illegal MPI_Cancel on Ibarrier in MPIEnvironment tea… #9322).
Cast_MaxQPCount128 — drives nqps past ionic's max_cqe (65435) to hit the CQ-size clamp (PR net_ib_cast: clamp CQ size to device max_cqe #9335).
Confirm no regression by running the broader RCCL MPI test set.
Sanity-check the timeout override via RCCL_TEST_TEARDOWN_TIMEOUT_SEC.
Test Result
Submission Checklist