Repository navigation
UCT/CUDA/COPY: Qualify cuda_copy for RNDV with peer-failure EH - #11319
Conversation
eec6960 to
4fd4eb4
Compare
| if (ep_init_flags & UCP_EP_INIT_ERR_MODE_PEER_FAILURE) { | ||
| /* No independent remote worker when connecting an EP to itself (loopback, | ||
| * memtype EPs, etc.): peer-failure iface caps and MD invalidation are | ||
| * relaxed in wireup; see ucp_request_get_invalidation_map(). */ | ||
| if ((unpacked_addr != NULL) && (unpacked_addr->uuid == worker->uuid)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
IMO it would be better to update ucp_ep_err_mode_init_flags to not set UCP_EP_INIT_ERR_MODE_PEER_FAILURE for uuid==remote_uuid case
There was a problem hiding this comment.
Updated. For UCP_ERR_HANDLING_MODE_PEER, ucp_ep_err_mode_init_flags() now omits UCP_EP_INIT_ERR_MODE_PEER_FAILURE when the remote UUID matches the local worker UUID. The effective flags are applied only during lane selection, so the endpoint configuration still retains UCP_ERR_HANDLING_MODE_PEER.
I intentionally kept the full requirements for FAILOVER: removing peer-failure handling there makes the PSN failover protocol unavailable for same-UUID endpoints. Added same-process wireup and optional-RMA-invalidation coverage as well.
|
🤖 CI Triage Agent — TL;DR: Both codestyle jobs ("AUTHORS file update check" and "commit title") failed because the PR commit range Full analysisSummary: Root cause: The step computes Implicated commit: File: Suggested fix:
Related: #11527 (introduced the current range computation), #11319 (this PR)
|
|
ok from my side. |
|
@rakhmets please review |
Advertise UCT_IFACE_FLAG_ERRHANDLE_PEER_FAILURE so ucp_wireup_fill_peer_err_criteria does not exclude cuda_copy from RMA BW lanes. Advertise UCT_MD_FLAG_INVALIDATE and UCT_MD_FLAG_INVALIDATE_RMA and handle UCT_MD_MEM_DEREG_FLAG_INVALIDATE in mem dereg (invoke completion), matching ucp_wireup_add_rma_bw_lanes and ucp_memh_dereg expectations.
When an endpoint is wired to the same UCP worker (same unpacked address UUID), there is no independent remote peer for cross-worker RMA. Skip requiring UCT_IFACE_FLAG_ERRHANDLE_PEER_FAILURE and UCT_MD_FLAG_INVALIDATE_RMA in RMA BW wireup criteria in that case. In ucp_request_get_invalidation_map(), return an empty invalidation map for UCP_EP_CONFIG_KEY_FLAG_SELF so RMA BW lanes without MD invalidate support (e.g. cuda_copy) remain valid for same-worker / memtype EPs with peer-failure error handling. Extend ucp_wireup_fill_peer_err_criteria() and ucp_wireup_fill_aux_criteria() with worker + unpacked address so auxiliary and other lane selection paths apply the same rule.
Derive effective wireup flags once from the requested error mode and\nthe remote worker UUID. Same-worker endpoints no longer require peer\nfailure or RMA invalidation transport capabilities, while their endpoint\nconfiguration retains the requested error mode.\n\nAdd self-transport coverage for peer error handling.
e2aa013 to
7c04647
Compare
|
I have now only rebased to resolve the codestyle errors, there were no code changes. |
|
@rakhmets anything to do here? |
|
🤖 Starting review — findings will be posted here when done. |
|
Question on self + FAILOVER: |
|
Residual coverage gap (noted in an earlier round): there is still no test that runs traffic and triggers error handling on a self PEER endpoint, so the |
|
🤖 Starting review — findings will be posted here when done. |
| key->dst_md_cmpts = dst_md_storage; | ||
|
|
||
| status = ucp_wireup_select_lanes(ep, ep_init_flags, *tl_bitmap, | ||
| select_ep_init_flags = ep_init_flags; |
There was a problem hiding this comment.
this repeats the address->uuid == worker->uuid check that ucp_wireup_select_lanes() already does to set UCP_EP_CONFIG_KEY_FLAG_SELF, and it only covers the worker-address path; wireup_cm.c:212 calls ucp_wireup_select_lanes() directly and keeps the peer-failure requirement, while the ucp_request.c change already treats such EPs as "no invalidation needed". Can we relax the flag inside ucp_wireup_select_lanes() so the CM path behaves the same?
There was a problem hiding this comment.
Not moving the relaxation into ucp_wireup_select_lanes(). Its CM caller constructs an intermediate client configuration from a locally packed dummy address before the actual peer address is known, that address has the local UUID by construction. Applying the relaxation there would incorrectly weaken peer-failure requirements for cross-worker CM EPs. The current check is intentionally limited to worker-address selection, where the UUID is the actual peer’s UUID.
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 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: unknown (not PR #11319's cuda_copy change; suspect area is the interval-tree/PSN multi-fragment AM assembly introduced/modified by ef265cd "Multi Eager PSN Protocol" and 2bafbb0 "AM/FT: resend flag for first fragment retransmissions", both by Evgeny Leksikov / shasson5) File: test/gtest/ucp/test_ucp_am.cc:1710 ( Suggested fix: Re-trigger the ASAN GPU job to confirm the failure is unrelated to this PR (expect it to pass; the PR touches only cuda_copy iface caps, and this test uses host memory with Related: PR #11319 (the PR under test; earlier attempt [REDACTED:Hex High Entropy String] was reverted by b020c4f); no existing issue found for
|
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The gtest job failed on exactly one test — Full analysisSummary: Root cause: The test runs Implicated commit: File: test/gtest/ucp/test_ucp_proto.cc:805 (test body) → test/gtest/ucp/test_ucp_proto.cc:426‑441 and :448‑459 (failing assertions) Suggested fix: Make the AM variant as tolerant as the RMA/tag variants: (1) add |
|
@pentschev please address and fix |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The "Create UCX source tarball" job failed in Full analysisSummary: Publish of the Root cause: Infrastructure/transient network failure, amplified by non-fail-fast submodule handling — not related to the PR's CUDA copy changes:
There is no hang: the largest log gap is the ~60 s built-in upload retry sleep, all other lines are continuous. Implicated commit: File: Suggested fix:
Related: PR #11319 (this build); submodule-introducing PRs #11122, #11094, and #11378 (GPUNetIO update error logging). No existing issue found tracking the broken-symlink artifact upload failure — worth opening one.
|
| if (!ucp_ep_is_lane_p2p(ep, lane)) { | ||
| /* Same-worker PEER selection may include MDs without RMA | ||
| * invalidation, while capable MDs still need it. */ | ||
| if ((key->flags & UCP_EP_CONFIG_KEY_FLAG_SELF) && |
There was a problem hiding this comment.
maybe check only UCT_MD_FLAG_INVALIDATE_RMA here and drop the SELF/err_mode conditions? They duplicate the selection rule from ucp_wireup_try_select_lanes(), so if the relaxation is later extended (e.g. to FAILOVER, as asked in the earlier thread), this assert starts firing again.
There was a problem hiding this comment.
I'm keeping the SELF/PEER guard because lane selection relaxes INVALIDATE_RMA only for that case; FAILOVER was explicitly kept under full requirements. Removing it would silently skip invalidation if an unsupported lane appeared on a cross-worker or FAILOVER endpoint. If selection policy changes later, both paths should change together.
|
🤖 Starting review — findings will be posted here when done. |
| lane = key->rma_bw_lanes[i]; | ||
|
|
||
| if (!ucp_ep_is_lane_p2p(ep, lane)) { | ||
| /* Same-worker PEER selection may include MDs without RMA |
There was a problem hiding this comment.
minor: can we key the skip on the MD capability instead of re-deriving the wireup relaxation here? It keeps the check tied to the precondition of the assert below, and UCP_EP_CONFIG_KEY_FLAG_SELF is also set for the CM intermediate config, which is selected against the local worker address (wireup_cm.c:190), so it is not by itself a reliable "same worker" indicator.
| /* Same-worker PEER selection may include MDs without RMA | |
| if (!(ucp_ep_md_attr(ep, lane)->flags & | |
| UCT_MD_FLAG_INVALIDATE_RMA)) { | |
| /* Same-worker PEER selection may pick MDs without RMA | |
| * invalidation */ | |
| ucs_assert(key->flags & UCP_EP_CONFIG_KEY_FLAG_SELF); | |
| continue; | |
| } | |
There was a problem hiding this comment.
The preliminary CM key can be marked SELF, but it selects lanes with the original peer-failure flags, so with RNDV enabled it still requires INVALIDATE_RMA. The proposed check would not distinguish that key and would also skip unsupported FAILOVER lanes. I’m keeping the exception limited to SELF+PEER, the case whose selection requirements this PR relaxes.
Reverted the UCT changes and pushed a fix on the UCP-layer.
`waitRequests()` progressed the worker before checking completion. In blocking progress mode, requests that completed inline leave nothing to wake `epoll_wait()`, so each wait lasted a full UCX keepalive interval (20 s). Surfaced with UCX master: since openucx/ucx#11319, same-worker endpoints use `self`/`cma` lanes for RMA, so GET and flush complete inline. Tested against UCX master: all `RequestTest.*` pass, 20 s stalls gone (100 -> 0). Failure log: https://dev.azure.com/ucfconsort/ucx/_build/results?buildId=138700&view=logs&j=d476be9b-0898-5876-257d-321aec8cffde&t=709b8934-27f9-5e64-88db-40e26fd98cd3&l=7506 Authors: - Alexey Rivkin (https://github.com/Alexey-Rivkin) - Peter Andreas Entschev (https://github.com/pentschev) Approvers: - Peter Andreas Entschev (https://github.com/pentschev) URL: #777
What?
Allow endpoints connected to the same UCP worker to select transports such as
cuda_copywhen usingUCP_ERR_HANDLING_MODE_PEER, even if those transports do not support peer-failure handling or RMA invalidation.For same-worker PEER endpoints, wireup relaxes the peer-failure requirements during lane selection while retaining the requested PEER mode in the endpoint configuration. When a request buffer must be invalidated, UCP skips only RMA-BW lanes whose memory domain lacks
UCT_MD_FLAG_INVALIDATE_RMA, while capable lanes remain in the invalidation map.Add a same-worker peer-error wireup test. Keep the existing RMA-invalidation test connected to a different worker so it continues to check the cross-worker requirement.
UCP_ERR_HANDLING_MODE_FAILOVERand cross-worker endpoints retain their existing requirements.Why?
With peer error handling and rendezvous enabled, lane selection normally requires peer-failure handling and RMA invalidation.
cuda_copydoes not provide those capabilities, so it was excluded even when both endpoint addresses refer to the same worker.Relaxing selection for that case permits the local copy path. Preserving invalidation for capable lanes also protects against in-flight RMA on network lanes that may be selected by a same-worker endpoint.
Closes #11318