[https://nvbugs/6278399][fix] Add x86_64 path using CU_MEM_HANDLE_TYPE_POSIX_FILE_DESCRIPTOR with… - #15129
Conversation
📝 WalkthroughWalkthroughThe PR extends DWDP transport to support POSIX file descriptor handles alongside FABRIC handles, with architecture-aware type selection and comprehensive FD lifecycle management including pidfd-based duplication, allocation tracking, and explicit cleanup on success and failure paths. ChangesDWDP POSIX File Descriptor Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tensorrt_llm/_torch/modules/dwdp/transport.py (2)
74-81: 💤 Low valueClarify comment about syscall number applicability.
The comment states these syscall numbers are shared by x86_64/aarch64, but the POSIX_FD code path is never taken on aarch64 (since
peer_handle_type()returns FABRIC there). Consider updating the comment to clarify that these syscalls are only exercised on x86_64.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/_torch/modules/dwdp/transport.py` around lines 74 - 81, Comment clarification: update the block around _SYS_pidfd_open and _SYS_pidfd_getfd to state these syscall numbers are defined for both x86_64 and aarch64 but are only exercised on x86_64 because peer_handle_type() returns FABRIC on aarch64, so the POSIX_FILE_DESCRIPTOR path (and therefore pidfd_open/pidfd_getfd) is not taken on aarch64; mention POSIX_FILE_DESCRIPTOR, FABRIC, and functions/classes MnnvlMemory.open_mnnvl_memory and peer_handle_type() to help locate the relevant logic and make the platform-specific behavior explicit.
188-189: 💤 Low valueMissing docstring entries for new parameters.
The constructor docstring (lines 191-207) doesn't document
local_export_fdsandimported_local_fds. Consider adding entries describing their purpose for completeness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/_torch/modules/dwdp/transport.py` around lines 188 - 189, The __init__ constructor in tensorrt_llm._torch.modules.dwdp.transport (the class taking parameters local_export_fds and imported_local_fds) is missing docstring entries for these two new parameters; update the constructor docstring to add clear descriptions for local_export_fds (e.g., a list of file descriptor integers that this side will export/share for local IPC) and imported_local_fds (e.g., a list of file descriptor integers imported from the peer to be used locally), including expected types and semantics so the purpose and usage of each parameter is documented alongside the existing parameter docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tensorrt_llm/_torch/modules/dwdp/transport.py`:
- Around line 74-81: Comment clarification: update the block around
_SYS_pidfd_open and _SYS_pidfd_getfd to state these syscall numbers are defined
for both x86_64 and aarch64 but are only exercised on x86_64 because
peer_handle_type() returns FABRIC on aarch64, so the POSIX_FILE_DESCRIPTOR path
(and therefore pidfd_open/pidfd_getfd) is not taken on aarch64; mention
POSIX_FILE_DESCRIPTOR, FABRIC, and functions/classes
MnnvlMemory.open_mnnvl_memory and peer_handle_type() to help locate the relevant
logic and make the platform-specific behavior explicit.
- Around line 188-189: The __init__ constructor in
tensorrt_llm._torch.modules.dwdp.transport (the class taking parameters
local_export_fds and imported_local_fds) is missing docstring entries for these
two new parameters; update the constructor docstring to add clear descriptions
for local_export_fds (e.g., a list of file descriptor integers that this side
will export/share for local IPC) and imported_local_fds (e.g., a list of file
descriptor integers imported from the peer to be used locally), including
expected types and semantics so the purpose and usage of each parameter is
documented alongside the existing parameter docs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f10d5c08-e89e-4c28-bcd5-6834f547e61d
📒 Files selected for processing (3)
tensorrt_llm/_torch/modules/dwdp/transport.pytensorrt_llm/_torch/modules/dwdp/vmm.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
|
[blocking] POSIX_FD path holds too many file descriptors — risks On the POSIX_FD path both
For a DSv3-class model (~58 MoE layers × several weights) at Per the CUDA driver semantics, neither needs to be held that long:
Requested changes:
This drops peak fd usage from (Note: this is entirely on the x86 POSIX_FD path — |
|
[maintainability] Two independent arch checks decide the handle type — they can silently diverge The peer-shareable handle type is currently decided in two places:
Both branch on Requested change: make (Resolved type is identical on aarch64 either way — |
DWDPTransport unconditionally used CU_MEM_HANDLE_TYPE_FABRIC for the peer-shareable expert weight handles. On x86_64 hosts without an IMEX channel (any non-GB200 Blackwell box, including the QA's B200 cluster), cuMemCreate(FABRIC) is denied with CUDA_ERROR_NOT_PERMITTED (800) and executor init aborts. Mirror the existing arch-aware pattern from MnnvlMemory in tensorrt_llm/_mnnvl_utils.py: - aarch64 (GB200): keep CU_MEM_HANDLE_TYPE_FABRIC. - x86_64: use CU_MEM_HANDLE_TYPE_POSIX_FILE_DESCRIPTOR; transfer the FDs between sibling DWDP MPI workers via pidfd_open + pidfd_getfd. DWDP semantics (composite VA, peer_views, scatter, prefetch) are preserved -- only the IPC primitive used to transfer the handle changes. The peer expert memory remains P2P-mappable across sibling processes. Also remove the now-obsolete nvbugs/6276923 SKIP entries for TestDwdpDeepSeekV3Lite tests in waives.txt. Verified: GSM8K accuracy 63.99 (threshold 61.54) on 4xB200 x86_64. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
…-time fd usage Previously both `imported_local_fds` and `local_export_fds` were retained until `release()`, peaking at roughly `dwdp_size × num_handles` open FDs during setup -- enough to exceed the default `ulimit -n` of 1024 on a DSv3-class model at `dwdp_size=8`. Per the CUDA driver semantics, neither needs to live that long: - `cuMemImportFromShareableHandle(POSIX_FILE_DESCRIPTOR)` dups the FD internally, so the local copy can be closed immediately after the import call returns. Drop the `imported_local_fds` list entirely and close inline in Phase 2. - The exported FD only needs to outlive every peer's `pidfd_getfd`, i.e. until the Phase 3 `comm.Barrier()`. Close `local_export_fds` right after the Barrier (alongside the existing pidfd cleanup) instead of at `release()`. Peak FD usage drops from ~`dwdp_size × num_handles` to a transient ~`num_handles`. The error-path caller of `_cleanup_resources` still defensively closes whatever FDs are tracked at the moment of failure -- it just sees an empty list on the success path. Also fix the now-stale docstring in `_cleanup_resources" that claimed imported FDs must "stay open as long as that handle is in use" -- not true for POSIX_FD (the imported CUDA handle is independent of the FD once `cuMemImportFromShareableHandle` returns). Behavior on aarch64 / FABRIC is unchanged (`is_posix_fd` is `False`, so `local_export_fds` stays empty and no inline close runs). Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
…gh peer_handle_type() `vmm.py::get_allocation_prop(fabric_only=False)` and `vmm.py::peer_handle_type()` both used to encode the same arch -> `CUmemAllocationHandleType` table independently. They agreed today (both branched on `platform.machine() == "aarch64"`) but a future change to one and not the other would create a handle of one type and then try to export it as another -- a runtime failure instead of a review-time catch. Make the non-`fabric_only` branch of `get_allocation_prop` delegate to `peer_handle_type()` so there is exactly one place that maps arch to handle type. Resolved values are unchanged on every supported arch (aarch64 -> FABRIC, x86_64 -> POSIX_FILE_DESCRIPTOR), so this is a no-op on GB200/FABRIC and only guards the x86 path against a future regression. The granularity-query path (`fabric_only=True`) is untouched -- the handle type there is never actually consumed. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
b071c0b to
e6c8892
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #53028 [ run ] triggered by Bot. Commit: |
|
PR_Github #53028 [ run ] completed with state
|
|
/bot run --disable-fail-fast --extra-stage "GB200-4_GPUs-PyTorch-1, GB200-4_GPUs-PyTorch-2" |
|
PR_Github #53073 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast --extra-stage "GB200-4_GPUs-PyTorch-1, GB200-4_GPUs-PyTorch-2" |
|
PR_Github #53223 [ run ] triggered by Bot. Commit: |
|
PR_Github #53073 [ run ] completed with state |
|
PR_Github #53223 [ run ] completed with state
|
|
/bot run --disable-fail-fast --extra-stage "GB200-4_GPUs-PyTorch-1, GB200-4_GPUs-PyTorch-2" |
|
PR_Github #53282 [ run ] triggered by Bot. Commit: |
|
PR_Github #53282 [ run ] completed with state |
4 participants
Summary
Test plan
Links
Summary by CodeRabbit
Bug Fixes
Chores