UCT/IB/MLX5: support in progress error handler - #11829
Conversation
|
🤖 CI Triage Agent — TL;DR: Two RC/mlx5 error-path gtests hung (3 min and 15 min of zero output, ending in the gtest watchdog Full analysisSummary: Root cause: Evidence of a hang, not slowness: the log jumps 07:27:15 → 07:42:15 with no output while in Implicated commit: 2123627 "UCT/IB/MLX5: add support for no completions" (Zihao Zhao), building on 106bf44 "UCT/IB/MLX5: support err handle return inprogess" File: src/uct/ib/mlx5/rc/rc_mlx5_iface.c:210-239 (with src/uct/ib/mlx5/rc/rc_mlx5_ep.c:726-775) Suggested fix: In the Related: PR #11829 ("[DNM] UCT/IB/MLX5: support in progress error handler"), prior work PR #11668 ("UCT: outstanding purge implementation") |
jeynmann
changed the title
2123627 to
0a8d5c3
Compare
jeynmann
changed the title
|
🤖 Starting review — findings will be posted here when done. |
| */ | ||
| ucs_assertv(bb_num > 0, "hw_ci=%d prev_sw_pi=%d available=%d bb_num=%d", | ||
| hw_ci, txwq->prev_sw_pi, txqp->available, bb_num); | ||
| uct_ib_mlx5_txwq_t *txwq = &ep->tx.wq; |
There was a problem hiding this comment.
Switch from the update_bb formula to incremental hw_ci - prev_hw_ci assumes strict monotonicity.
uint16_t bb_num = hw_ci - prev_hw_ci;
ucs_assertv(bb_num > 0, ...);
txwq->hw_ci = hw_ci;The previous update_tx_res derived bb_num from prev_sw_pi/available, which was robust to repeated completions reporting the same counter. The new code requires every reported hw_ci/pi to strictly advance past the stored hw_ci. In handle_failure the flushed error CQEs feed pi into this function via out_update_tx_res, and this now also runs on the repeated-failure path (ERR_HANDLER_INVOKED/FLUSH_CANCEL -> goto out_update_tx_res). If a subsequent error CQE reports the same wqe_counter, bb_num == 0 trips the assert. Worth confirming that flushed error CQEs always carry a strictly increasing counter, otherwise this is a regression from the old formula.
There was a problem hiding this comment.
It's strict monotonicity. HW should not generates 2 cqes for 1 wqe.
|
Notes / no action: Making Test coverage: This is a behavioral change to the RC error/purge path. I did not find an added regression test in the diff. Since the new |
|
🤖 Starting review — findings will be posted here when done. |
|
Test coverage: no regression test in the diff for the deferred |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: At 11:49:40 the log shows:
The drop tool therefore died right after being backgrounded (confirmed later by The script has no check on the drop tool's success: line 549 launches Implicated commit: [REDACTED:Hex High Entropy String] — "CI/FT: Adds fault tolerance CI test (#11570)", william-gallagher-nv (added File: contrib/test_jenkins.sh:549 (drop tool launch, unchecked) — failure surfaces at contrib/test_jenkins.sh:557 via Suggested fix:
${drop_tool} -d ${dev} -s ${sl} -t ${tc} > drop_tool.${dev}.log 2>&1 &
drop_pid=$!
sleep 2
if ! kill -0 ${drop_pid} 2>/dev/null; then
cat drop_tool.${dev}.log
echo "Warning: drop tool failed on ${dev} (flow table not supported/permitted) - skipping FT test"
kill_background_processes "${background_pids[@]}"
return 0 # or 'continue' if other devices should still be tried
fi
background_pids+=(${drop_pid})
Related: PR #11570 (added the fault tolerance CI test); PR #11829 (this build) — no existing issue found for syndrome
|
| txwq->curr = txwq->qstart; | ||
| txwq->sw_pi = 0; | ||
| txwq->prev_sw_pi = UINT16_MAX; | ||
| txwq->ft_ci = UINT16_MAX; | ||
| txwq->hw_ci = UINT16_MAX; | ||
| #if UCS_ENABLE_ASSERT | ||
| txwq->hw_ci = 0xFFFF; | ||
| txwq->flags = 0; | ||
| txwq->flags = 0; |
There was a problem hiding this comment.
Too many spaces (about 4 too many)
| uct_rc_mlx5_base_ep_t *ep = ucs_derived_of(tl_ep, uct_rc_mlx5_base_ep_t); | ||
| ucs_status_t status = uct_ib_mlx5_ext_ep_outstanding_purge(tl_ep, params); | ||
|
|
||
| uct_rc_mlx5_ep_update_tx_qp_res(ep, ep->tx.wq.ft_ci); |
There was a problem hiding this comment.
Should we gate here with NO_COMPLETIONS flag in the EP? otherwise ft_ci value would be UINT16_MAX
we should also clean NO_COMPLETIONS flag after that
There was a problem hiding this comment.
When err callback returns in progress, UCP decides when to purge, clean up, and destroy the endpoint. The endpoint does not return to normal completion processing. UCT don't need touch this anymore.
| uct_ep_h tl_ep, const uct_ep_outstanding_purge_params_t *params) | ||
| { | ||
| uct_rc_mlx5_base_ep_t *ep = ucs_derived_of(tl_ep, uct_rc_mlx5_base_ep_t); | ||
| ucs_status_t status = uct_ib_mlx5_ext_ep_outstanding_purge(tl_ep, params); |
There was a problem hiding this comment.
So that API stays in EXT?
There was a problem hiding this comment.
It can be removed in the PR for the purge implementation. In this PR it keeps the behavior the same as master. IMO, cleanup plugin call is not this PR’s responsibility.
|
🤖 Starting review — findings will be posted here when done. |
| UCT_RC_MLX5_BASE_EP_DECL(tl_ep, iface, ep); | ||
|
|
||
| return uct_ib_mlx5_modify_qp_state(&iface->super.super, &ep->tx.wq.super, | ||
| IBV_QPS_ERR); |
There was a problem hiding this comment.
uct_rc_mlx5_ep_outstanding_purge asserts ep->no_comp on a public API path. This function is installed as the transport's public ep_outstanding_purge op (uct_ep_outstanding_purge). The API contract does not require the ep to have first entered the deferred-completion (no_comp) state. If a caller invokes uct_ep_outstanding_purge on an ep that never failed, ep->no_comp == 0: in debug build the ucs_assert(ep->no_comp) fires; in release build ep->tx.wq.ft_ci is still UINT16_MAX (its reset value) so uct_rc_mlx5_ep_update_tx_qp_res(ep, ft_ci) computes wrong tx-qp resources. Please guard this path (return an error, or early-out) when no_comp is not set, instead of asserting it.
There was a problem hiding this comment.
uct_rc_mlx5_ep_outstanding_purge should only be called when error callback return in progress.
| } | ||
|
|
||
| ucs_status_t uct_rc_mlx5_base_ep_fc_ctrl(uct_ep_t *tl_ep, unsigned op, | ||
| uct_rc_pending_req_t *req) |
There was a problem hiding this comment.
ft_ci passed to update_tx_qp_res on purge leaves the outstanding range's tx-qp credits unreclaimed. ft_ci is set in handle_failure to the hw_ci of the first outstanding WQE: ft_ci = prev_sw_pi - (bb_max - available). Feeding that same value back into available = bb_max - (prev_sw_pi - ft_ci) recomputes exactly the pre-failure available, so the purge returns zero tx-qp resources for the just-purged range. If the intent is to reclaim the purged WQEs, this looks like it should pass sw_pi (end of the range), not ft_ci. Can you confirm the intended value here, and whether the ep is expected to be destroyed right after so the unreclaimed credits don't matter?
There was a problem hiding this comment.
Purge will be implemented later PR. ft_ci will be updated in Purge.
|
Minor: |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Build of Root cause: Compiler output (log 02:12:31.83): The build uses Implicated commit: e428402 "UCT/IB/MLX5: address comments" (Zihao Zhao), on top of b27def8 "UCT/IB/MLX5: add support for no completions" (Zihao Zhao) — PR #11829 File: src/uct/ib/mlx5/rc/rc_mlx5_ep.c:825 (call site; repo copy line 794) — prototype at src/uct/ib/mlx5/ib_mlx5_ext.h:203 Suggested fix: Ensure #include <uct/ib/mlx5/ib_mlx5_log.h>
#include <uct/ib/mlx5/ib_mlx5_ext.h> /* <- required for
uct_ib_mlx5_ext_ep_outstanding_purge() */The branch tip already carries this include at Related: PR #11829 (this PR), PR #11668 (added
|
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| status = uct_ib_mlx5_ext_ep_outstanding_purge(tl_ep, params); | ||
|
|
||
| uct_rc_mlx5_ep_update_tx_qp_res(ep, ep->tx.wq.ft_ci); |
There was a problem hiding this comment.
ft_ci was set in handle_failure to prev_sw_pi - (bb_max - available), i.e. the CI of the first outstanding WQE. Passing it here yields available = bb_max - (prev_sw_pi - ft_ci) = bb_max - outstanding, which equals the current available, so update_tx_qp_res adds 0 — the outstanding TX resources are never restored after the purge. Is the plugin expected to advance ep->tx.wq.ft_ci (to prev_sw_pi) before returning? If so, can we add a short comment stating that contract; if not, should this pass the post-purge CI instead of the range start?
|
No regression test in the diff exercises the new deferred Note (no action): Making Note (no action): Note (no action): The signed/unsigned math in |
|
🤖 Starting review — findings will be posted here when done. |
| .ep_get_device_ep = (uct_ep_get_device_ep_func_t)ucs_empty_function_return_unsupported, | ||
| .ep_put_sgl_zcopy = uct_rc_mlx5_ep_put_sgl_zcopy, | ||
| .ep_outstanding_purge = uct_ib_mlx5_ext_ep_outstanding_purge | ||
| .ep_outstanding_purge = uct_rc_mlx5_ep_outstanding_purge |
There was a problem hiding this comment.
Rewiring .ep_outstanding_purge to uct_rc_mlx5_ep_outstanding_purge, which begins with ucs_assert(ep->err_handler_inprogress) and then reads ep->tx.wq.ft_ci, makes the public uct_ep_outstanding_purge() API path unsafe whenever the ep is not mid error-handling: it aborts in debug builds and, in release, feeds a stale/uninitialized ft_ci into uct_rc_mlx5_ep_update_tx_qp_res, corrupting QP resource accounting. The purge dispatch should tolerate err_handler_inprogress == 0 (early-return / delegate to the external purge without touching ft_ci) rather than requiring callers to route around it.
There was a problem hiding this comment.
Purge should be called only after error callback return UCS_INPROGRESS.
| IBV_QPS_ERR); | ||
| } | ||
|
|
||
| ucs_status_t uct_rc_mlx5_ep_outstanding_purge( |
There was a problem hiding this comment.
uct_rc_mlx5_ep_outstanding_purge begins with ucs_assert(ep->err_handler_inprogress) then reads ep->tx.wq.ft_ci; when the ep is not mid error-handling this aborts in debug builds and feeds a stale/uninitialized ft_ci into uct_rc_mlx5_ep_update_tx_qp_res in release builds. It should tolerate err_handler_inprogress == 0 instead of requiring callers to bypass the public API.
There was a problem hiding this comment.
Purge should be called only after error callback return UCS_INPROGRESS.
| scoped_log_handler wrap_err(wrap_errors_logger); | ||
| EXPECT_EQ(UCS_ERR_INVALID_PARAM, | ||
| uct_ep_outstanding_purge(m_e1->ep(0), ¶ms)); | ||
| uct_ib_mlx5_ext_ep_outstanding_purge(m_e1->ep(0), ¶ms)); |
There was a problem hiding this comment.
This test now calls uct_ib_mlx5_ext_ep_outstanding_purge() directly instead of the public uct_ep_outstanding_purge(). That change is a workaround for the newly-broken public entry point — it hides the regression and removes public-path coverage.
There was a problem hiding this comment.
Will be removed in later PRs.
| params.arg = &callback_invoked; | ||
|
|
||
| ASSERT_UCS_OK(uct_ep_outstanding_purge(m_e1->ep(0), ¶ms)); | ||
| ASSERT_UCS_OK(uct_ib_mlx5_ext_ep_outstanding_purge(m_e1->ep(0), ¶ms)); |
There was a problem hiding this comment.
This test now calls uct_ib_mlx5_ext_ep_outstanding_purge() directly instead of the public uct_ep_outstanding_purge(), bypassing the newly-broken public entry point and removing public-path coverage.
There was a problem hiding this comment.
Will be removed in later PRs.
|
Test coverage gap: no regression test in the diff exercises the deferred UCS_INPROGRESS -> err_handler_inprogress -> uct_rc_mlx5_ep_outstanding_purge -> uct_rc_mlx5_ep_update_tx_qp_res flow; it depends on an external plugin not present in-tree. Additionally, the plugin-less/public-API path is reachable without any plugin and is exactly the case the modified gtest now avoids. Please add a focused test for uct_ep_outstanding_purge() on an RC mlx5 ep outside error handling, or point to the CI/error-injection job that covers the supported-INPROGRESS branch. |
|
🤖 Starting review — findings will be posted here when done. |
|
Test coverage gap (still open): No regression test in the diff exercises the deferred UCS_INPROGRESS -> err_handler_inprogress -> uct_rc_mlx5_ep_outstanding_purge -> uct_rc_mlx5_ep_update_tx_qp_res flow; it depends on an out-of-tree plugin. Additionally, the existing test_ib_mlx5_ext.cc was changed to call uct_ib_mlx5_ext_ep_outstanding_purge directly instead of the public uct_ep_outstanding_purge, which sidesteps the newly-inserted wrapper and its ucs_assert(err_handler_inprogress) - masking the plugin-less public-API path rather than covering it. Please point to the CI/error-injection job that covers the supported-INPROGRESS branch, or add a focused test. |
|
🤖 Starting review — findings will be posted here when done. |
| UCT_RC_MLX5_BASE_EP_DECL(tl_ep, iface, ep); | ||
|
|
||
| return uct_ib_mlx5_modify_qp_state(&iface->super.super, &ep->tx.wq.super, | ||
| IBV_QPS_ERR); |
There was a problem hiding this comment.
uct_rc_mlx5_ep_outstanding_purge asserts ep->err_handler_inprogress, but the public uct_ep_outstanding_purge API routes through it and is reachable outside error handling (fresh ep → err_handler_inprogress == 0 → debug abort).
| bool callback_invoked = false; | ||
| uct_ep_outstanding_purge_params_t params = {}; | ||
|
|
||
| { |
There was a problem hiding this comment.
The gtest was changed from uct_ep_outstanding_purge to uct_ib_mlx5_ext_ep_outstanding_purge, sidestepping the new wrapper and its assert rather than covering the public-API path — which is exactly the reachable case in Finding 1.
|
No regression test in the diff exercises the deferred UCS_INPROGRESS → err_handler_inprogress → uct_rc_mlx5_ep_outstanding_purge → uct_rc_mlx5_ep_update_tx_qp_res flow; it depends on an out-of-tree plugin. |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The gtest process hung for exactly 900 s inside Full analysisSummary: Root cause: Log timeline proves a hang, not slowness:
Implicated commit: File: Suggested fix:
Related: PR #11829 (this PR, "UCT/IB/MLX5: support in progress error handler"); base-path context in #11821 (RC send PSN tracking) which the branch merged just before this run. |
|
@jeynmann it seems like the CI failures are related to this PR |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
I didn't change anything on |
| ucs_status_t uct_rc_mlx5_ep_outstanding_purge( | ||
| uct_ep_h tl_ep, const uct_ep_outstanding_purge_params_t *params); |
There was a problem hiding this comment.
should probably remove this, will be added in am_short purge PR
#11857
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
5 participants
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 #11684