Repository navigation
UCP/FT/TF: Integrate lane hold, wireup reply, and token tests - #12195
evgeny-leksikov wants to merge 4 commits into
Conversation
|
🤖 CI Triage Agent — TL;DR: The "AUTHORS file update check" job failed because Full analysisSummary: Azure job Root cause: The step computes For PR #12195 (source branch The Implicated commit: File: Suggested fix: Make the empty range non-fatal for CI, one of:
set -eEx
base_revision=$(git rev-parse "$(Build.SourceVersion)^1")
head_revision=$(git rev-parse "$(Build.SourceVersion)^2")
if [ -z "$(git log --first-parent --no-merges --format=%h "$base_revision..$head_revision")" ]; then
echo "No first-parent non-merge commits in range - skipping AUTHORS check"
exit 0
fi
./contrib/authors_update.sh "$base_revision..$head_revision"
git diff --exit-code
Also consider validating the range explicitly ( Related: PR #11527 (introduced |
|
🤖 Starting review — findings will be posted here when done. |
| } else { | ||
| shuffle_lanes(lanes, lane_type_str); | ||
| } | ||
|
|
There was a problem hiding this comment.
blocker: get_lanes() is in the shared base class, so this replaces the random shuffle for every PUT-only variant of the already-enabled test_ucp_fault_tolerance on all transports, and order_put_lanes() skips unless there are exactly 6 RMA_BW lanes numbered 0..5. That silently drops the PUT coverage we have today. Pls keep shuffle_lanes() and, if the 4,0,2,1,3,5 order is needed to reproduce a specific failure, add it as a separate dedicated test case.
| UCS_TEST_MESSAGE << lane_type << ": " << size_t(lane) << "/" | ||
| << lanes.size(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The comment "Replay the rc_x/PUT order 4, 0, 2, 1, 3, then 5" plus the per-lane UCS_TEST_MESSAGE loop reads like leftover debug scaffolding. Can we drop this helper?
| (op_info->comp->func == ucp_proto_repost_completion)) { | ||
| req = ucs_container_of(op_info->comp, ucp_request_t, | ||
| send.state.uct_comp); | ||
| ucp_trace_req(req, "repost restart op %s", |
There was a problem hiding this comment.
This trace is unrelated to the LANES_ADDR reply change. Pls move it to a separate PR (REVIEW.md: feature PRs must not carry unrelated changes).
| ucp_proto_repost_completion); | ||
| } | ||
|
|
||
| ucp_trace_req(req, "repost submit op %s lane %u", |
There was a problem hiding this comment.
This trace is unrelated to the LANES_ADDR reply change. Pls move it to a separate PR (REVIEW.md: feature PRs must not carry unrelated changes).
|
|
||
| if (ucp_ep_has_cm_lane(ep)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Suggestion:
ucs_assert(!ucp_ep_has_cm_lane(ep));
The if (ucp_ep_has_cm_lane(ep)) return; right after the assert is dead code — pls keep only one of the two. Same pattern in ucp_ep_tf_accept_peer_req() at L2776-2778.
| if ((ucp_ep_recovery_arg(ep) == NULL) || | ||
| (ucp_ep_recovery_arg(ep)->tf_reply == NULL)) { | ||
| ucs_assert(ucp_ep_recovery_arg(ep) == NULL); | ||
| } |
There was a problem hiding this comment.
Suggestion:
/* A peer request moves the queue pair to error and waits for that
* completion before this lane is failed. Recovery stays idle until
* the completion holds it. */
ucs_assert((ucp_ep_recovery_arg(ep) == NULL) ||
(ucp_ep_recovery_arg(ep)->tf_reply != NULL));
The if (...) { ucs_assert(...) } form is equivalent but much harder to read.
| const void *tx_token, uint8_t tx_len, | ||
| void **rx_token_p, uint8_t *rx_len_p); | ||
| int require_open, void **rx_token_p, | ||
| uint8_t *rx_len_p); |
There was a problem hiding this comment.
require_open is a flag whose only job is to decide whether ucp_ep_tf_derive_rx() applies its ucp_ep_tf_exchange_open() gate, and it is threaded through ucp_wireup_derive_rx_tokens() as well. The root AGENTS.md says not to add a parameter that decides whether the function performs its core operation — can the request path do the ucp_ep_tf_exchange_open() check itself and keep one derive function?
| 2) it does not make sense to recover lanes during close protocol */ | ||
| 2) it does not make sense to recover lanes during close protocol | ||
| The low bit set means there is no pointer: the remaining bits | ||
| are the last accepted 16-bit peer request id. */ |
There was a problem hiding this comment.
Tagging the close_req/recovery_arg union slot with a low bit to stash a 16-bit peer id is fragile: every reader now has to go through ucp_ep_recovery_arg(), while ucp_ep_set_close_request() still writes the slot directly, and the TODO at L581 shows the field layout is not settled. Since ucp_ep_ext_t already gains a uint16_t tf_request_id, why not add a plain uint16_t tf_seen_peer_id next to it and drop the tagging plus most of the accessor churn?
| ucs_debug("ep %p: lane %d keep uct_ep %p until the ACK token", ep, | ||
| lane, tf->uct_ep); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Which message later releases a lane that was purged (tf->purged = 1) but is not in release_lanes? On the reply path release_lanes is derived from provided_lane_map while the purge walks requested_lane_map, so a lane the peer did not provide stays UCP_EP_TF_LANE_HELD with a live uct_ep. It then stays in ucp_ep_tf_held_lanes() forever, so ucp_ep_recovery_progress() can never clear it and the retries get exhausted. Can you point at the releasing path, or should the mask be intersected with the purged lanes instead?
|
PR-level: This adds roughly 800 lines. Pls split it: the Residual coverage gap: No test exercises the new deferred-reply path (a LANES_ADDR request parked on Residual coverage gap: |
Publish the replacement address once the held lane error completion arrives, and ignore a stale peer request.
…hm tls Co-authored-by: svc-nvidia-pr-review <svc-nvidia-pr-review@nvidia.com>
shm,ib selects the same token lanes as rc_x, so it adds no coverage.
33f29d2 to
ecfb222
Compare
|
🤖 Starting review — findings will be posted here when done. |
| } | ||
|
|
||
| ucp_ep_set_lanes_failed(ucp_ep, UCS_BIT(lane), status); | ||
| ucp_wireup_tf_lane_errored(ucp_ep, lane); |
There was a problem hiding this comment.
blocker: ucp_ep_set_lanes_failed() can fail the whole ep and destroy it (ucp_ep_set_failed() -> ucp_ep_disconnected() -> ucp_ep_destroy_internal()), so ucp_ep may already be freed here.
| ucp_wireup_tf_lane_errored(ucp_ep, lane); | |
| ucp_wireup_tf_lane_errored(ucp_ep, lane); | |
| ucp_ep_set_lanes_failed(ucp_ep, UCS_BIT(lane), status); | |
| return UCS_OK; |
| } | ||
| } | ||
|
|
||
| ucs_debug("worker %p: uct_ep %p is ep %p lane %d slot %p", worker, uct_ep, |
There was a problem hiding this comment.
minor: this looks like leftover debugging and mostly repeats the debug message at the top of the handler, pls drop it.
| /* A peer request moves the queue pair to error and waits for that | ||
| * completion before this lane is failed. Recovery stays idle until | ||
| * the completion holds it. */ | ||
| if ((ucp_ep_recovery_arg(ep) == NULL) || |
There was a problem hiding this comment.
pls write this as one assert, the if + assert form is hard to read:
| if ((ucp_ep_recovery_arg(ep) == NULL) || | |
| /* A peer request moves the queue pair to error and waits for that | |
| * completion before this lane is failed, so a parked reply can keep | |
| * the recovery state alive with no failed lane. */ | |
| ucs_assert((ucp_ep_recovery_arg(ep) == NULL) || | |
| (ucp_ep_recovery_arg(ep)->tf_reply != NULL)); | |
| goto done; |
| ucs_queue_head_t tf_pending_q; | ||
| /* LANES_ADDR reply waiting for error completions. Owned by wireup. | ||
| * NULL when no reply is waiting. */ | ||
| void *tf_reply; |
There was a problem hiding this comment.
IMO pls forward-declare ucp_wireup_tf_reply_t and use it here instead of void*, so this pointer keeps type checking.
| static ucp_ep_recovery_arg_t *ucp_wireup_tf_recovery_arg(ucp_ep_h ep) | ||
| { | ||
| if (!ucp_ep_err_mode_eq(ep, UCP_ERR_HANDLING_MODE_FAILOVER) || | ||
| (ucp_ep_get_cm_lane(ep) != UCP_NULL_LANE)) { |
There was a problem hiding this comment.
ucp_ep_recovery_arg() already returns NULL when the ep has a CM lane, so this check is redundant - pls keep only the err_mode check, like ucp_ep_tf_get() does.
| (ucp_ep_get_cm_lane(ep) != UCP_NULL_LANE)) { | |
| if (!ucp_ep_err_mode_eq(ep, UCP_ERR_HANDLING_MODE_FAILOVER)) { |
| uint8_t scheduled; | ||
| ucp_wireup_lane_token_t rx_tokens[UCP_MAX_LANES]; | ||
| void *address; | ||
| size_t address_length; |
There was a problem hiding this comment.
minor: address_length is only written, never read (ucp_address_unpack() does not take a length), pls remove it.
| if (status == UCS_OK) { | ||
| /* Installed before the invalidate, so a completion that arrives | ||
| * from it can finish the reply. */ | ||
| armed = ucp_ep_tf_invalidate_uct_eps(ep, pending); |
There was a problem hiding this comment.
the old code put pending & ~armed into peer_unaware so those lanes went through local failover; now they are only cleared from waiting and are also excluded from peer_unaware by skip_lanes, so a lane whose uct_ep_invalidate() failed is never failed over nor rebuilt. can we fail them here, same as the stuck handling above?
| shuffle_lanes(lanes, lane_type_str); | ||
| if ((op_mask & TEST_OP_PUT) && | ||
| !(op_mask & (TEST_OP_GET | TEST_OP_AM))) { | ||
| order_put_lanes(lanes, lane_type_str); |
There was a problem hiding this comment.
blocker: this also replaces the random shuffle for every PUT-only variant of the base test_ucp_fault_tolerance, and order_put_lanes() skips the test unless there are exactly 6 RMA_BW lanes numbered 0..5, so existing PUT coverage is lost on most transports. if token failover only passes with this specific order, pls fix the ordering dependency instead of hard-coding the reproducer.
|
Not re-raised (already reported): PR size/split, missing deferred-reply test, and Residual coverage gap: |
|
🤖 CI Triage Agent — TL;DR: The only failure in the whole 8873-test run is Full analysisSummary: Root cause: Implicated commit: No code regression. Contributing CI-topology changes: 390a30e "AZP/TESTS: Build once per GPU test row and share the tree with the workers" and ed5255a "AZP/TESTS: Run MPI tests on a worker instead of the prebuild job" (both Alexey Rivkin, Oct 2026), which increase contention on the shared worker/build tree. File: test/gtest/ucs/test_config.cc:781-795 (timed block), test/gtest/ucs/test_config.cc:405 ( Suggested fix:
Related: PR #12195 (the build under test); CI-topology commits 390a30e and ed5255a. No existing issue tracking |
|
all the rest changes are pulled into #12184 |
Summary
Test plan
test_ucp_fault_toleranceandtest_ucp_fault_tolerance_tf