Repository navigation
Conversation
|
🤖 Starting review — findings will be posted here when done. |
| return pi; | ||
| } | ||
|
|
||
| if (uct_ib_mlx5_wqe_size(ctrl) > rdma_wqe_size) { |
There was a problem hiding this comment.
An unsignaled zero-length put_short WQE is also just ctrl+raddr with no inline segment (see the length == 0 branch in uct_rc_mlx5_txqp_inline_post), so it reaches the scan below and can return the sn of an unrelated SGL op posted after it, completing that op before its WQEs are reported. Can we require wqe_size == rdma_wqe_size + sizeof(struct mlx5_wqe_data_seg) instead?
There was a problem hiding this comment.
Good catch, thanks. Reproduced by put_after_failed_put_short (7f99f92): without the fix, the SGL put posted after a failed zero-length put_short completed with UCS_ERR_ENDPOINT_TIMEOUT from the put_short failure. Fixed in 57e385e: an SGL chain WQE must now be exactly ctrl + raddr + one data segment, and not inline. A WQE of a zero-length SGL element can't be told from a zero-length put_short, so it is not detected; this is noted in the function comment.
| test_put_sgl_zcopy_peer_failure(SGL_COUNT - 1); | ||
| } | ||
|
|
||
| UCT_INSTANTIATE_TEST_CASE(test_uct_peer_failure_sgl_zcopy) |
There was a problem hiding this comment.
cuda_ipc also reports UCT_IFACE_FLAG_V2_PUT_SGL_ZCOPY (CUDA >= 13) and ERRHANDLE_PEER_FAILURE, but the test allocates host mapped_buffer, which the cuda_ipc MD cannot register (reg_mem_types is CUDA only), so the test would not skip and would fail there.
| UCT_INSTANTIATE_TEST_CASE(test_uct_peer_failure_sgl_zcopy) | |
| UCT_INSTANTIATE_RC_TEST_CASE(test_uct_peer_failure_sgl_zcopy) |
Alternatively select the memory type from md_attr().reg_mem_types like test_uct_sgl does.
There was a problem hiding this comment.
Done in f8e89c1, the test is instantiated with UCT_INSTANTIATE_RC_TEST_CASE.
|
|
||
| ASSERT_EQ(1u, comp.done); | ||
| ASSERT_EQ(1ul, m_failed_eps.count(ep0())); | ||
| EXPECT_EQ(m_failed_eps[ep0()], comp.uct.status) |
There was a problem hiding this comment.
Do put_fail_first/put_fail_middle fail without the iface change? The last WQE of the chain is signaled, and its flush-error CQE should purge the op with the same endpoint status, so it would help to state the symptom being fixed in the PR description.
There was a problem hiding this comment.
Yes: without the rc_mlx5_iface.c change they complete with UCS_ERR_CANCELED (-16) instead of UCS_ERR_CONNECTION_RESET (-25). The flush-error CQE of the last WQE does not carry the endpoint status: uct_ib_mlx5_completion_with_err() maps WR_FLUSH_ERR to UCS_ERR_CANCELED, and uct_rc_mlx5_iface_handle_failure() purges with the status of that CQE.
|
Residual coverage gap: only Residual coverage gap: the UCP |
|
🤖 CI Triage Agent — TL;DR: The newly added gtest Full analysisSummary: Root cause:
So this is a new-test/transport-coverage bug exposed by a missing input-validation path in cuda_ipc, not a flake or a timeout. Implicated commit: File: Suggested fix:
Related: PR #12205 ("UCP/UCT/IB: Return endpoint error status for SGL put on peer failure") — #12205; cuda_ipc SGL implementation PRs #11359 and #11762.
|
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The build was not a code failure — the Azure DevOps agent on Full analysisSummary: Job "Tests roce on worker 0" (build 141904) was aborted during the Root cause: Infrastructure, not code. The log shows a clean build with zero compiler/linker errors right up to the end (libucs/libuct/libucp, tools, examples and gtest objects all built fine). At This is not a hang or a wall-clock timeout: timestamps are continuous with gaps of only 1–6 seconds (e.g. 13:22:54 → 13:22:55 → 13:22:59 → 13:23:00) all the way to the cancellation, and the step had only been running ~3 minutes of compilation. The work was progressing normally when the agent process went away. Implicated commit: unknown — no commit is implicated; the aborted step produced no error attributable to File: n/a — failure originates in the Azure Pipelines agent on host Suggested fix:
Related: #12194 (CI: pinning jobs to specific workers) — tangentially related CI-infrastructure work; no existing issue tracks this agent shutdown.
|
|
maybe we can identify the SGL chain from the send op instead of parsing the WQE, e.g. keep the chain's first sn (or WQE count) in the op? these sizes duplicate the layout built by
can we copy the stub's current Residual coverage note: the |
3637358 to
57e385e
Compare
|
🤖 Starting review — findings will be posted here when done. |
| } | ||
|
|
||
| /* SGL put protocols use only RMA_BW lanes which support put zcopy */ | ||
| bool test_ucp_peer_failure::has_put_zcopy_rma_bw_lane() |
There was a problem hiding this comment.
minor: can we use sender().has_lane_with_caps(UCT_IFACE_FLAG_PUT_ZCOPY) here instead of a new helper? test_ucp_rma_sgl uses it for the same skip, and it also accepts v2 caps if we want to key the skip on UCT_IFACE_FLAG_V2_PUT_SGL_ZCOPY.
Impact: ~20 lines of duplicated lane/iface-attr walking in a fixture, against test/AGENTS.md ("Reuse helpers from test/gtest/common and existing fixtures before adding a new fixture shape").
There was a problem hiding this comment.
This is what the test used first, but it is too weak: on tcp a non-RMA lane has PUT_ZCOPY while no put/sgl protocol is selected ("cannot find remote protocol"), so the test failed instead of skipping. Keying on UCT_IFACE_FLAG_V2_PUT_SGL_ZCOPY would skip rc, dcx and dc_mlx5, where put/sgl/offload_sw is used and the test passes. The helper checks the RMA_BW lanes, which is what both SGL put protocols use.
|
|
Considered it, but
The table follows |
|
@michal-shalev @evgeny-leksikov could you review these chages, please? |
evgeny-leksikov
left a comment
There was a problem hiding this comment.
please split the pull request to independent parts and clarify exactly what is each part needed for. For example purge outstanding is not needed for per_failure err handling. It's needed for token based failover and recovery.
57e385e to
85f2685
Compare
|
🤖 Starting review — findings will be posted here when done. |
| uint16_t op_flags, uct_completion_t *comp) | ||
| { | ||
| uct_ib_mlx5_txwq_t *txwq = &ep->tx.wq; | ||
| const uint16_t first_pi = txwq->sw_pi; |
There was a problem hiding this comment.
minor: first_pi and pi are both initialized from the same txwq->sw_pi (pi = txwq->sw_pi; a few lines below), i.e. redundant state that can silently drift. Can we set pi = first_pi there so the two cannot drift apart?
|
Residual coverage note: the new tests only exercise the non-failover purge path (
The suite is instantiated via |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| /* Without a failure the operation completes with UCS_OK by the CQE of the | ||
| * last WQE of the chain, so the data of all its elements is in place */ | ||
| void test_sgl_zcopy_completion(bool is_put_op) |
There was a problem hiding this comment.
maybe move these success-path checks to test_uct_sgl, which already has with_callback? They don't inject any failure, and this fixture lowers RC_TIMEOUT to 100us / RC_RETRY_COUNT to 4, which adds flakiness risk to a test that asserts m_failed_eps.count(ep0()) == 0.
Rationale: put_completion flushes before pattern_check(), so it cannot detect a premature completion and is effectively a duplicate of test_uct_sgl.with_callback; only get_completion (data checked without flush) adds a new assertion.
|
🤖 Starting review — findings will be posted here when done. |
| test_put_zcopy_after_failed_sgl_zcopy(); | ||
| } | ||
|
|
||
| UCS_TEST_SKIP_COND_P(test_uct_peer_failure_sgl_zcopy, put_completion, |
There was a problem hiding this comment.
minor: maybe drop put_completion/get_completion? test_uct_sgl.with_callback in test/gtest/uct/v2/test_uct_sgl.cc already covers the multi-element success path for put and get (completion fired with UCS_OK + pattern_check after flush) on all transports, so this duplicates it in a peer-failure fixture.
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
| !check_caps(UCT_IFACE_FLAG_ERRHANDLE_PEER_FAILURE, 0, | ||
| UCT_IFACE_FLAG_V2_PUT_SGL_ZCOPY)) | ||
| { | ||
| test_sgl_zcopy_completion(true); |
There was a problem hiding this comment.
Do we need add case for success path in test_uct_peer_failure_sgl_zcopy ?
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| bool is_caps_supported(uint64_t required_flags); | ||
| bool check_caps(uint64_t required_flags, uint64_t invalid_flags = 0); | ||
| bool check_caps(uint64_t required_flags, uint64_t invalid_flags = 0, |
There was a problem hiding this comment.
minor: can we keep the existing idiom !check_caps(A) || !check_caps_v2(B) (as in test_rc.cc) instead of adding a third positional parameter? the 0 for invalid_flags repeated in all 10 new skip conditions is less readable than two calls.
|
🤖 CI Triage Agent — TL;DR: The gtest suite on worker 3 reported exactly one failure — Full analysisSummary: Root cause: From the log, the run completed normally (8902 passed, 3361 skipped, 3,122,763 ms total, continuous timestamped activity right up to the tear-down at Crucially, the change under test (PR #12205, "UCT/IB/MLX5/RC: Complete SGL op with ep error status on peer failure", branch Implicated commit: unknown — not attributable to [REDACTED:Hex High Entropy String] from the log evidence (PR touches mlx5/RC SGL only; failure is in the tcp non-SGL put path) File: test/gtest/ucp/test_ucp_memheap.cc:110-116 (validation point reached from test/gtest/ucp/test_ucp_rma.cc:283 Suggested fix:
Related: PR #12205 (the PR under test), #11923 (UCP/FLUSH selective endpoint flush), #12091 (repost GET/zcopy on selected lane)
|
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
ba9cb21 to
2cea8f4
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
An SGL operation posts one WQE per element, and only the last WQE is signaled. Its send op was kept on the last WQE, so the peer failure purge, which completes the operations up to the failed WQE with the endpoint error status, missed it when an element other than the last one failed, and the operation was completed later by the QP flush with UCS_ERR_CANCELED. Keep the send op on the first WQE of the chain, so a failure of any of its WQEs completes the operation with the endpoint error status, like an operation of a single WQE. Without a failure it is still completed by the CQE of the last WQE. Add test_uct_peer_failure_sgl_zcopy on rc_mlx5 and cuda_ipc: a failure of the first, a middle or the last element of an SGL put/get, SGL puts with a zero-length element, an SGL put posted after a failed put_short, a put_zcopy posted after a failed SGL put, and completion without a failure. uct_test::check_caps() gets optional required v2 caps for the SGL caps check.
2cea8f4 to
50be3cf
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
Residual note (not blocking): Residual note (not blocking): the failover purge path ( Residual note (not blocking): Minor judgement call (not blocking): |
|
🤖 CI Triage Agent — TL;DR: The job never built anything — it died in the "Get sources" step because Full analysisSummary: Azure Pipelines job Root cause: Authentication failure at source checkout. The agent ( Every attempt (3 inner retries × 5 outer There is zero build or compile output in the log — no configure, no Implicated commit: unknown — infrastructure failure, unrelated to the PR commit File: n/a (failure in Azure Pipelines "Get sources" task, before any repo file was fetched; no Suggested fix:
Related: none found — a search for this error signature in the repo's issues/PRs returned no matching reports (only unrelated PRs).
|
|
@evgeny-leksikov @jeynmann @michal-shalev could you approve the PR again? the commits were rebased and squashed |
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
🤖 CI Triage Agent — TL;DR: The gtest binary aborted on Full analysisSummary: Root cause: Backtrace from the log: Implicated commit: unknown (not [REDACTED:Hex High Entropy String] / PR #12205 — it only touches UCT/IB/MLX5/RC). The unsafe lock-free put path originates in File: src/ucs/memory/rcache.inl:121-132 ( Suggested fix:
Related: PR under test: #12205 (unrelated to the failure). No existing issue found for this assertion; worth opening one for the
|
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
What?
With peer error handling, an rc_mlx5
put_sgl_zcopy/get_sgl_zcopyoperation completes withUCS_ERR_CANCELEDinstead of the endpoint error status when the peer fails and the error is reported for any element of the SGL except the last one. This PR keeps the send op of an SGL operation on the first WQE of its chain, so the failure purge completes it like an operation of a single WQE.Why?
An SGL operation posts one WQE per element and kept its send op and CQ update on the last WQE of the chain. The error CQE is reported for the first failed WQE of the chain, and
uct_rc_mlx5_iface_handle_failure()completes the outstanding send ops withsnup to that WQE with the endpoint error status, so the SGL send op was skipped. It was completed later by the flush CQE of its last WQE (WR_FLUSH_ERR) withUCS_ERR_CANCELED, so a user of the HW SGL protocol (UCPput/sgl/offload) could not tell a peer failure from a cancellation.How?
UCT/IB/MLX5/RC:
uct_rc_mlx5_base_ep_sgl_zcopy_post()adds the send op with thesnof the first WQE of the chain instead of the last one. A failure reported for any WQE of the chain is at or after thatsn, so the existing purge completes the operation with the endpoint error status. The operation still completes only by the CQE of the last WQE, since the other WQEs of the chain are unsignaled. Operations posted after the failed WQE are still completed by the QP flush, as before. The token based failover purge (uct_rc_mlx5_ep_outstanding_purge()) is not changed; it does not support SGL operations either before or after this change.GTEST/UCT: add
test_uct_peer_failure_sgl_zcopyon rc_mlx5 and cuda_ipc, the transports with SGL zcopy. The tests are skipped by the SGL caps, e.g. on cuda_ipc built with CUDA older than 13; for thatuct_test::check_caps()gets optional required v2 caps. On rc_mlx5 the tests:put_sgl_zcopy/get_sgl_zcopyand check that the completion status equals the status reported to the error handler;max_inl_cqe;put_short, and aput_zcopyposted after a failed SGL put, are completed by the QP flush;UCS_OKand moves the data of all the elements but a zero-length one.On cuda_ipc only the completion tests run, with CUDA memory, since cuda_ipc does not report errors of posted operations. The completion test with a zero-length element is skipped there until UCT/CUDA_IPC: Skip zero-length elements in SGL zcopy #12219, which fixes cuda_ipc failing such an SGL.
The UCP part (SGL operations on failed lanes return the endpoint error status) will come in a separate PR.
Tests (devel builds):
put_fail_first/put_fail_middle/get_fail_first/get_fail_middlefail on rc_mlx5 (UCS_ERR_CANCELEDinstead ofUCS_ERR_CONNECTION_RESET);*_fail_lastandput_after_failed_put_shortpass.test_uct_peer_failure_sgl_zcopy39/39 pass on rc_mlx5 (3 devices); on cuda_ipcput_completion/get_completionpass and the failure tests are skipped.test_uct_peer_failure*on rc_mlx5, rc_verbs and dc_mlx5, and rc_mlx5test_uct_purge_outstanding,test_uct_sglandtest_rc_get_limit: 405/405 pass.test_ucp_rma_sgl: 80/80 pass.