Repository navigation
UCT/IB/MLX5: Purge outstanding AM short operations - #11857
Conversation
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| size_t uct_ib_mlx5_wqe_size(const struct mlx5_wqe_ctrl_seg *ctrl) | ||
| { | ||
| uint8_t ds = ntohl(ctrl->qpn_ds) & UINT8_MAX; |
There was a problem hiding this comment.
The old num_posted_wqes used (ctrl->qpn_ds >> 24), which extracts ds only on little-endian; the new ntohl(ctrl->qpn_ds) & UINT8_MAX is correct on both endiannesses. This is effectively a big-endian bug fix and should be called out.
|
This PR adds >500 lines and bundles the qpn_ds/wqe-size refactor with the new native purge. Per the size guideline it's hard to review as one change; consider splitting the wqe-size/next_ci refactor (a behavior fix on big-endian) into a separate PR. ib_mlx5_log.c:311 still uses ctrl->qpn_ds >> 24 and was not refactored to use the new uct_ib_mlx5_wqe_size()/opcode helpers, leaving it inconsistent and incorrect for ds on big-endian. Consider routing it through the helpers too. The new test_rc_mlx5_outstanding_purge.am_short_bcopy test requires a DEVX-capable mlx5 device and skips otherwise, so the new native path is only exercised on such hardware in CI. CI/hardware coverage could not be verified. |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Azure Pipelines Root cause: In the new Implicated commit: [REDACTED:Hex High Entropy String] — "UCT/IB/MLX5: Purge outstanding AM short and bcopy operations", Roie Danino (the commit under test; the file's prior revision 8282944 did not contain this loop) File: src/uct/ib/mlx5/rc/rc_mlx5_ep.c:1001 (loop at 995–1020; Suggested fix: Remove the dead store by inverting the condition so for (ci = start_ci; ci != end_ci;
ci = uct_ib_mlx5_txwq_next_ci(ci, wqe_size)) {
ctrl = uct_ib_mlx5_txwq_get_wqe(txwq, ci);
wqe_size = uct_ib_mlx5_wqe_size(ctrl);
opcode = uct_ib_mlx5_wqe_opcode(ctrl);
if (opcode != MLX5_OPCODE_NOP) {
payload_length = uct_ib_mlx5_send_wqe_payload_length(txwq, ctrl,
wqe_size);
num_packets = uct_rc_mlx5_num_packets(txwq, payload_length);
delivery_status = uct_ib_mlx5_psn_delivery_status(
first_psn, receiver_next_psn, num_packets);
if (delivery_status == UCS_ERR_INVALID_PARAM) {
return delivery_status;
}
first_psn = (first_psn + num_packets) & UCT_IB_MLX5_PSN_MASK;
}
status = uct_rc_mlx5_ep_validate_purge_wqe(
txwq, txqp, ctrl, wqe_size, ci, &info, &skip, callback_data);
if (status != UCS_OK) {
return status;
}
}Optionally scope Related: PR #11857 (this change); predecessors touching the same code path: #11821 (RC send PSN with path MTU), #11804 (token query support), #11668 (outstanding purge implementation).
|
461a2d0 to
fefd444
Compare
|
🤖 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.
Plugin purge providers are no longer consulted for rc_mlx5 (behavior change). Previously .ep_outstanding_purge was uct_ib_mlx5_ext_ep_outstanding_purge, which iterated registered extension plugins. The new native uct_rc_mlx5_ep_outstanding_purge returns UCS_ERR_UNSUPPORTED for any WQE that is not SEND/NOP (e.g. RDMA/atomic) and never falls back to the ext plugin chain. Is dropping the ext-plugin dispatch here intentional? Registered plugins no longer see rc_mlx5 purge requests.
fefd444 to
4f92ec2
Compare
|
🤖 Starting review — findings will be posted here when done. |
roiedanino
changed the title
| @@ -857,6 +857,59 @@ void *uct_ib_mlx5_txwq_get_wqe(const uct_ib_mlx5_txwq_t *txwq, uint16_t pi) | |||
| return UCS_PTR_BYTE_OFFSET(txwq->qstart, (pi % num_bb) * MLX5_SEND_WQE_BB); | |||
There was a problem hiding this comment.
Asymmetric endianness handling between the two new helpers is worth a one-line note. uct_ib_mlx5_wqe_size() correctly reads ds via ntohl(ctrl->qpn_ds) & UINT8_MAX (fixing the big-endian bug in the old >> 24), but uct_ib_mlx5_wqe_opcode() deliberately keeps ctrl->opmod_idx_opcode >> 24. This is correct because opmod_idx_opcode is built with host-order shifts rather than a plain htonl, so the opcode is only in the top byte on little-endian. The asymmetry looks like an oversight but is intentional — worth a short comment noting the opcode field is written in host order (unlike qpn_ds), so the raw shift is intended and not a missed BE fix. Optional.
4f92ec2 to
c11223b
Compare
|
🤖 Starting review — findings will be posted here when done. |
| return UCS_PTR_BYTE_OFFSET(txwq->qstart, (pi % num_bb) * MLX5_SEND_WQE_BB); | ||
| } | ||
|
|
||
| uint8_t uct_ib_mlx5_wqe_opcode(const struct mlx5_wqe_ctrl_seg *ctrl) |
There was a problem hiding this comment.
uct_ib_mlx5_wqe_opcode reads ctrl->opmod_idx_opcode >> 24 without ntohl, relying on the host-order layout used by uct_ib_mlx5_set_ctrl_seg. This matches the existing ib_mlx5_log.c convention, so it is consistent, but since this PR consolidates WQE-field access into helpers, add a short comment noting the field is written in host order (not network order) to avoid a future reader 'fixing' it with ntohl.
|
Coverage note: the new test_rc_mlx5_outstanding_purge.am_short test skips unless DEVX is available, so the native purge path (WQE parsing, PSN range ambiguity rejection, pure-grant skipping) is only exercised on DEVX-capable mlx5 hardware. uct_ib_mlx5_psn_delivery_status has hardware-independent unit coverage, but the end-to-end purge has no non-DEVX coverage. CI/hardware coverage for that path could not be verified. |
c11223b to
ab9e2d6
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 Starting review — findings will be posted here when done. |
|
PR size/scope: The PR is >500 added lines and bundles the qpn_ds/wqe-size refactor with the new native purge. Per the size guideline, splitting the wqe-size/next_ci refactor (a big-endian behavior fix) into a separate PR would ease review. This aligns with REVIEW.md ("Refactoring must be in a separate PR" and the 500-line hard limit). Consistency (ib_mlx5_log.c:308): Residual coverage gap: |
|
@jeynmann can you please also review? |
8e62b1c to
c369f13
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 Starting review — findings will be posted here when done. |
|
Not repeated (verified still present in this revision): |
|
🤖 CI Triage Agent — TL;DR: The Coverity job never built anything — the "Get sources" step failed 15 times with Full analysisSummary: Azure Pipelines build 136072 ( Root cause: The checkout ran Implicated commit: unknown — no repository commit is implicated; the failure precedes source checkout. The regression is in the CI agent/container environment (agent File: none in the repo — failure is in the Azure Suggested fix: Fix the agent/image environment rather than the PR:
Related: none found — a search of the project's issues/PRs for this signature returned no matches.
|
|
🤖 Starting review — findings will be posted here when done. |
|
pls return an error instead of aborting on the docs/CodeStyle.md: "Use
minor: in the pure-grant check in Not repeated here (verified still present, belongs on existing threads): non- Not repeated here (belongs on existing thread): Not repeated here (belongs on existing thread): silent Not repeated here (belongs on existing thread): Not repeated here (belongs on existing thread): naming of the static rc helpers still prefixed Not repeated here (belongs on existing thread): deleting Not repeated here (belongs on existing thread): PR size/scope (>500 added lines, refactor bundled with the feature). Residual coverage gap: Residual coverage gap: the test skips unless Residual coverage gap: |
| uct_rc_txqp_purge_outstanding(&iface->super, &ep->super.txqp, | ||
| UCS_ERR_CANCELED, txwq->prev_sw_pi, 0); |
There was a problem hiding this comment.
we should purge / call user cb in order
| } | ||
| } | ||
|
|
||
| out_purge: |
There was a problem hiding this comment.
| out_purge: | |
| out: |
|
🤖 Starting review — findings will be posted here when done. |
|
Deleting
CI/hardware coverage could not be verified from the checkout (no network access in this session; the worktree is at the base commit, so all diff-level claims are based on the provided diff plus base context). |
|
🤖 Starting review — findings will be posted here when done. |
|
On the removed
Silent
In Non-
Naming/const issues: Deleting PR size/scope concern: >500 added lines, with a refactor bundled together with the feature. Coverage gap:
|
|
🤖 Starting review — findings will be posted here when done. |
| EXPECT_GT(ctx.num_ops_purged, 0u); | ||
| EXPECT_LT(ctx.num_ops_purged, num_posted); | ||
| EXPECT_EQ(unsigned(flush_outstanding), ctx.num_flush_purged); | ||
| EXPECT_LE(ctx.num_ops_purged, num_posted); |
There was a problem hiding this comment.
can we keep a deterministic check that already-delivered operations are not reported? dropping the initial flush() and relaxing EXPECT_LT to EXPECT_LE means this test passes even if the PSN delivery check reports every outstanding WQE - e.g. count messages in am_handler and expect ctx.num_ops_purged <= num_posted - num_received.
Problem/impact: the delivery filtering (uct_ib_mlx5_wqe_is_delivered() / the receiver_next_psn comparison) is the core of the feature, and after this change nothing in the suite fails if it stops excluding delivered WQEs. The new am_handler() discards the payload without counting, so the information needed for the assertion is available but unused. Severity: no prefix (should fix / worth discussing), not a blocker.
|
Still present (belongs on the existing threads): Still present: silent Still present: Still present: non- Still present: Still present: Residual coverage gap: Residual coverage gap: CI status could not be verified from this checkout (no network access; the worktree is at the base commit, so all diff-level claims come from the provided diff plus base context). |
What?
Add native RC mlx5 outstanding-operation purge support for active messages posted with
uct_ep_am_short().The purge path:
Why?
Fault-tolerance recovery needs the transport to report AM short operations that were not delivered after an endpoint is invalidated. RC mlx5 currently lacks native purge support for this operation.
This replaces the narrowly scoped UCT purge work from #11785.
How?
Parse the outstanding send queue without adding send-time PSN bookkeeping. First validate the outstanding WQE range and derive packet PSNs from the existing
next_wqe_psn, then report undelivered AM short operations through the UCT purge callback.