Skip to content

UCT/IB/MLX5: support in progress error handler #11829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
roiedanino merged 20 commits into from
Sep 1, 2026
Merged

UCT/IB/MLX5: support in progress error handler #11829

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
b27def8
UCT/IB/MLX5: add support for no completions
JeynmannZ Aug 25, 2026
0a8d5c3
UCT/IB/MLX5: fix purge for handler ok path
JeynmannZ Aug 26, 2026
38e997a
UCT/IB/MLX5: rm gates for hw_ci
JeynmannZ Aug 27, 2026
4107de3
UCT/IB/MLX5: fix range in log
JeynmannZ Aug 27, 2026
6e05659
UCT/IB/MLX5: address comments
JeynmannZ Aug 28, 2026
e428402
UCT/IB/MLX5: address comments
JeynmannZ Aug 28, 2026
41e17cd
Merge branch 'openucx:master' into failover_uct_rcx_no_comp_impl
jeynmann Aug 28, 2026
9c4afd0
UCT/API: Add error handler in-progress capability
JeynmannZ Aug 28, 2026
a34966e
UCT/IB/MLX5: refactor
JeynmannZ Aug 28, 2026
b01ffa0
UCT/API: rm in progress flag and revert query
JeynmannZ Aug 28, 2026
a80d584
UCT/IB/MLX5: rm UCT_IFACE_FLAG_V2_ERR_HANDLER_INPROGRESS
JeynmannZ Aug 28, 2026
529fceb
UCT/API: fix doc build
JeynmannZ Aug 28, 2026
220b9e0
UCT/IB: cleanup
JeynmannZ Aug 28, 2026
95d6161
UCT/IB/MLX5: address comments
JeynmannZ Aug 28, 2026
04896be
Merge remote-tracking branch 'origin/master' into failover_uct_rcx_no…
JeynmannZ Aug 28, 2026
1232be4
UCT/IB/MLX5: update comments
JeynmannZ Aug 28, 2026
1ba4778
UCT/IB/MLX5: format
JeynmannZ Aug 28, 2026
7ae7ad8
UCT/IB/MLX5: format
JeynmannZ Aug 28, 2026
55b6fd9
Merge branch 'master' into failover_uct_rcx_no_comp_impl
jeynmann Aug 31, 2026
c4be7ed
UCT/IB/MLX5: address comments
JeynmannZ Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/uct/ib/mlx5/ib_mlx5.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,11 +697,12 @@ void uct_ib_mlx5_txwq_reset(uct_ib_mlx5_txwq_t *txwq)
txwq->sw_pi = 0;
txwq->prev_sw_pi = UINT16_MAX;
txwq->next_wqe_psn = 0;
txwq->hw_ci = UINT16_MAX;
txwq->ft_ci = UINT16_MAX;
txwq->path_mtu_mask = 0;
txwq->path_mtu_shift = 0;
#if UCS_ENABLE_ASSERT
txwq->hw_ci = 0xFFFF;
txwq->flags = 0;
txwq->flags = 0;
#endif
uct_ib_fence_info_init(&txwq->fi);
}
Expand Down Expand Up @@ -745,10 +746,10 @@ void uct_ib_mlx5_txwq_vfs_populate(uct_ib_mlx5_txwq_t *txwq, void *parent_obj)
UCS_VFS_TYPE_U16, "bb_max");
ucs_vfs_obj_add_ro_file(parent_obj, ucs_vfs_show_primitive, &txwq->sig_pi,
UCS_VFS_TYPE_U16, "sig_pi");
#if UCS_ENABLE_ASSERT
ucs_vfs_obj_add_ro_file(parent_obj, ucs_vfs_show_primitive, &txwq->ft_ci,
UCS_VFS_TYPE_U16, "ft_ci");
ucs_vfs_obj_add_ro_file(parent_obj, ucs_vfs_show_primitive, &txwq->hw_ci,
UCS_VFS_TYPE_U16, "hw_ci");
#endif
}

ucs_status_t
Expand Down
3 changes: 2 additions & 1 deletion src/uct/ib/mlx5/ib_mlx5.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,10 +695,11 @@ typedef struct uct_ib_mlx5_txwq {
void *qend;
uint16_t bb_max;
uint16_t sig_pi; /* PI for last signaled WQE */
uint16_t hw_ci; /* First BB index of last completed WQE */
uint16_t ft_ci; /* First BB index of last ft completed WQE */
uint16_t path_mtu_mask; /* Path MTU in bytes - 1 */
uint8_t path_mtu_shift; /* log2(path MTU in bytes) */
#if UCS_ENABLE_ASSERT
uint16_t hw_ci; /* First BB index of last completed WQE */
uint8_t flags; /* Debug flags */
#endif
uct_ib_fence_info_t fi;
Expand Down
2 changes: 0 additions & 2 deletions src/uct/ib/mlx5/ib_mlx5.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,7 @@ uct_ib_mlx5_poll_cq(uct_ib_iface_t *iface, uct_ib_mlx5_cq_t *cq, int poll_flags,
static UCS_F_ALWAYS_INLINE uint16_t
uct_ib_mlx5_txwq_update_bb(uct_ib_mlx5_txwq_t *wq, uint16_t hw_ci)
{
#if UCS_ENABLE_ASSERT
wq->hw_ci = hw_ci;
#endif
return wq->bb_max - (wq->prev_sw_pi - hw_ci);
}

Expand Down
4 changes: 4 additions & 0 deletions src/uct/ib/mlx5/rc/rc_mlx5.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ typedef struct uct_rc_mlx5_base_ep {
struct {

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using uint8_t err_handler_inprogress; instead of bool. bool is not used elsewhere in the IB ep/iface structs, and the initializer self->err_handler_inprogress = 0; assigns an integer rather than false; uint8_t matches the existing convention (e.g. uct_rc_ep_t::flags). Not blocking.

uct_ib_mlx5_txwq_t wq;
} tx;
uint8_t err_handler_inprogress;
} uct_rc_mlx5_base_ep_t;

typedef __be32 uct_rc_mlx5_tx_token_t;
Expand Down Expand Up @@ -208,6 +209,9 @@ ucs_status_t
uct_rc_mlx5_base_ep_invalidate(uct_ep_h tl_ep,
const uct_ep_invalidate_params_t *params);

ucs_status_t uct_rc_mlx5_ep_outstanding_purge(
uct_ep_h tl_ep, const uct_ep_outstanding_purge_params_t *params);

@roiedanino roiedanino Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably remove this, will be added in am_short purge PR
#11857


ucs_status_t uct_rc_mlx5_base_ep_fc_ctrl(uct_ep_t *tl_ep, unsigned op,
uct_rc_pending_req_t *req);

Expand Down
44 changes: 25 additions & 19 deletions src/uct/ib/mlx5/rc/rc_mlx5.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1931,31 +1931,36 @@ uct_rc_mlx5_iface_common_atomic_data(unsigned opcode, unsigned size, uint64_t va
}

static UCS_F_ALWAYS_INLINE void
uct_rc_mlx5_iface_update_tx_res(uct_rc_iface_t *rc_iface,
uct_rc_mlx5_base_ep_t *rc_mlx5_base_ep,
uint16_t hw_ci)
uct_rc_mlx5_iface_update_tx_cq_res(uct_rc_iface_t *rc_iface,
uct_rc_mlx5_base_ep_t *ep, uint16_t hw_ci)
{
uct_ib_mlx5_txwq_t *txwq = &rc_mlx5_base_ep->tx.wq;
uct_rc_txqp_t *txqp = &rc_mlx5_base_ep->super.txqp;
uint16_t bb_num;

bb_num = uct_ib_mlx5_txwq_update_bb(txwq, hw_ci) -
uct_rc_txqp_available(txqp);

/* Must always have positive number of released resources. The first
* completion will report bb_num=1 (because prev_sw_pi is initialized to -1)
* and all the rest report the amount of BBs the previous WQE has consumed.
*/
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;

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jeynmann jeynmann Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's strict monotonicity. HW should not generates 2 cqes for 1 wqe.

uint16_t prev_hw_ci = txwq->hw_ci;
uint16_t bb_num = hw_ci - prev_hw_ci;

uct_rc_txqp_available_add(txqp, bb_num);
ucs_assert(uct_rc_txqp_available(txqp) <= txwq->bb_max);
ucs_assertv(bb_num > 0, "hw_ci=%d prev_hw_ci=%d ft_ci=%d bb_num=%d", hw_ci,
prev_hw_ci, txwq->ft_ci, bb_num);

txwq->hw_ci = hw_ci;
uct_rc_iface_update_reads(rc_iface);
uct_rc_iface_add_cq_credits(rc_iface, bb_num);
}

static UCS_F_ALWAYS_INLINE void
uct_rc_mlx5_ep_update_tx_qp_res(uct_rc_mlx5_base_ep_t *ep, uint16_t sw_ci)
{
uct_ib_mlx5_txwq_t *txwq = &ep->tx.wq;
uct_rc_txqp_t *txqp = &ep->super.txqp;
int16_t prev_available = uct_rc_txqp_available(txqp);
uint16_t available = txwq->bb_max -
(txwq->prev_sw_pi - sw_ci);

ucs_assert(available >= prev_available);
uct_rc_txqp_available_add(txqp, available - prev_available);

ucs_assert(uct_rc_txqp_available(txqp) <= txwq->bb_max);
}

static UCS_F_ALWAYS_INLINE unsigned
uct_rc_mlx5_iface_poll_tx(uct_rc_mlx5_iface_common_t *iface, int poll_flags)
{
Expand Down Expand Up @@ -1986,7 +1991,8 @@ uct_rc_mlx5_iface_poll_tx(uct_rc_mlx5_iface_common_t *iface, int poll_flags)

uct_rc_mlx5_txqp_process_tx_cqe(&ep->super.txqp, cqe, hw_ci);
ucs_arbiter_group_schedule(&iface->super.tx.arbiter, &ep->super.arb_group);
uct_rc_mlx5_iface_update_tx_res(&iface->super, ep, hw_ci);
uct_rc_mlx5_ep_update_tx_qp_res(ep, hw_ci);
uct_rc_mlx5_iface_update_tx_cq_res(&iface->super, ep, hw_ci);
uct_rc_iface_arbiter_dispatch(&iface->super);
uct_ib_mlx5_update_db_cq_ci(&iface->cq[UCT_IB_DIR_TX]);

Expand Down
20 changes: 20 additions & 0 deletions src/uct/ib/mlx5/rc/rc_mlx5_ep.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#endif

#include <uct/ib/mlx5/ib_mlx5_log.h>
#include <uct/ib/mlx5/ib_mlx5_ext.h>
#include <ucs/vfs/base/vfs_cb.h>
#include <ucs/vfs/base/vfs_obj.h>
#include <ucs/arch/cpu.h>
Expand Down Expand Up @@ -817,6 +818,24 @@ ucs_status_t uct_rc_mlx5_base_ep_invalidate(uct_ep_h tl_ep,
IBV_QPS_ERR);

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jeynmann jeynmann Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uct_rc_mlx5_ep_outstanding_purge should only be called when error callback return in progress.

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

ucs_status_t uct_rc_mlx5_ep_outstanding_purge(

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocker: this replaces the plugin dispatch (uct_ib_mlx5_ext_ep_outstanding_purge) with a function that never calls the registered plugin, so the public uct_ep_outstanding_purge no longer validates rx_token or invokes params->cb, and always returns UCS_OK. This also breaks the existing test_uct_ib_mlx5_ext_rc.ep_outstanding_purge gtest (rc_mlx5), which registers plugins and expects the plugin callback to run — it will now trip ucs_assert(err_handler_inprogress). How is the plugin purge (token validation + per-op cb) still reached after the error path defers with err_handler_inprogress?

@jeynmann jeynmann Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will impl uct_rc_mlx5_ep_outstanding_purge in later PRs.

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jeynmann jeynmann Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purge should be called only after error callback return UCS_INPROGRESS.

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;

ucs_assert(ep->err_handler_inprogress);

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocker: uct_ep_outstanding_purge is a public entry point that can be called outside the error handler. Here err_handler_inprogress is 0 and ft_ci is unset, so this asserts in debug and reads a stale ft_ci (feeding update_tx_qp_res) in release. pls gate on the flag and return an error / dispatch to the plugin instead of asserting.


/* TODO: Implement purge and replace the external purge call. */
status = uct_ib_mlx5_ext_ep_outstanding_purge(tl_ep, params);

/* The purge function advances ft_ci as each WQE is processed. Reclaim TX
Comment thread
jeynmann marked this conversation as resolved.
Outdated
* resources up to ft_ci. */
uct_rc_mlx5_ep_update_tx_qp_res(ep, ep->tx.wq.ft_ci);

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


return status;
}

ucs_status_t uct_rc_mlx5_base_ep_fc_ctrl(uct_ep_t *tl_ep, unsigned op,
uct_rc_pending_req_t *req)

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@jeynmann jeynmann Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purge will be implemented later PR. ft_ci will be updated in Purge.

{
Expand Down Expand Up @@ -1242,6 +1261,7 @@ UCS_CLASS_INIT_FUNC(uct_rc_mlx5_base_ep_t, const uct_ep_params_t *params)

UCS_CLASS_CALL_SUPER_INIT(uct_rc_ep_t, &iface->super,
self->tx.wq.super.qp_num, params);
self->err_handler_inprogress = 0;

if (self->tx.wq.super.type == UCT_IB_MLX5_OBJ_TYPE_VERBS) {
status = uct_rc_iface_qp_init(&iface->super,
Expand Down
47 changes: 41 additions & 6 deletions src/uct/ib/mlx5/rc/rc_mlx5_iface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,10 @@ void uct_rc_mlx5_iface_handle_failure(uct_ib_iface_t *ib_iface, void *arg,
qp_num),
uct_rc_mlx5_base_ep_t);
uint16_t pi = ntohs(cqe->wqe_counter);
#if UCS_ENABLE_ASSERT
uct_iface_attr_v2_t iface_attr;
ucs_status_t query_status;
#endif
ucs_log_level_t log_lvl;
ucs_status_t status;

Expand All @@ -203,27 +207,58 @@ void uct_rc_mlx5_iface_handle_failure(uct_ib_iface_t *ib_iface, void *arg,
goto out;
}

uct_rc_txqp_purge_outstanding(iface, &ep->super.txqp, ep_status, pi, 0);
ucs_arbiter_group_purge(&iface->tx.arbiter, &ep->super.arb_group,
uct_rc_ep_arbiter_purge_internal_cb, NULL);
uct_rc_mlx5_iface_update_tx_res(iface, ep, pi);
uct_ib_mlx5_txwq_update_flags(&ep->tx.wq, UCT_IB_MLX5_TXWQ_FLAG_FAILED, 0);

if (ep->super.flags & (UCT_RC_EP_FLAG_ERR_HANDLER_INVOKED |
UCT_RC_EP_FLAG_FLUSH_CANCEL)) {
goto out;
goto out_update_tx_res;
}

ep->super.flags |= UCT_RC_EP_FLAG_ERR_HANDLER_INVOKED;
uct_rc_fc_restore_wnd(iface, &ep->super.fc);

status = uct_iface_handle_ep_err(&iface->super.super.super,
&ep->super.super.super, ep_status);
log_lvl = uct_base_iface_failure_log_level(&ib_iface->super, status,
ep_status);

if (status == UCS_INPROGRESS) {
#if UCS_ENABLE_ASSERT
iface_attr.field_mask = UCT_IFACE_ATTR_FIELD_CAP_FLAGS;

query_status = uct_iface_query_v2(&iface->super.super.super,
&iface_attr);
ucs_assert(query_status == UCS_OK);
ucs_assert(iface_attr.cap.flags & UCT_IFACE_FLAG_V2_QUERY_TOKEN);
#endif

/* Save last completed WQE. TX QP resources are reserved until purge. */
ep->tx.wq.ft_ci = ep->tx.wq.prev_sw_pi -
(ep->tx.wq.bb_max -
uct_rc_txqp_available(&ep->super.txqp));
ep->err_handler_inprogress = 1;

ucs_debug("ep %p outstanding WQE range (%u, %u)", ep, ep->tx.wq.ft_ci,
Comment thread
jeynmann marked this conversation as resolved.
ep->tx.wq.sw_pi);

log_lvl = uct_base_iface_failure_log_level(&ib_iface->super, UCS_OK,
ep_status);
} else {
log_lvl = uct_base_iface_failure_log_level(&ib_iface->super, status,
ep_status);
}

Comment thread
jeynmann marked this conversation as resolved.

uct_ib_mlx5_completion_with_err(ib_iface, arg, &ep->tx.wq, log_lvl);

out_update_tx_res:
if (!(ep->err_handler_inprogress)) {
uct_rc_txqp_purge_outstanding(iface, &ep->super.txqp, ep_status, pi, 0);
uct_rc_mlx5_ep_update_tx_qp_res(ep, pi);
}

uct_rc_mlx5_iface_update_tx_cq_res(iface, ep, pi);

out:
uct_rc_iface_arbiter_dispatch(iface);
}
Expand Down Expand Up @@ -1204,7 +1239,7 @@ static uct_rc_iface_ops_t uct_rc_mlx5_iface_ops = {
.ep_is_connected = uct_rc_mlx5_base_ep_is_connected,
.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

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jeynmann jeynmann Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purge should be called only after error callback return UCS_INPROGRESS.

},
.create_cq = uct_rc_mlx5_iface_common_create_cq,
.destroy_cq = uct_rc_mlx5_iface_common_destroy_cq,
Expand Down
4 changes: 2 additions & 2 deletions test/gtest/uct/ib/test_ib_mlx5_ext.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ UCS_TEST_P(test_uct_ib_mlx5_ext_rc, ep_outstanding_purge)
{

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

scoped_log_handler wrap_err(wrap_errors_logger);
EXPECT_EQ(UCS_ERR_INVALID_PARAM,
uct_ep_outstanding_purge(m_e1->ep(0), &params));
uct_ib_mlx5_ext_ep_outstanding_purge(m_e1->ep(0), &params));

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jeynmann jeynmann Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be removed in later PRs.

}

register_plugin("stub");
Expand All @@ -118,7 +118,7 @@ UCS_TEST_P(test_uct_ib_mlx5_ext_rc, ep_outstanding_purge)
params.cb = purge_cb;
params.arg = &callback_invoked;

ASSERT_UCS_OK(uct_ep_outstanding_purge(m_e1->ep(0), &params));
ASSERT_UCS_OK(uct_ib_mlx5_ext_ep_outstanding_purge(m_e1->ep(0), &params));

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jeynmann jeynmann Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be removed in later PRs.

EXPECT_TRUE(callback_invoked);
}

Expand Down
Loading