Skip to content

UCT/IB/MLX5: rc support in progress error handler. - #11823

Merged
gleon99 merged 7 commits into
openucx:masterfrom
jeynmann:failover_uct_rcx_no_comp
Aug 26, 2026
Merged

gleon99 merged 7 commits into
openucx:masterfrom
jeynmann:failover_uct_rcx_no_comp

Conversation

@jeynmann

@jeynmann jeynmann commented Aug 25, 2026 •

Copy link
Copy Markdown
Contributor

What?

Allow an RC mlx5 error handler to return UCS_INPROGRESS so the transport does not immediately purge outstanding TX WQEs.

Why?

On endpoint failure, RC mlx5 currently always purges outstanding ops and returns both CQ and QP resources in one step. Failover needs those WQEs to stay posted so the UCP can inspect or recover them.

How?

Allows error handler return UCS_INPROGRESS to indicate that completions should not be purged imediately.

Split ep invalidate from PR#11684

@jeynmann jeynmann changed the title UCT: RC_X support in progress error handler. UCT/IB/MLX5: rc support in progress error handler. Aug 25, 2026
@svc-ucx

svc-ucx commented Aug 25, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests roce on worker 1) · commit 8d6ef117

TL;DR: rc_mlx5/test_uct_peer_failure_multiple.test/0 first hit a 5-minute uct_test::flush() timeout and then aborted on the new ucs_assert(pi == txwq->hw_ci) in uct_ib_mlx5_txwq_num_posted_wqes() during EP teardown; the new "err handler returns INPROGRESS" flow in this PR advances txwq->hw_ci on the error CQE while deliberately not releasing txqp.available, so the cleanup path's outstanding = bb_max - available no longer matches hw_ci.

Full analysis

Summary: gtest rc_mlx5/test_uct_peer_failure_multiple.test/0 failed with "Timed out" on iface flush (uct_test.cc:782) and then Assertion 'pi == txwq->hw_ci' failed at ib_mlx5.c:858, crashing the test binary (make: *** [test] Aborted (core dumped)).

Root cause: In uct_rc_mlx5_iface_handle_failure() TX resources are reconciled only on the status == UCS_OK branch (uct_rc_txqp_purge_outstanding() + uct_rc_mlx5_ep_update_tx_qp_res(ep, pi)). On the two other paths added/kept by this PR — status == UCS_INPROGRESS (only ft_ci is recorded, WQEs intentionally preserved for failover) and the early goto out_update_tx_cq_res when UCT_RC_EP_FLAG_ERR_HANDLER_INVOKED/FLUSH_CANCEL is already set — control still falls into uct_rc_mlx5_iface_update_tx_cq_res(), which sets txwq->hw_ci = pi of the error CQE. txqp.available is left untouched, so the invariant prev_sw_pi - (bb_max - available) == hw_ci is broken. Consequences observed in the log, in order:

  1. Those outstanding TX resources are never released, so uct_iface_flush() keeps returning UCS_INPROGRESS → the test's flush loop spins for ~5 min until the deadline (EXPECT_TRUE(flushed) << "Timed out").
  2. At TearDown, uct_rc_mlx5_ep_t cleanup recomputes outstanding = bb_max - txqp.available (rc_mlx5_ep.c:1320) and calls uct_ib_mlx5_txwq_num_posted_wqes(), whose brand-new assert pi == txwq->hw_ci (added by this PR) fires and aborts.

The assert and the INPROGRESS branch are both new in this PR — ib_mlx5.c had no other changes since 2025.

Implicated commit: 106bf44 "UCT/IB/MLX5: support err handle return inprogess" (Zihao Zhao), refined by 5d72c3c and head commit 8d6ef11 "UCT/IB/MLX5: fix tx res order" (same author)

File: src/uct/ib/mlx5/rc/rc_mlx5_iface.c:228-237 (with src/uct/ib/mlx5/ib_mlx5.c:858 and src/uct/ib/mlx5/rc/rc_mlx5_ep.c:1320-1322)

Suggested fix: Keep hw_ci and txqp.available consistent, and stop deriving the WQE scan start from txqp.available:

  • Do not advance txwq->hw_ci (i.e. skip/limit uct_rc_mlx5_iface_update_tx_cq_res()'s hw_ci update) while a WQE range is being preserved for failover (UCS_INPROGRESS / UCT_IB_MLX5_TXWQ_FLAG_FAILED), or record the preserved range in ft_ci and release the corresponding TX credits so flush can complete.
  • Change uct_ib_mlx5_txwq_num_posted_wqes() to take the start PI explicitly (txwq->hw_ci, or ft_ci when the failed/preserved flag is set) instead of prev_sw_pi - outstanding plus an assert; correspondingly compute outstanding = txwq.prev_sw_pi - txwq.hw_ci in UCS_CLASS_CLEANUP_FUNC(uct_rc_mlx5_ep_t).
  • Also make the ERR_HANDLER_INVOKED | FLUSH_CANCEL early-exit path release/purge TX resources (or explain why it cannot), otherwise repeated failures on the same EP permanently stall uct_iface_flush() as seen here.

Related: PR #11823 (this PR); prior work in this series: #11668 (UCT outstanding purge implementation), #11580. Historical similar symptom: #5999 (dc_mlx5/test_uct_peer_failure_multiple.test/0 flush failure).

@gleon99
gleon99 requested a review from roiedanino August 25, 2026 12:10
@gleon99

gleon99 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

@roiedanino @evgeny-leksikov please review

roiedanino
roiedanino previously approved these changes Aug 25, 2026
Comment thread src/uct/base/uct_iface.h Resolved
@svc-ucx

svc-ucx commented Aug 26, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests new on worker 3) · commit 5a22d4c2

TL;DR: The gtest run failed on a single unrelated timing-sensitive test, test_config.performance, which asserts that one car_opts config parse completes within a hard-coded 50 ms wall-clock budget (UCS_TEST_TIME_LIMIT(0.05)) — it flaked on a loaded, parallel-executor Azure worker; re-run the job, and if it recurs, raise/relax that budget rather than treating it as a PR regression.

Full analysis

Summary: make test in the "Tests new on worker 3" job exited with code 1 because 1 of 8539 gtests failed: test_config.performance (all other tests, including the PR's new test_ucp_fault_tolerance cases, passed).

Root cause: Evidence from the log: [ FAILED ] 1 test, listed below: [ FAILED ] test_config.performance followed by make: *** [Makefile:4713: test] Error 1. That test (test/gtest/ucs/test_config.cc:781-795) pushes 300 dummy env vars and then wraps a single car_opts opts(UCS_DEFAULT_ENV_PREFIX, NULL) construction in UCS_TEST_TIME_LIMIT(0.05), i.e. the whole ucs_config_parse_config_files() + ucs_config_parser_fill_opts() path must finish in 50 ms of wall time. UCS_TEST_TIME_LIMIT (test/gtest/common/test_helpers.h:158-168) reports a non-fatal failure purely on elapsed wall clock, with no CPU-time or load compensation. This job ran 8539 tests over ~62 min (3718476 ms total) on a shared worker with parallel executors (nworkers/worker in contrib/test_jenkins.sh), so a 50 ms budget is easily blown by scheduler noise. Nothing in this PR (branch failover_uct_rcx_no_comp, UCT RC/DC failover) touches src/ucs/config/parser.c or the config tables — the last changes there are 597005c9 and 49a0d4c1, both long predating this commit — so this is not a PR regression. Note that 597005c9 ("UCS/CONFIG: Support numeric ranges for devices") did add an extra ALLOW_LIST_WITH_RANGES field to car_opts_table (test_config.cc:240-243) without adjusting the 50 ms budget, which plausibly narrowed the margin and made this test more prone to flaking.

Implicated commit: Not the PR commit [REDACTED:Hex High Entropy String]. Closest contributing change: 597005c9 (Guy Ealey Morag, "UCS/CONFIG: Support numeric ranges for devices (#11340)") which enlarged car_opts_table while leaving the perf budget unchanged.

File: test/gtest/ucs/test_config.cc:792 (UCS_TEST_TIME_LIMIT(0.05)); mechanism at test/gtest/common/test_helpers.h:158

Suggested fix: Re-run the failed Azure job first — this is an environment-sensitive flake, not a code defect in PR #11823. To stop it recurring, make the perf budget robust: raise UCS_TEST_TIME_LIMIT(0.05) to something like 0.2 in test_config.cc:792 (justified by the extra fields added in #11340), or better, measure the parse with CPU time / take the best of several iterations instead of a single wall-clock sample, and scale the limit via ucs::test_time_multiplier() when running under parallel executors (nworkers > 1).

Related: PR #11340 (added ALLOW_LIST_WITH_RANGES to the test config table); no existing issue tracking test_config.performance flakiness was found — worth opening one.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id ebab315e-fadd-4a0c-a8b0-379b4ab352b3 in the triage console for the audit trail.

@gleon99
gleon99 enabled auto-merge (squash) August 26, 2026 08:46
@gleon99
gleon99 merged commit 9b07214 into openucx:master Aug 26, 2026
162 checks passed
@jeynmann
jeynmann deleted the failover_uct_rcx_no_comp branch August 26, 2026 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants