[WIP][DNM] UCT/IB/MLX5: wqe parser for uct flush - #11844
Conversation
|
🤖 Starting review — findings will be posted here when done. |
| uint32_t qp_num; | ||
| } uct_rc_mlx5_mp_hash_key_t; | ||
|
|
||
| #define UCT_RC_MLX5_RMA_MAX_IOV(_av_size) \ |
There was a problem hiding this comment.
blocker — UCT_RC_MLX5_RMA_MAX_IOV is already defined earlier in this same header (line 138). This adds a second, whitespace-only-different definition of the identical macro. Please drop this block; the macro already exists a few lines above at line 138.
| { | ||
| *skip_p = 0; | ||
|
|
||
| switch (uct_ib_mlx5_wqe_opcode(ctrl)) { |
There was a problem hiding this comment.
blocker (likely CI failure) — the case MLX5_OPCODE_NOP:/case MLX5_OPCODE_RDMA_READ: body only returns when op is a flush; otherwise it falls through into default:. UCX builds with -Wall -Werror, and GCC's -Wimplicit-fallthrough (enabled by -Wall) will error on this implicit fall-through. The rest of the mlx5 code annotates intentional fall-through with /* Fall through */ (see rc_mlx5.inl:577,699,895). Please either add the annotation or restructure so the non-flush path returns UCS_ERR_UNSUPPORTED explicitly:
switch (uct_ib_mlx5_wqe_opcode(ctrl)) {
case MLX5_OPCODE_NOP:
case MLX5_OPCODE_RDMA_READ:
if (uct_rc_mlx5_op_info_is_flush(op)) {
uct_rc_mlx5_op_info_fill_flush(info, op);
return UCS_OK;
}
/* Fall through */
default:
ucs_diag("unsupported op %d", uct_ib_mlx5_wqe_opcode(ctrl));
return UCS_ERR_UNSUPPORTED;
}| #include <uct/ib/rc/base/rc_iface.h> | ||
| #include <ucs/arch/bitops.h> | ||
| #include <ucs/profile/profile.h> | ||
| #include <endian.h> |
There was a problem hiding this comment.
minor — #include <endian.h> and #include <string.h> are redundant; both are already pulled in via ib_mlx5.h (lines 42-43), included transitively through rc_mlx5.inl.
| } | ||
|
|
||
| ucs_status_t | ||
| uct_rc_mlx5_op_info_fill(uct_ep_op_info_t *info, const uct_ib_mlx5_txwq_t *txwq, |
There was a problem hiding this comment.
minor — uct_rc_mlx5_op_info_fill takes txwq, wqe_size, and callback_data but the body uses none of them. They appear to be reserved for future RMA-op decoding by the plugin. If they are part of a committed plugin ABI that lands later, a one-line comment noting that would help; otherwise consider dropping them until they are used.
|
🤖 Starting review — findings will be posted here when done. |
| return ctrl->opmod_idx_opcode >> 24; | ||
| } | ||
|
|
||
| void uct_ib_mlx5_txwq_copy_segs(const uct_ib_mlx5_txwq_t *txwq, const void *src, |
There was a problem hiding this comment.
uct_ib_mlx5_txwq_copy_segs has no callers in this PR. can we add it together with the code that uses it?
|
|
||
| uint8_t uct_ib_mlx5_wqe_opcode(const struct mlx5_wqe_ctrl_seg *ctrl); | ||
|
|
||
| void uct_ib_mlx5_txwq_copy_segs(const uct_ib_mlx5_txwq_t *txwq, const void *src, |
There was a problem hiding this comment.
Dead code / unused infrastructure: declaration of uct_ib_mlx5_txwq_copy_segs, which has no callers anywhere in the tree. Adding an unused helper + its declaration in a feature PR goes against the "keep the implementation as small as possible / remove anything unused" guidance in REVIEW.md.
| } | ||
|
|
||
| ucs_status_t | ||
| uct_rc_mlx5_op_info_fill(uct_ep_op_info_t *info, const uct_ib_mlx5_txwq_t *txwq, |
There was a problem hiding this comment.
txwq, wqe_size, and callback_data are unused here (and uct_rc_mlx5_op_callback_data_t is only ever passed as this unused arg). can we drop them until the path that needs them lands, to keep this PR scoped?
| uct_rc_mlx5_op_info_fill_flush(info, op); | ||
|
|
||
| return UCS_OK; | ||
| } |
There was a problem hiding this comment.
minor: pls add a /* fall through */ comment here — the intended path when op is not a flush is non-obvious. Note: this does not break the build, since UCX compiles with -Wall -Werror but not -Wextra, so -Wimplicit-fallthrough is not active.
| while (ci != txwq->sw_pi) { | ||
| ctrl = static_cast<const struct mlx5_wqe_ctrl_seg*>( | ||
| uct_ib_mlx5_txwq_get_wqe(txwq, ci)); | ||
| wqe_size = (ctrl->qpn_ds >> 24) * UCT_IB_MLX5_WQE_SEG_SIZE; |
There was a problem hiding this comment.
minor: wqe_size = (ctrl->qpn_ds >> 24) * UCT_IB_MLX5_WQE_SEG_SIZE; re-derives the WQE ds count inline. The production uct_rc_mlx5_op_info_fill receives wqe_size as a parameter but ignores it, and the test computes its own. If a helper is intended for this, reusing it would avoid drift.
|
🤖 CI Triage Agent — TL;DR: The Coverity release job failed on a single new defect — MISSING_BREAK at Full analysisSummary: Root cause: In
This function is brand new in this PR branch ( Implicated commit: File: Suggested fix: Make the non-flush path exit the switch explicitly rather than falling through. E.g.: switch (uct_ib_mlx5_wqe_opcode(ctrl)) {
case MLX5_OPCODE_NOP:
case MLX5_OPCODE_RDMA_READ:
if (uct_rc_mlx5_op_info_is_flush(op)) {
uct_rc_mlx5_op_info_fill_flush(info, op);
return UCS_OK;
}
break;
default:
break;
}
ucs_diag("unsupported op %d", uct_ib_mlx5_wqe_opcode(ctrl));
return UCS_ERR_UNSUPPORTED;This is behaviour-preserving and removes the implicit fallthrough entirely (preferable to adding a Related: PR #11844 (this build); no existing issue found for this defect.
|
4 participants
What?
Add wqe parse support for uct flush.
Why?
FT need extract info from wqe before replay uct flush.
How?
Add parser for uct flush local/remote