Repository navigation
UCT/ROCM_IPC: Advertise peer failure error handling capability - #11977
Conversation
|
🤖 Starting review — findings will be posted here when done. |
| iface_attr->ep_addr_len = 0; | ||
| iface_attr->max_conn_priv = 0; | ||
| iface_attr->cap.flags = UCT_IFACE_FLAG_GET_ZCOPY | | ||
| iface_attr->cap.flags = UCT_IFACE_FLAG_ERRHANDLE_PEER_FAILURE | |
There was a problem hiding this comment.
rocm_ipc never calls uct_iface_handle_ep_err() (see rocm_ipc_ep.c:95-160, rocm_signal.c:57-63) and returns UCS_ERR_INVALID_ADDR/UCS_ERR_IO_ERROR, which are not endpoint-level errors (UCS_IS_ENDPOINT_ERROR). Even without the fatal above, UCP would fail the individual request rather than mark the lane/EP failed, so the flag does not deliver peer-failure semantics. How is a dead peer expected to be detected and reported to UCP?
There was a problem hiding this comment.
wouldn't UCP's keepalive lane detect the dead peer?
| iface_attr->ep_addr_len = 0; | ||
| iface_attr->max_conn_priv = 0; | ||
| iface_attr->cap.flags = UCT_IFACE_FLAG_GET_ZCOPY | | ||
| iface_attr->cap.flags = UCT_IFACE_FLAG_ERRHANDLE_PEER_FAILURE | |
There was a problem hiding this comment.
rocm_ipc MD (rocm_ipc_md.c:33) reports only REG | NEED_RKEY | IPC_MEMTYPE_COPY, while cuda_ipc additionally reports INVALIDATE | INVALIDATE_RMA | INVALIDATE_AMO. With err_mode=peer, ucp_wireup_add_rma_bw_lanes() adds UCT_MD_FLAG_INVALIDATE_RMA to the local MD criteria (ucp/wireup/select.c:2278-2280), and rocm_ipc is CONNECT_TO_IFACE so the p2p exemption at select.c:570-574 does not apply. Will rocm_ipc actually be selected as an rndv/RMA lane with error handling, or is the iface flag alone not enough (i.e. the change may be a no-op for the intended use case)?
There was a problem hiding this comment.
added the required flags
|
Test/validation gap: nothing exercises rocm_ipc with a dead peer — |
|
🤖 Starting review — findings will be posted here when done. |
| iface_attr->ep_addr_len = 0; | ||
| iface_attr->max_conn_priv = 0; | ||
| iface_attr->cap.flags = UCT_IFACE_FLAG_GET_ZCOPY | | ||
| iface_attr->cap.flags = UCT_IFACE_FLAG_ERRHANDLE_PEER_FAILURE | |
There was a problem hiding this comment.
How does a dead peer reach the user's err_handler? Nothing in rocm/ipc calls uct_iface_handle_ep_err(), and the attach failure now returns UCS_ERR_INVALID_ADDR, which is not an endpoint error, so UCP aborts the request via ucp_proto_request_abort() and leaves the ep alive. With this flag set, wireup will pick rocm_ipc for RMA lanes in UCP_ERR_HANDLING_MODE_PEER.
|
Test/validation gap: no test exercises the newly advertised capability. Every candidate test is skipped for rocm_ipc: |
|
🤖 CI Triage Agent — TL;DR: The gtest run itself was 100% green (8707/8707 passed) — the job failed only because LeakSanitizer reported a single 40-byte leak allocated inside Full analysisSummary: Root cause: All tests passed ( Frame #1 is inside Implicated commit: unknown — no commit from this PR appears in the leak stack; the implicated code ( File: Suggested fix:
Related: PR #11977 (
|
## What? Set `ROCM_IPC_MIN_ZCOPY=0` in the UCX context NIXL creates. ## Why? `rocm_ipc` defaults to a 128 byte minimum zero-copy size (openucx/ucx#9568, for small-message MPI collectives). UCP put/get zero-copy protocols reject any lane with a non-zero minimum, so NIXL VRAM to VRAM transfers on AMD never use `rocm_ipc` (XGMI) and are staged through host memory instead. `cuda_ipc` already uses 0. Setting it only in NIXL's UCX context leaves MPI and other UCX users unchanged. A user-set `UCX_ROCM_IPC_MIN_ZCOPY` still wins. On UCX builds without ROCm support the setting is unknown and is ignored with a debug log. With NIXL's default peer error handling, `rocm_ipc` is selected only once it advertises peer-failure support (openucx/ucx#11977, open). This PR does not depend on it, but the full benefit needs both. ## Results MI355X, NIXL WRITE GPU0 to GPU4, UCX master + openucx/ucx#11977: | size | without this PR | with this PR | |---|---|---| | 64 KiB | 1.68 GB/s | 4.54 GB/s | | 512 KiB | 11.89 GB/s | 23.74 GB/s | | 2 MiB | 30.05 GB/s | 43.69 GB/s | | 128 MiB | 60.22 GB/s | 60.85 GB/s | UCX protocol selection (`UCX_PROTO_INFO=y`) changes from a rendezvous read to direct `rocm_ipc` zero-copy for all sizes. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Updated ROCm IPC zero-copy settings during UCX initialization. * Added a warning when AMD GPUs are detected but UCX lacks ROCm memory-type support, helping identify a configuration that may affect GPU communication. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Colin Hirsch <chirsch@nvidia.com>
|
🤖 CI Triage Agent — TL;DR: The ASAN-GPU gtest run failed on a single test, Full analysisSummary: Root cause: Evidence from the log: Implicated commit: Not this PR. The assertion and its remote-provenance model come from the CUDA staging-force series — File: Suggested fix: Re-run the "AddressSanitizer gpu on worker 3" job to confirm the failure is not reproducible with this PR; it should not block #11977. For a durable fix, make the test tolerant of hosts where Related: PR #11977 (the PR under test), #11990 (UCP/CORE: Ignore DPU in topology assignments), #11685 (remote system device namespace isolation), and the diagnostic PRs opened specifically against this test: #11858, #11861, #11864.
|
|
🤖 Starting review — findings will be posted here when done. |
| uct_md_base_md_query(md_attr); | ||
| md_attr->rkey_packed_size = sizeof(uct_rocm_ipc_key_t); | ||
| md_attr->flags = UCT_MD_FLAG_REG | UCT_MD_FLAG_NEED_RKEY | | ||
| UCT_MD_FLAG_INVALIDATE | |
There was a problem hiding this comment.
uct_rocm_ipc_mem_dereg() still passes 0 as _invalidate_supported and ignores UCT_MD_MEM_DEREG_FLAG_INVALIDATE. With these flags advertised, rocm_ipc gets into memh->inv_md_map and ucp_memh_dereg() passes that flag, so with params check enabled dereg returns UCS_ERR_UNSUPPORTED before ucs_free(mem_hndl) (key leak), and otherwise the invalidation is skipped and comp is never invoked. How is invalidation supposed to work for rocm_ipc?
|
Residual coverage gap: no new test exercises the newly advertised capability, and nothing in the diff covers the |
- Advertise peer failure error handling capability - Return errors instead of aborting on IPC cache failures, add ep_check stub - Add memory invalidation MD flags
a345a98 to
1cf9701
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The ASAN/RoCE gtest shard hung on its very first test, Full analysisSummary: Root cause: Hang, not slowness. The log shows a single 877-second gap with zero output between Crucially, the test under which this happened uses host memory over Implicated commit: unknown (the PR commit File: test/gtest/ucp/test_ucp_am.cc:542 (assertion site, Suggested fix: Re-run the "AddressSanitizer roce on worker 0" job; this is not a code defect introduced by the PR. If it reproduces on the same agent, treat it as node health: check Related: PR #11977 (the PR under test, ROCm IPC peer-failure capability — unrelated to the hang). No existing issue found matching |
edgargabriel
left a comment
There was a problem hiding this comment.
I approve the PR as is. From the error handling capability, the commit matches the rocm_ipc TL with the cuda_ipc. Neither one truly handles every situations correctly.
The flag is however required for the rocm_ipc component to be used from NIXL, and the PR adds some base capabilities. Anything beyond the PR can be addressed at a later stage if necessary,
…-v1.23.x UCT/ROCM_IPC: Add peer failure error handling - v1.23.x (#11977)
Port of vllm-project#62827 to this branch's Dockerfile.rocm: - UCX_BRANCH -> 2ff0b47c (v1.23.0-rc1), which adds peer error handling to rocm_ipc (openucx/ucx#11977), so NIXL selects it for GPU-to-GPU reads instead of the TCP fallback or the host-staged pipeline. - Drop UCX_RMA_PPLN_ENABLE=y, which is no longer needed. - Set UCX_ROCM_IPC_SIGPOOL_MAX_ELEMS=inf, matching rocm_copy: each in-flight rocm_ipc copy holds one HSA signal. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andreas Karatzas <AndreasKaratzas@users.noreply.github.com>
What?
Advertises peer failure error handling capability
Why?
How?