From 7a6defe6a8872f318f295f7f1312aa76bca6a22b Mon Sep 17 00:00:00 2001 From: nileshnegi Date: Sun, 26 Apr 2026 11:33:50 -0500 Subject: [PATCH 1/2] Fix collective mismatch hang/segfault in multi-rank error paths Three places used ERR_CHECK inside rank-specific guards preceding (or within loops containing) subsequent Broadcast collectives. A failure on one rank would cause it to early-return before the collective, leaving all other ranks blocked indefinitely. - ExchangeMemory (POD path): capture hipSetDevice + hipMemExport- ToShareableHandle errors into exportErr and broadcast it after the handle broadcast, so all ranks see the failure and return together. Apply the same treatment to the import-side sequence (hipSetDevice, hipMemImport, hipMemAddressReserve, hipMemMap, hipMemSetAccess), broadcasting importErr before returning. - NIC executor QP setup: replace ERR_CHECK inside the per-QP loop with captured ErrType values broadcast from srcMemRank and dstMemRank respectively, so a TransitionQpToRtr/Rts failure on any rank does not leave others blocked on the next iteration's Broadcast when qpCount > 1. - BroadcastTfrResult: fix non-root path broadcasting only the first element of perIterCUs[i] regardless of setSize; replace the if-guard with a loop matching the root's per-element sends. Fix segfault and improve error diagnostics in multi-rank error paths - CheckMultiNodeConfigConsistency: null out vector members in DataOptions (fillPattern, fillCompress) and GfxOptions (cuMask, prefXccTable) before sizeof-broadcast. Broadcasting a struct containing std::vector transmits rank 0's heap pointers to other ranks; on scope exit the receiving rank calls free(remote_ptr) causing a segfault. These fields are permitted to differ across ranks and are not compared by the consistency check. - NIC QP transition: replace bare ErrType broadcast with a POD QpTransitionResult {errType, rtrFailed} struct so the failing stage (RTR vs RTS) is preserved and reported accurately. Previously all failures reported "to RTS" regardless of which transition failed. Adds static_assert(is_trivially_copyable) to enforce broadcast safety. Co-authored-by: Claude --- src/header/TransferBench.hpp | 93 ++++++++++++++++++++++++++++-------- 1 file changed, 73 insertions(+), 20 deletions(-) diff --git a/src/header/TransferBench.hpp b/src/header/TransferBench.hpp index 217ae0c3..e719ed84 100644 --- a/src/header/TransferBench.hpp +++ b/src/header/TransferBench.hpp @@ -1867,6 +1867,11 @@ namespace { // Compare data options { DataOptions data = cfg.data; + // Null out vector members before sizeof-broadcast: vectors carry heap pointers that are + // invalid on other ranks; freeing a remote pointer on scope exit causes a segfault + // These fields are permitted to differ across ranks and are not compared below + decltype(data.fillPattern)().swap(data.fillPattern); + decltype(data.fillCompress)().swap(data.fillCompress); System::Get().Broadcast(root, sizeof(data), &data); // data.alwaysValidate is permitted to be different across ranks @@ -1911,6 +1916,10 @@ namespace { // Compare GFX Executor options { GfxOptions gfx = cfg.gfx; + // Same as above: null out vector members before sizeof-broadcast + // both fields are permitted to differ across ranks + decltype(gfx.cuMask)().swap(gfx.cuMask); + decltype(gfx.prefXccTable)().swap(gfx.prefXccTable); System::Get().Broadcast(root, sizeof(gfx), &gfx); if (gfx.blockOrder != cfg.gfx.blockOrder) ADD_ERROR("cfg.gfx.blockOrder"); if (gfx.blockSize != cfg.gfx.blockSize) ADD_ERROR("cfg.gfx.blockSize"); @@ -3523,13 +3532,38 @@ static bool IsConfiguredGid(union ibv_gid const& gid) // Move queue pairs to ready-to-receive (RTR), using exchanged connection info // Then move them to read-to-send (RTS) + // Broadcast each rank's result so all ranks fail together rather than + // hanging on the next iteration's Broadcast when qpCount > 1. + struct QpTransitionResult { ErrType errType; bool rtrFailed; }; + static_assert(std::is_trivially_copyable::value, "QpTransitionResult must be trivially copyable for MPI broadcast"); + QpTransitionResult srcQpResult = {ERR_NONE, false}; if (GetRank() == srcMemRank) { - ERR_CHECK(TransitionQpToRtr(rss.srcQueuePairs[i], dstConnInfo, port, srcIsRoCE, rss.srcPortAttr.active_mtu)); - ERR_CHECK(TransitionQpToRts(rss.srcQueuePairs[i])); + ErrResult err = TransitionQpToRtr(rss.srcQueuePairs[i], dstConnInfo, port, srcIsRoCE, rss.srcPortAttr.active_mtu); + srcQpResult.rtrFailed = (err.errType != ERR_NONE); + if (err.errType == ERR_NONE) { + err = TransitionQpToRts(rss.srcQueuePairs[i]); + } + srcQpResult.errType = err.errType; + } + System::Get().Broadcast(srcMemRank, sizeof(srcQpResult), &srcQpResult); + if (srcQpResult.errType != ERR_NONE) { + return {ERR_FATAL, "SRC rank %d failed to transition QP %d to %s", srcMemRank, i, + srcQpResult.rtrFailed ? "RTR" : "RTS"}; } + + QpTransitionResult dstQpResult = {ERR_NONE, false}; if (GetRank() == dstMemRank) { - ERR_CHECK(TransitionQpToRtr(rss.dstQueuePairs[i], srcConnInfo, port, dstIsRoCE, rss.dstPortAttr.active_mtu)); - ERR_CHECK(TransitionQpToRts(rss.dstQueuePairs[i])); + ErrResult err = TransitionQpToRtr(rss.dstQueuePairs[i], srcConnInfo, port, dstIsRoCE, rss.dstPortAttr.active_mtu); + dstQpResult.rtrFailed = (err.errType != ERR_NONE); + if (err.errType == ERR_NONE) { + err = TransitionQpToRts(rss.dstQueuePairs[i]); + } + dstQpResult.errType = err.errType; + } + System::Get().Broadcast(dstMemRank, sizeof(dstQpResult), &dstQpResult); + if (dstQpResult.errType != ERR_NONE) { + return {ERR_FATAL, "DST rank %d failed to transition QP %d to %s", dstMemRank, i, + dstQpResult.rtrFailed ? "RTR" : "RTS"}; } // Prepare scatter-gather element / work request for this queue pair in advance @@ -3920,27 +3954,46 @@ static bool IsConfiguredGid(union ibv_gid const& gid) // If pod communication is required, export/import fabric handle if (memDevice.memRank != exeDevice.exeRank && IsGpuExeType(exeDevice.exeType)) { #ifdef POD_COMM_ENABLED - // mem rank exports to sharable fabric handle - hipMemFabricHandle_t fabricHandle; + // mem rank exports to sharable fabric handle; broadcast handle + status so all + // ranks fail together instead of hanging on the next collective if export fails + hipMemFabricHandle_t fabricHandle = {}; + hipError_t exportErr = hipSuccess; if (memDevice.memRank == GetRank()) { - ERR_CHECK(hipSetDevice(memDevice.memIndex)); - ERR_CHECK(hipMemExportToShareableHandle(&fabricHandle, *memHandle, hipMemHandleTypeFabric, 0)); + exportErr = hipSetDevice(memDevice.memIndex); + if (exportErr == hipSuccess) { + exportErr = hipMemExportToShareableHandle(&fabricHandle, *memHandle, hipMemHandleTypeFabric, 0); + } } System::Get().Broadcast(memDevice.memRank, sizeof(hipMemFabricHandle_t), &fabricHandle); + System::Get().Broadcast(memDevice.memRank, sizeof(hipError_t), &exportErr); + if (exportErr != hipSuccess) { + return {ERR_FATAL, "HIP Error during fabric handle export: %s", hipGetErrorString(exportErr)}; + } - // exe rank imports the fabric handle + // exe rank imports the fabric handle; broadcast result so all ranks fail together + hipError_t importErr = hipSuccess; if (exeDevice.exeRank == GetRank()) { - ERR_CHECK(hipSetDevice(exeDevice.exeIndex)); - ERR_CHECK(hipMemImportFromShareableHandle(memHandle, (void*)&fabricHandle, hipMemHandleTypeFabric)); - ERR_CHECK(hipMemAddressReserve((gpu_device_ptr*)memPtr, *pActualBytes, 0, 0, 0)); - ERR_CHECK(hipMemMap((gpu_device_ptr)*memPtr, *pActualBytes, 0, *memHandle, 0)); - - // Specify memory access descriptor to enable local read/write - hipMemAccessDesc desc; - desc.location = {hipMemLocationTypeDevice, exeDevice.exeIndex}; - desc.flags = hipMemAccessFlagsProtReadWrite; - ERR_CHECK(hipMemSetAccess((gpu_device_ptr)*memPtr, *pActualBytes, &desc, 1)); + importErr = hipSetDevice(exeDevice.exeIndex); + if (importErr == hipSuccess) { + importErr = hipMemImportFromShareableHandle(memHandle, (void*)&fabricHandle, hipMemHandleTypeFabric); + } + if (importErr == hipSuccess) { + importErr = hipMemAddressReserve((gpu_device_ptr*)memPtr, *pActualBytes, 0, 0, 0); + } + if (importErr == hipSuccess) { + importErr = hipMemMap((gpu_device_ptr)*memPtr, *pActualBytes, 0, *memHandle, 0); + } + if (importErr == hipSuccess) { + hipMemAccessDesc desc; + desc.location = {hipMemLocationTypeDevice, exeDevice.exeIndex}; + desc.flags = hipMemAccessFlagsProtReadWrite; + importErr = hipMemSetAccess((gpu_device_ptr)*memPtr, *pActualBytes, &desc, 1); + } + } + System::Get().Broadcast(exeDevice.exeRank, sizeof(hipError_t), &importErr); + if (importErr != hipSuccess) { + return {ERR_FATAL, "HIP Error during fabric handle import: %s", hipGetErrorString(importErr)}; } #else return {ERR_FATAL, "Unable to export/import fabric handle without compiling with pod communication support"}; @@ -7000,7 +7053,7 @@ static bool IsConfiguredGid(union ibv_gid const& gid) } else { BROADCAST(setSize); tfrResult.perIterCUs[i].clear(); - if (setSize > 0) { + for (size_t j = 0; j < setSize; j++) { pair p; BROADCAST(p); tfrResult.perIterCUs[i].insert(p); From 36489224d19bd44bd17f5c1240d336aaf9a80d8e Mon Sep 17 00:00:00 2001 From: nileshnegi Date: Sun, 26 Apr 2026 21:16:42 -0500 Subject: [PATCH 2/2] Address Copilot review: CUDA type fix and diagnostic improvements - Fix CUDA build: hip fabric macros (hipMemExportToShareableHandle, hipMemImportFromShareableHandle, hipMemAddressReserve, hipMemMap, hipMemSetAccess, hipMemUnmap, hipMemRelease, hipMemAddressFree) now cast CUresult to cudaError_t so callers can use a single hipError_t variable across runtime and driver API calls. - Fabric handle export/import error messages now include the specific failing call (e.g. "HIP Error in hipMemAddressReserve during fabric handle import") to make multi-rank failures diagnosable without re-running with extra logging. - Fix typo: "sharable" -> "shareable" in comment (matches HIP API name). Co-authored-by: Claude --- src/header/TransferBench.hpp | 38 ++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/header/TransferBench.hpp b/src/header/TransferBench.hpp index e719ed84..fce0d2b3 100644 --- a/src/header/TransferBench.hpp +++ b/src/header/TransferBench.hpp @@ -667,14 +667,15 @@ namespace TransferBench #define hipStreamSynchronize cudaStreamSynchronize #define hipMemGetAllocationGranularity cuMemGetAllocationGranularity #define hipMemCreate cuMemCreate - #define hipMemAddressReserve cuMemAddressReserve - #define hipMemMap cuMemMap - #define hipMemSetAccess cuMemSetAccess - #define hipMemUnmap cuMemUnmap - #define hipMemRelease cuMemRelease - #define hipMemAddressFree cuMemAddressFree - #define hipMemExportToShareableHandle cuMemExportToShareableHandle - #define hipMemImportFromShareableHandle cuMemImportFromShareableHandle + // cu* driver API returns CUresult; cast to cudaError_t so callers can use a single error variable + #define hipMemAddressReserve(...) ((cudaError_t)cuMemAddressReserve(__VA_ARGS__)) + #define hipMemMap(...) ((cudaError_t)cuMemMap(__VA_ARGS__)) + #define hipMemSetAccess(...) ((cudaError_t)cuMemSetAccess(__VA_ARGS__)) + #define hipMemUnmap(...) ((cudaError_t)cuMemUnmap(__VA_ARGS__)) + #define hipMemRelease(...) ((cudaError_t)cuMemRelease(__VA_ARGS__)) + #define hipMemAddressFree(...) ((cudaError_t)cuMemAddressFree(__VA_ARGS__)) + #define hipMemExportToShareableHandle(...) ((cudaError_t)cuMemExportToShareableHandle(__VA_ARGS__)) + #define hipMemImportFromShareableHandle(...) ((cudaError_t)cuMemImportFromShareableHandle(__VA_ARGS__)) using gpu_device_ptr = CUdeviceptr; @@ -3547,8 +3548,8 @@ static bool IsConfiguredGid(union ibv_gid const& gid) } System::Get().Broadcast(srcMemRank, sizeof(srcQpResult), &srcQpResult); if (srcQpResult.errType != ERR_NONE) { - return {ERR_FATAL, "SRC rank %d failed to transition QP %d to %s", srcMemRank, i, - srcQpResult.rtrFailed ? "RTR" : "RTS"}; + return {ERR_FATAL, "SRC rank %d failed to transition QP %d to %s", + srcMemRank, i, srcQpResult.rtrFailed ? "RTR" : "RTS"}; } QpTransitionResult dstQpResult = {ERR_NONE, false}; @@ -3562,8 +3563,8 @@ static bool IsConfiguredGid(union ibv_gid const& gid) } System::Get().Broadcast(dstMemRank, sizeof(dstQpResult), &dstQpResult); if (dstQpResult.errType != ERR_NONE) { - return {ERR_FATAL, "DST rank %d failed to transition QP %d to %s", dstMemRank, i, - dstQpResult.rtrFailed ? "RTR" : "RTS"}; + return {ERR_FATAL, "DST rank %d failed to transition QP %d to %s", + dstMemRank, i, dstQpResult.rtrFailed ? "RTR" : "RTS"}; } // Prepare scatter-gather element / work request for this queue pair in advance @@ -3954,13 +3955,15 @@ static bool IsConfiguredGid(union ibv_gid const& gid) // If pod communication is required, export/import fabric handle if (memDevice.memRank != exeDevice.exeRank && IsGpuExeType(exeDevice.exeType)) { #ifdef POD_COMM_ENABLED - // mem rank exports to sharable fabric handle; broadcast handle + status so all + // mem rank exports to shareable fabric handle; broadcast handle + status so all // ranks fail together instead of hanging on the next collective if export fails hipMemFabricHandle_t fabricHandle = {}; hipError_t exportErr = hipSuccess; + const char* exportStep = "hipSetDevice"; if (memDevice.memRank == GetRank()) { exportErr = hipSetDevice(memDevice.memIndex); if (exportErr == hipSuccess) { + exportStep = "hipMemExportToShareableHandle"; exportErr = hipMemExportToShareableHandle(&fabricHandle, *memHandle, hipMemHandleTypeFabric, 0); } } @@ -3968,23 +3971,28 @@ static bool IsConfiguredGid(union ibv_gid const& gid) System::Get().Broadcast(memDevice.memRank, sizeof(hipMemFabricHandle_t), &fabricHandle); System::Get().Broadcast(memDevice.memRank, sizeof(hipError_t), &exportErr); if (exportErr != hipSuccess) { - return {ERR_FATAL, "HIP Error during fabric handle export: %s", hipGetErrorString(exportErr)}; + return {ERR_FATAL, "HIP Error in %s during fabric handle export: %s", exportStep, hipGetErrorString(exportErr)}; } // exe rank imports the fabric handle; broadcast result so all ranks fail together hipError_t importErr = hipSuccess; + const char* importStep = "hipSetDevice"; if (exeDevice.exeRank == GetRank()) { importErr = hipSetDevice(exeDevice.exeIndex); if (importErr == hipSuccess) { + importStep = "hipMemImportFromShareableHandle"; importErr = hipMemImportFromShareableHandle(memHandle, (void*)&fabricHandle, hipMemHandleTypeFabric); } if (importErr == hipSuccess) { + importStep = "hipMemAddressReserve"; importErr = hipMemAddressReserve((gpu_device_ptr*)memPtr, *pActualBytes, 0, 0, 0); } if (importErr == hipSuccess) { + importStep = "hipMemMap"; importErr = hipMemMap((gpu_device_ptr)*memPtr, *pActualBytes, 0, *memHandle, 0); } if (importErr == hipSuccess) { + importStep = "hipMemSetAccess"; hipMemAccessDesc desc; desc.location = {hipMemLocationTypeDevice, exeDevice.exeIndex}; desc.flags = hipMemAccessFlagsProtReadWrite; @@ -3993,7 +4001,7 @@ static bool IsConfiguredGid(union ibv_gid const& gid) } System::Get().Broadcast(exeDevice.exeRank, sizeof(hipError_t), &importErr); if (importErr != hipSuccess) { - return {ERR_FATAL, "HIP Error during fabric handle import: %s", hipGetErrorString(importErr)}; + return {ERR_FATAL, "HIP Error in %s during fabric handle import: %s", importStep, hipGetErrorString(importErr)}; } #else return {ERR_FATAL, "Unable to export/import fabric handle without compiling with pod communication support"};