Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 45 additions & 8 deletions cpp/tensorrt_llm/common/ncclUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,20 +57,33 @@ struct CudaMallocGuard
CudaMallocGuard& operator=(CudaMallocGuard const&) = delete;
};

// RAII guard for ncclMemAlloc. Frees the pointer on destruction, logging a warning on failure.
// Roll back a window before freeing its allocation; failed deregistration retains the storage.
struct NcclMemGuard
{
void* ptr{nullptr};
ncclComm_t comm;
ncclWindow_t window{nullptr};

explicit NcclMemGuard(void* p) noexcept
NcclMemGuard(void* p, ncclComm_t c) noexcept
: ptr(p)
, comm(c)
{
}

~NcclMemGuard()
{
if (ptr)
{
if (window)
{
auto const result = ncclCommWindowDeregister(comm, window);
TLLM_NCCL_CHECK_WARN(result);
if (result != ncclSuccess)
{
TLLM_LOG_WARNING("[NCCLUtil] Retaining allocation after failed window rollback.");
return;
}
}
TLLM_NCCL_CHECK_WARN(ncclMemFree(ptr));
}
}
Expand Down Expand Up @@ -981,12 +994,18 @@ void NCCLWindowAllocator::quarantineTensorLeaseLocked(TensorLease const& tensorL
entry.graphPoolOwner.reset();
entry.releaseOwnerWhenUnused = false;
}
TLLM_LOG_DEBUG("[NCCLUtil] Quarantining NCCL window lease %llu on comm %p after a failed tensor lease scope.",
TLLM_LOG_DEBUG("[NCCLUtil] Quarantining NCCL window lease %llu on comm %p.",
static_cast<unsigned long long>(tensorLease.leaseId), static_cast<void*>(tensorLease.comm));
return;
}
}

void NCCLWindowAllocator::abandonReservation(ncclComm_t comm, void* ptr, uint64_t leaseId)
{
std::lock_guard<std::mutex> lock(mMutex);
quarantineTensorLeaseLocked(TensorLease{comm, ptr, leaseId});
}

void NCCLWindowAllocator::releaseBufferFromDestructor(ncclComm_t comm, void* ptr, uint64_t leaseId)
{
if (!comm || !ptr || leaseId == 0)
Expand Down Expand Up @@ -1123,7 +1142,7 @@ bool NCCLWindowAllocator::isCommValid(ncclComm_t comm) const noexcept
}

NCCLWindowBuffer NCCLWindowAllocator::allocateAndRegisterBuffer(
ncclComm_t comm, size_t size, int handle, cudaStream_t stream)
ncclComm_t comm, size_t size, int handle, cudaStream_t stream, WindowRegisterFunc registerWindow)
{
// Step 1: Pre-allocate the rank-sync flag before ncclMemAlloc. ncclMemAlloc can fail
// asymmetrically with ncclUnhandledCudaError on configurations where the symmetric/VMM path
Expand All @@ -1143,7 +1162,7 @@ NCCLWindowBuffer NCCLWindowAllocator::allocateAndRegisterBuffer(
void* ncclPtr = nullptr;
TLLM_NCCL_CHECK_WARN(ncclMemAlloc(&ncclPtr, size));
int const localAllocOk = (ncclPtr != nullptr) ? 1 : 0;
NcclMemGuard ncclGuard{ncclPtr}; // frees ncclPtr on any early return or exception
NcclMemGuard ncclGuard{ncclPtr, comm}; // frees ncclPtr on any early return or exception
clearCudaErrorIfSymmetricAllocationFailed(localAllocOk);

// Step 3: ncclCommWindowRegister is collective. If any rank skips it, all other ranks hang.
Expand Down Expand Up @@ -1176,12 +1195,30 @@ NCCLWindowBuffer NCCLWindowAllocator::allocateAndRegisterBuffer(

// Step 4: Register with NCCL as a window. This is collective, so all ranks must reach it.
// Failure here is non-fatal: warn and fall back to regular allreduce.
// ncclGuard frees ncclPtr on return.
// Initialize the vote before registration can leave a CUDA error behind.
TLLM_CUDA_CHECK(cudaMemsetAsync(rankSyncFlag, 0, sizeof(int), stream));
ncclWindow_t window = nullptr;
ncclResult_t const regResult = ncclCommWindowRegister(comm, ncclPtr, size, &window, NCCL_WIN_COLL_SYMMETRIC);
ncclResult_t const regResult = registerWindow(comm, ncclPtr, size, &window, NCCL_WIN_COLL_SYMMETRIC);
ncclGuard.window = window;
TLLM_NCCL_CHECK_WARN(regResult);
if (regResult != ncclSuccess)
int const localRegisterOk = (regResult == ncclSuccess && window != nullptr) ? 1 : 0;
// A collective registration call does not establish identical local outcomes.
// Agree before adding any buffer to the pool, including success with a null window.
clearCudaErrorIfSymmetricAllocationFailed(localRegisterOk);
if (localRegisterOk)
{
TLLM_CUDA_CHECK_WARN(
cudaMemcpyAsync(rankSyncFlag, &localRegisterOk, sizeof(int), cudaMemcpyHostToDevice, stream));
}
TLLM_NCCL_CHECK(ncclAllReduce(rankSyncFlag, rankSyncFlag, 1, ncclInt32, ncclMin, comm, stream));
TLLM_CUDA_CHECK(cudaStreamSynchronize(stream));
int allRegisterOk = 0;
TLLM_CUDA_CHECK(cudaMemcpy(&allRegisterOk, rankSyncFlag, sizeof(int), cudaMemcpyDeviceToHost));
if (!allRegisterOk)
{
// No caller has seen these windows. Local deregistration rolls back the
// successful registrations before their allocations are freed by ncclGuard.
TLLM_LOG_WARNING("[NCCLUtil] Window registration unavailable on at least one rank; falling back on all ranks.");
return NCCLWindowBuffer{};
}

Expand Down
7 changes: 6 additions & 1 deletion cpp/tensorrt_llm/common/ncclUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,9 @@ class NCCLWindowAllocator
// teardown because destructor timing and stream identity are not rank-deterministic.
void releaseBufferFromDestructor(ncclComm_t comm, void* ptr, uint64_t leaseId);

// Explicitly quarantine a raw reservation until communicator teardown. Never makes it reusable.
void abandonReservation(ncclComm_t comm, void* ptr, uint64_t leaseId);

// Get the window handle for a specific buffer pointer
ncclWindow_t getWindow(ncclComm_t comm, void* ptr) const;

Expand Down Expand Up @@ -322,7 +325,9 @@ class NCCLWindowAllocator
~NCCLWindowAllocator() = default;

// Allocate a new buffer and register it with NCCL as a window
NCCLWindowBuffer allocateAndRegisterBuffer(ncclComm_t comm, size_t size, int handle, cudaStream_t stream);
using WindowRegisterFunc = ncclResult_t (*)(ncclComm_t, void*, size_t, ncclWindow_t*, int);
NCCLWindowBuffer allocateAndRegisterBuffer(ncclComm_t comm, size_t size, int handle, cudaStream_t stream,
WindowRegisterFunc registerWindow = ncclCommWindowRegister);

// Record a failed new symmetric allocation (assumes mMutex is already locked).
void recordSymmetricFailureLocked(ncclComm_t comm, size_t size);
Expand Down
104 changes: 104 additions & 0 deletions cpp/tensorrt_llm/thop/allocateOutput.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@

#include "tensorrt_llm/thop/outputTensor.h"

#include <c10/cuda/CUDAGuard.h>
#include <torch/custom_class.h>
#include <torch/extension.h>

#include <vector>
Expand All @@ -37,6 +39,102 @@ TRTLLM_NAMESPACE_BEGIN
namespace torch_ext
{

//! A context-owned checkout, independent of operation-scoped tensor leases.
//! Construction and release must occur in identical rank order outside capture.
//! Before use, callers must drain prior users on every rank. Before release,
//! callers must drain all users on every rank and retire graphs using this tensor.
class NcclWindowReservation : public torch::CustomClassHolder
{
public:
NcclWindowReservation(at::Tensor like, std::vector<int64_t> group)
: mTensor(like)
{
TORCH_CHECK(like.is_cuda(), "NCCL window reservations require a CUDA tensor");
c10::cuda::CUDAGuard const guard(like.device());
cudaStreamCaptureStatus status{};
TLLM_CUDA_CHECK(cudaStreamIsCapturing(at::cuda::getCurrentCUDAStream().stream(), &status));
TORCH_CHECK(status == cudaStreamCaptureStatusNone, "Reserve NCCL windows before CUDA graph capture");
#if ENABLE_MULTI_DEVICE
std::set<int> const ranks(group.begin(), group.end());
mComm = getComm(ranks);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nice to have] Question: the reservation registers its window on TRT-LLM's getComm(moe_ep_group) communicator, but under MPI the NCCL-EP group is created on its own nccl4py communicator from a fresh unique id (nccl_ep_utils.py:225-234, 257). Window registration is per communicator, so why is this handle valid when passed to that group for dispatch, and now for combine, CUDA graphs and FP8/NVFP4? Could you state the reason in the NCCL-EP section of MOE_DEVELOPER_GUIDE.md, or register on the group's own communicator if it does not hold?

if (!mComm || !*mComm)
{
return;
}
mBuffer = common::nccl_util::NCCLWindowAllocator::getInstance().requestBuffer(
*mComm, like.nbytes(), at::cuda::getCurrentCUDAStream().stream());
if (mBuffer.isValid())
{
// Deliberately do not register a temporary tensor lease. The reservation
// owns the checkout; tensor views cannot transfer it to a module scope.
mTensor = torch::from_blob(mBuffer.ptr, like.sizes(), [comm = mComm](void*){}, like.options());
}
#endif
}

~NcclWindowReservation() override
{
#if ENABLE_MULTI_DEVICE
if (mBuffer.isValid())
{
// Missing explicit release must never make storage eligible via GC timing.
common::nccl_util::NCCLWindowAllocator::getInstance().releaseBufferFromDestructor(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Blocker] If a reservation is destroyed without release(), this goes through releaseBufferFromDestructor() -> assertLifecycleWarning() -> std::abort() (TLLM_NCCL_WINDOW_LIFECYCLE_ASSERT defaults to 1; only the C++ unit tests suppress it). Since the reservation registers no tensor lease, any NcclEpContext still alive at interpreter exit aborts the process, e.g.:

  • examples/layer_wise_benchmarks with NCCL-EP (it never destroys comms, and destroy_all_nccl_ep_contexts() has no callers);
  • an executor that fails after the first MoE forward (warmup runs inside PyExecutor.__init__ without cleanup);
  • non-leader ranks under the RPC orchestrator;
  • an exception from the sync / Barrier() in NcclEpContext.destroy(), which runs before release() with no try/finally (and release_nccl_ep_context() has already popped the context).

These used to exit cleanly; now they end in SIGABRT / core dumps on top of the real error. Could the destructor quarantine with a warning only (no lifecycle assert) for reservations, and destroy() release or abandon the reservation in a finally?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for flagging the construction failure path. The destructor assertion is intentional: silently abandoning there would hide missing lifecycle cleanup from CI.

We added explicit constructor rollback instead. After reservation setup, ranks exchange initialization status through MPI or the EP process group. If any rank fails, all ranks abandon their reservations; the failing rank preserves its original exception, and peers report the construction failure.
Successful initialization transfers ownership normally.

Validated with two-GPU tests injecting allocation and descriptor failures on one rank, for both communication paths, with lifecycle assertions enabled. This addresses constructor failures after reservation setup; callers still need to explicitly destroy successfully constructed contexts.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor-failure portion is covered by the ExitStack rollback guard and _construction_rendezvous(): failures after reservation setup abandon the reservation, exchange initialization status, and preserve the local exception. The failure-injection contract tests cover local/peer failure and status-exchange errors.

Leaving this open because that does not cover every case in the original comment. The reservation destructor still uses releaseBufferFromDestructor(), so a successfully constructed context discarded without explicit release/abandon can still hit the lifecycle assertion. There is also no general finally-based abandonment around synchronization/native teardown. The earlier constructor-rollback reply should not be read as resolving those remaining cases.

*mComm, mBuffer.ptr, mBuffer.leaseId);
}
#endif
}

//! Quarantine without CUDA or collective calls; retain registration until communicator teardown.
void abandon()
{
#if ENABLE_MULTI_DEVICE
if (mBuffer.isValid())
{
common::nccl_util::NCCLWindowAllocator::getInstance().abandonReservation(
*mComm, mBuffer.ptr, mBuffer.leaseId);
mBuffer = {};
}
#endif
}

at::Tensor getTensor() const
{
return mTensor;
}

//! Return the raw ncclWindow_t encoded as an integer, or 0 on a reservation miss.
int64_t getWindow() const
{
#if ENABLE_MULTI_DEVICE
return static_cast<int64_t>(reinterpret_cast<uintptr_t>(mBuffer.window));
#else
return 0;
#endif
}

//! Release after the caller's collective quiescence boundary; idempotent.
void release()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nice to have] The class comment requires release outside capture (allocateOutput.cpp:43), but only the constructor checks it (:54-56). A release() issued during capture would mark the buffer reusable while the graph still references it instead of failing fast; add the same cudaStreamIsCapturing check here. Relatedly, a supplied combine window that fails the identity check (communication/nccl_ep.py:492-506) and a non-BF16 output_dtype (nccl_ep.py:399-401) both fall back silently; a one-time debug log would make a disabled window path visible.

{
#if ENABLE_MULTI_DEVICE
if (mBuffer.isValid())
{
c10::cuda::CUDAGuard const guard(mTensor.device());
common::nccl_util::NCCLWindowAllocator::getInstance().releaseBuffer(
*mComm, mBuffer.ptr, at::cuda::getCurrentCUDAStream().stream());
mBuffer = {};
}
#endif
mTensor = at::Tensor();
}

private:
at::Tensor mTensor;
#if ENABLE_MULTI_DEVICE
std::shared_ptr<ncclComm_t> mComm;
common::nccl_util::NCCLWindowBuffer mBuffer{};
#endif
};

std::tuple<at::Tensor, int64_t> allocateOutputOp(at::Tensor const& like, int64_t output_buffer_kind,
c10::optional<torch::List<int64_t>> group, c10::optional<at::IntArrayRef> shape,
c10::optional<at::ScalarType> out_dtype)
Expand Down Expand Up @@ -93,6 +191,12 @@ TRTLLM_NAMESPACE_END

TORCH_LIBRARY_FRAGMENT(trtllm, m)
{
m.class_<tensorrt_llm::torch_ext::NcclWindowReservation>("NCCLWindowReservation")
.def(torch::init<at::Tensor, std::vector<int64_t>>())
.def("tensor", &tensorrt_llm::torch_ext::NcclWindowReservation::getTensor)
.def("window", &tensorrt_llm::torch_ext::NcclWindowReservation::getWindow)
.def("abandon", &tensorrt_llm::torch_ext::NcclWindowReservation::abandon)
.def("release", &tensorrt_llm::torch_ext::NcclWindowReservation::release);
m.def(
"allocate_output(Tensor like, int output_buffer_kind, int[]? group, "
"int[]? shape=None, ScalarType? out_dtype=None) -> (Tensor, int)");
Expand Down
52 changes: 52 additions & 0 deletions cpp/tests/unit_tests/multi_gpu/ncclUtilsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ namespace tensorrt_llm::common::nccl_util
class NCCLWindowAllocatorTestAccess
{
public:
static NCCLWindowBuffer allocateWithRegistration(
NCCLWindowAllocator& allocator, ncclComm_t comm, NCCLWindowAllocator::WindowRegisterFunc registerWindow)
{
return allocator.allocateAndRegisterBuffer(comm, 1024 * 1024, 0, nullptr, registerWindow);
}

static void recordSymmetricFailure(NCCLWindowAllocator& allocator, ncclComm_t comm, size_t size)
{
std::lock_guard<std::mutex> lock(allocator.mMutex);
Expand Down Expand Up @@ -467,6 +473,52 @@ TEST_F(NCCLWindowAllocatorTest, BasicAllocation)
allocator.releaseBuffer(*mComm, buffer.ptr);
}

TEST_F(NCCLWindowAllocatorTest, AsymmetricRegistrationFallsBackOnEveryRank)
{
auto& allocator = nccl_util::NCCLWindowAllocator::getInstance();
// Both ranks enter real registration; only rank zero reports an unavailable window.
// Exercise both an explicit error and NCCL's successful-but-null result.
for (bool const nullSuccess : {false, true})
{
auto const registerWithError = [](ncclComm_t comm, void* ptr, size_t size, ncclWindow_t* window, int flags)
{
auto const result = ncclCommWindowRegister(comm, ptr, size, window, flags);
if (result == ncclSuccess && mpi::MpiComm::world().getRank() == 0)
{
TLLM_NCCL_CHECK(ncclCommWindowDeregister(comm, *window));
*window = nullptr;
return ncclSystemError;
}
return result;
};
auto const registerWithNull = [](ncclComm_t comm, void* ptr, size_t size, ncclWindow_t* window, int flags)
{
auto const result = ncclCommWindowRegister(comm, ptr, size, window, flags);
if (result == ncclSuccess && mpi::MpiComm::world().getRank() == 0)
{
TLLM_NCCL_CHECK(ncclCommWindowDeregister(comm, *window));
*window = nullptr;
}
return result;
};
auto const buffer = nccl_util::NCCLWindowAllocatorTestAccess::allocateWithRegistration(
allocator, *mComm, nullSuccess ? +registerWithNull : +registerWithError);
EXPECT_FALSE(buffer.isValid());
if (buffer.isValid())
{
TLLM_NCCL_CHECK(ncclCommWindowDeregister(*mComm, buffer.window));
TLLM_NCCL_CHECK(ncclMemFree(buffer.ptr));
}
}
// Rollback must leave the communicator usable for a subsequent pooled allocation.
auto const buffer = allocator.requestBuffer(*mComm, 1024 * 1024);
EXPECT_TRUE(buffer.isValid());
if (buffer.isValid())
{
allocator.releaseBuffer(*mComm, buffer.ptr);
}
}

TEST_F(NCCLWindowAllocatorTest, ExplicitStreamAllocationAndCapture)
{
auto& allocator = nccl_util::NCCLWindowAllocator::getInstance();
Expand Down
Loading
Loading