Repository navigation
UCP/CORE: Purge held outstanding work with the stored RX token - #12008
evgeny-leksikov merged 8 commits into
Conversation
|
🤖 Starting review — findings will be posted here when done. |
| } | ||
|
|
||
| uct_ep_destroy(tf->uct_ep); | ||
| tf->uct_ep = NULL; |
There was a problem hiding this comment.
after the purge the lane stays UCP_EP_TF_LANE_HELD with its tokens, so the next REQUEST republishes a tx_token of a destroyed ep (ucp_ep_tf_set_request_id + ucp_ep_tf_tx_len only check HELD), and a later ucp_ep_tf_hold() on the same lane trips ucs_assert(tf->state == UCP_EP_TF_LANE_EMPTY) in ucp_ep_tf_claim() and overwrites both token pointers. pls release the lane here like ucp_ep_tf_cleanup() does: free tokens, reset state/request_id/peer_id/rsc_index and handle deactivate_cfg_index.
| * the trailer would not know the message type either */ | ||
| * the trailer would not know the message type either. The reply may | ||
| * still have stored an RX token for our held lanes. */ | ||
| ucp_ep_tf_lanes_purge_outstanding(ep, |
There was a problem hiding this comment.
purge call in the !ucp_wireup_ep_supports_tokens(ep) branch: request_id is always 0 here, because ucp_wireup_parse_lanes_addr() returns before setting it when the peer adds no trailer, and ucp_ep_tf_lanes_purge_outstanding() drops request_id == 0 - so this call can never purge anything. pls drop it (and the comment above it).
| tf->uct_ep = NULL; | ||
| } | ||
|
|
||
| void ucp_ep_tf_lanes_purge_outstanding(ucp_ep_h ep, ucp_lane_map_t lanes, |
There was a problem hiding this comment.
| void ucp_ep_tf_lanes_purge_outstanding(ucp_ep_h ep, ucp_lane_map_t lanes, | |
| void ucp_ep_tf_lanes_purge_outstanding(ucp_ep_h ep, ucp_lane_map_t lanes, | |
| uint32_t request_id, int from_ack) |
minor: function arguments are indented on column (docs/CodeStyle.md); same for the prototype in ucp_ep.h.
| static void | ||
| ucp_ep_tf_purge_replay_cb(const uct_ep_op_info_t *op_info, void *arg) | ||
| { | ||
| ucs_error("ep %p: replaying purged operation %p is not implemented", arg, |
There was a problem hiding this comment.
minor: pls use ucs_diag here (ucs_error in ucp_ep_tf_purge_replay_cb) - this fires once per undelivered operation on every token failover, and UCT already completes those ops with UCS_ERR_CANCELED (uct_rc_mlx5_ep_outstanding_complete_send_ops), so it is not an error the program cannot continue from.
| static ucs_status_t | ||
| ucp_wireup_read_token_section(ucp_lane_map_t lane_map, const void *section, | ||
| size_t avail, size_t *consumed_p, | ||
| ucp_wireup_lane_token_t *slots) |
There was a problem hiding this comment.
ucp_wireup_read_token_section: can we extend test_ucp_wireup_token_section to cover the new slot-filling path (lane indexing and a token cut in the middle)? DISABLED_test_ucp_fault_tolerance_tf is still disabled, so today nothing exercises the parsed tokens.
|
Process caveat: this environment has no Things checked and not flagged: Trailer layout/versioning is still gated by Residual coverage gap: the end-to-end flow (hold → derive → publish → purge) only runs with RC mlx5 DEVX hardware and the |
| /* Undelivered operations are reported here. Keeping them for replay is left | ||
| * to a later change. */ | ||
| static void | ||
| ucp_ep_tf_purge_replay_cb(const uct_ep_op_info_t *op_info, void *arg) |
There was a problem hiding this comment.
For an undelivered PUT_ZCOPY, UCT places the user completion in op_info, then releases the send operation without invoking that completion on the canceled path. The request can remain outstanding indefinitely.
| continue; | ||
| } | ||
|
|
||
| params.field_mask = UCT_EP_OUTSTANDING_FIELD_RX_TOKEN | |
There was a problem hiding this comment.
After purging and destroying its UCT EP, the lane retains its tokens and state. A later request can publish the old TX token, and holding that lane again hits the EMPTY assertion.
946c784 to
1e98dc2
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The ASAN job did not run out of time — Full analysisSummary: gtest aborted via watchdog ("Connection timed out - abort testing") during Root cause: The log shows a hang, not slowness. Timestamps:
That is a 900-second gap with zero output — by far the largest gap in the run (every other test completes in 1–1800 ms). The backtrace names the hung operation precisely: The frames at |
A lanes-address reply or ACK carries the peer receive position of a held lane. Purge that lane with the stored token and destroy the endpoint. Replaying the undelivered operations is left for a later change.
A successful purge destroyed the transport endpoint and left the lane held, so the next request republished that endpoint's TX token. Drop the reply purge that cannot match a request id, complete an undelivered zcopy, and log the missing replay at diag.
A token means that lane already failed remotely. Leave the transport endpoint on the lane until the error handler holds it. Lanes that cannot carry a token still take the software discard path.
6708ee2 to
98a57b3
Compare
|
🤖 Starting review — findings will be posted here when done. |
| ucs_assert(tf != NULL); | ||
| tf->peer_id = peer_id; | ||
| tf->state = UCP_EP_TF_LANE_INVALIDATED; | ||
| tf->rsc_index = ucp_ep_get_rsc_index(ep, lane); |
There was a problem hiding this comment.
tf->rsc_index is recorded here, but ucp_ep_tf_lane_wiface() only prefers it when the state is HELD, so ucp_ep_tf_derive_rx()/ucp_ep_tf_save_rx() on an invalidated lane still resolve the iface from the current lane config; after a reconfig that can be a different device and the token is dropped. can we accept UCP_EP_TF_LANE_INVALIDATED there as well?
| &remote_address); | ||
| } else if (msg->type == UCP_WIREUP_MSG_LANES_ADDR_REPLY) { | ||
| ucs_assert(lanes_info != NULL); | ||
| ucp_wireup_invalidate_unaware_failed_lanes(ep, lanes_info, |
There was a problem hiding this comment.
this invalidates lanes we still consider alive, and it runs before ucp_wireup_process_lanes_addr_reply() sends the ACK, so if the current am_lane is in that set the ACK goes out on a QP we just invalidated and the peer never purges. the REQUEST path deliberately does local failover first so "the reply rides the new am_lane" — can we send the ACK before invalidating?
| /* A lane can enter the token path only when a later hold would accept it. | ||
| * Anything else stays on the software discard path. */ | ||
| static int | ||
| ucp_ep_tf_lane_supported(ucp_ep_h ep, ucp_lane_index_t lane, uct_ep_h uct_ep) |
There was a problem hiding this comment.
this repeats the gating of ucp_ep_tf_claim() almost line by line (mode, FAILED, err_mode, cm_lane, dst_version, token caps, tx_len range), so the two can silently drift. can we extract one helper and call it from both?
| * puts a zcopy completion in op_info and does not invoke it when the | ||
| * operation is canceled, so complete it here. */ | ||
| static void | ||
| ucp_ep_tf_purge_replay_cb(const uct_ep_op_info_t *op_info, void *arg) |
There was a problem hiding this comment.
this is nearly identical to ucp_ep_tf_outstanding_purge_cb(); can we reuse that one with params.arg = UCS_STATUS_PTR(UCS_ERR_CANCELED) and drop the ucs_diag, which prints one line per purged operation?
| return; | ||
| } | ||
|
|
||
| lanes = ucp_wireup_lanes_with_tokens(lanes_info->requested_lane_map, |
There was a problem hiding this comment.
why are the reply's RX-token lanes part of the invalidate set? they name our own held lanes, and a held lane is always in ucp_ep_get_failed_lanes() (hold → detach → ucp_ep_reconfig_internal), so this term is masked out two lines below.
|
Residual coverage gap: no test accompanies the change. The invalidate → derive → store → purge path needs RC mlx5 DEVX hardware and the |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The ASAN BlueField build died because gcc's Full analysisSummary: Root cause: Evidence from the log (
This signature on an aarch64/BlueField agent with gcc-11 plus Implicated commit: unknown — not attributable to commit [REDACTED:Hex High Entropy String]; the failing file is untouched by this PR. Environment-related; File: Suggested fix:
Related: PR #12008 (the PR under test); #11915 (only loose textual match, not the same failure) — no existing issue tracking this ICE.
|
Failover reconfig keeps each lane on its original transport resource, so token derive and save use the live endpoint config.
Hold copied the checks that decide whether a lane can carry a token, so the two paths could drift.
The token path had its own callback that completed a zcopy and logged every operation. The shared callback already completes with the status it is given.
|
🤖 Starting review — findings will be posted here when done. |
| invalidated = 0; | ||
| if (request_id != 0) { | ||
| ucp_ep_tf_hold_lanes(ep, lanes_info->provided_lane_map, request_id); | ||
| invalidated = ucp_ep_tf_invalidate_lanes(ep, |
There was a problem hiding this comment.
A lane is invalidated before the reply is sent. Same in reply path. If that's am_lane, its QP is put into error but ucp_wireup_msg_progress() still selects it?
There was a problem hiding this comment.
in this case it will be handled as a new lane failure or operation for retransmit on purge, both are not fatal and recoverable
|
A 0-length peer token (e.g SW failover) still takes the token path |
|
What?
Purge held outstanding work with the stored RX token
Why?
Preparation to replaying the undelivered operations. Moved from #12005 to be under 500 lines limit.
How?
A lanes-address reply or ACK carries the peer receive position of a held
lane. Purge that lane with the stored token and destroy the endpoint.