[DNM] UCT/IB/MLX5: wqe parser for uct put - #11836
Conversation
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Build of Root cause: Incomplete refactor in the PR branch. The helper was refactored to a 5-parameter form: but the only caller inside GCC reports two errors on this: argument 5 is Implicated commit: File: Suggested fix: Update the call site to match the new signature — pass the struct pointer and drop the redundant size argument, since the callee already derives the bound via if (inl->byte_count & htonl(MLX5_INLINE_SEG)) {
uct_rc_mlx5_op_info_fill_put_short(info, txwq, inl, raddr,
callback_data);
return UCS_OK;
}Then rebuild locally with the same flags the CI uses ( Related: PR #11836 (branch
|
026c3c2 to
535cec7
Compare
|
🤖 Starting review — findings will be posted here when done. |
| int *skip_p, | ||
| uct_rc_mlx5_op_callback_data_t *callback_data) | ||
| { | ||
| *skip_p = 0; |
There was a problem hiding this comment.
uct_rc_mlx5_op_info_fill: *skip_p = 0 is written unconditionally and never set to 1 anywhere in the function or its callees. The skip_p output parameter has no effect in-tree. Why do we need skip_p? It is always set to 0 and never to 1 in this PR.
| #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: <endian.h> and <string.h> are already pulled in transitively via ib_mlx5.h (through rc_mlx5.inl). The explicit includes are redundant.
|
|
||
| inl = uct_ib_mlx5_txwq_wrap_any((uct_ib_mlx5_txwq_t*)txwq, | ||
| (void*)(raddr + 1)); | ||
| if (inl->byte_count & htonl(MLX5_INLINE_SEG)) { |
There was a problem hiding this comment.
uct_rc_mlx5_op_info_fill_put: A keepalive/flush-check posts MLX5_OPCODE_RDMA_WRITE with a 0-length inline seg and remote_addr=0 (uct_rc_mlx5_base_ep_post_check); it will be reported to the purge callback as UCT_EP_OP_PUT_SHORT with remote_addr=0. This is an edge case a purge consumer could observe though it is not a real user PUT; worth confirming the purge consumer tolerates this synthetic PUT.
|
Test coverage: the gtest exercises PUT_SHORT / PUT_BCOPY / PUT_ZCOPY reconstruction only when an ext plugin is registered that returns success — otherwise it skips. In upstream CI (no plugin) these three cases are effectively no-ops, so the parsing helpers are not exercised by CI. Worth noting to the author, though may be acceptable given the consumer is out-of-tree. |
|
🤖 Starting review — findings will be posted here when done. |
| uct_rc_mlx5_op_info_fill(uct_ep_op_info_t *info, const uct_ib_mlx5_txwq_t *txwq, | ||
| uct_rc_iface_send_op_t *op, | ||
| const struct mlx5_wqe_ctrl_seg *ctrl, size_t wqe_size, | ||
| int *skip_p, |
There was a problem hiding this comment.
skip_p is passed here but never written by this function or its callees. Is the skip path unfinished? If it's meant as an out-param the caller will read uninitialized data; if unused, pls drop it.
| { | ||
| size_t iovcnt; | ||
|
|
||
| ucs_assert(seg_size >= 0); |
There was a problem hiding this comment.
ucs_assert(seg_size >= 0) is always true because seg_size is size_t (unsigned). The assert checks nothing; if the intent is to guard the == 0 path this can just be removed.
| void uct_ib_mlx5_txwq_copy_segs(const uct_ib_mlx5_txwq_t *txwq, const void *src, | ||
| void *dst, size_t length) | ||
| { | ||
| size_t copy_len = ucs_min(length, UCS_PTR_BYTE_DIFF(src, txwq->qend)); |
There was a problem hiding this comment.
Mixed-sign ucs_min in uct_rc_mlx5_txwq_copy_segs: ucs_min(length /* size_t */, UCS_PTR_BYTE_DIFF(src, txwq->qend) /* ptrdiff_t */) mixes unsigned and signed. Callers pass src inside [qstart, qend) via txwq_wrap_any, so the diff is positive today, but the mixed-sign comparison is fragile. Consider casting the diff to size_t after the wrap invariant, or adding an assert that src < qend. Following the uct_ib_mlx5_inline_copy pattern (consistent ptrdiff_t n) would be more consistent.
|
The three |
|
🤖 Starting review — findings will be posted here when done. |
| { | ||
| size_t iovcnt; | ||
|
|
||
| ucs_assert(seg_size >= 0); |
There was a problem hiding this comment.
Tautological assertion: ucs_assert(seg_size >= 0); — seg_size is size_t (unsigned), so this comparison is always true and will likely produce compiler/linter warnings. Drop the assert; the seg_size == 0 case is already handled right below.
| uct_rc_mlx5_op_info_fill(uct_ep_op_info_t *info, const uct_ib_mlx5_txwq_t *txwq, | ||
| uct_rc_iface_send_op_t *op, | ||
| const struct mlx5_wqe_ctrl_seg *ctrl, size_t wqe_size, | ||
| int *skip_p, |
There was a problem hiding this comment.
skip_p output parameter is never written. uct_rc_mlx5_op_info_fill() takes int *skip_p but no path ever assigns *skip_p. The caller (and the gtest, which pre-sets skip = 0 then EXPECT_EQ(0, skip)) can't learn whether an op should be skipped. Either set *skip_p on the paths that need to skip (e.g. NOP/zero-length or unsupported cases), or remove the parameter. As-is it's a dead output that gives out-of-tree callers a false contract.
6654193 to
ab25169
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| uct_rc_mlx5_op_info_fill_rma_raddr(info, raddr); | ||
|
|
||
| info->operation = is_dm ? UCT_EP_OP_PUT_SHORT : UCT_EP_OP_PUT_BCOPY; |
There was a problem hiding this comment.
Minor: DM put_bcopy reports UCT_EP_OP_PUT_SHORT with a device-memory buffer. In uct_rc_mlx5_op_info_fill_put_bcopy, when is_dm is true, info->rma.payload.data.buffer is set to op->buffer, which for a DM descriptor points into device memory (start_va + offset). A consumer doing a host-side read of payload.data.buffer inside the callback (as the gtest's non-zcopy path does with memcmp) would read device memory. Is a host-side read of payload.data.buffer by the consumer expected to work here?
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The ASAN/RoCE gtest job failed on a single test out of 8616 — Full analysisSummary: Root cause: Evidence from the log tail: The failing test is the only test that allocates Nothing in this PR plausibly reaches this path: the PR commits are Caveat: the exact assertion text is not recoverable — the log fetch returns only the final ~30 s of a ~44-minute run, and the failure occurred much earlier. The above is inferred from the summary line plus the test's source and history. Implicated commit: none in this PR — most likely environmental/flaky. Historical context: File: Suggested fix:
Related: PR #11836 (this PR,
|
jeynmann
changed the title
|
🤖 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. |
|
|
||
| uct_rc_mlx5_op_info_fill_rma_raddr(info, raddr); | ||
|
|
||
| info->operation = is_dm ? UCT_EP_OP_PUT_SHORT : UCT_EP_OP_PUT_BCOPY; |
There was a problem hiding this comment.
minor: In uct_rc_mlx5_op_info_fill_put_bcopy, the function returns UCS_ERR_UNSUPPORTED at lines 138-140 when is_dm is true, so is_dm is always false when reaching line 148. The ternary is_dm ? UCT_EP_OP_PUT_SHORT : UCT_EP_OP_PUT_BCOPY is dead — the operation is always UCT_EP_OP_PUT_BCOPY.
| info->operation = is_dm ? UCT_EP_OP_PUT_SHORT : UCT_EP_OP_PUT_BCOPY; | |
| info->operation = UCT_EP_OP_PUT_BCOPY; |
is_dm is always false here since the function already returned UCS_ERR_UNSUPPORTED above when is_dm is set. Similarly, uct_rc_mlx5_get_dptr_buffer computes op->buffer only to discard it in the is_dm case.
|
🤖 CI Triage Agent — TL;DR: The Coverity release gate failed with one DEADCODE defect at Full analysisSummary: Root cause: In 137: uct_rc_mlx5_get_dptr_buffer(op, dptr, &buffer, &length, &is_dm);
138: if (is_dm) {
139: return UCS_ERR_UNSUPPORTED;
140: }Any code after line 140 is only reachable when Implicated commit: File: Suggested fix: Drop the now-redundant ternary so the operation is set unconditionally: info->operation = UCT_EP_OP_PUT_BCOPY;
info->field_mask |= UCT_EP_OP_INFO_FIELD_OPERATION;While there, consider whether Related: PR #11836 (branch
|
b254a03 to
9355f4c
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
4 participants
What?
Add wqe parse support for uct put.
Why?
FT need extract info from wqe before replay uct put.
How?
Add parser for uct put short/bcopy/zcopy.