Repository navigation
UCT/IB/MLX5: support rma get in purge outstanding - #11866
Conversation
|
🤖 Starting review — findings will be posted here when done. |
|
Coverage note: The gtest ( |
|
🤖 CI Triage Agent — TL;DR: The new gtest Full analysisSummary: Azure job "BlueField on worker 1" failed in Root cause: The test added by this PR calls
So a 64-byte Implicated commit: File: Suggested fix: Derive the zcopy length from the interface attributes instead of hard-coding it, e.g.: UCS_TEST_SKIP_COND_P(test_rc_purge_outstanding, get_zcopy,
!check_caps(UCT_IFACE_FLAG_GET_ZCOPY))
{
test_get(UCT_EP_OP_GET_ZCOPY,
ucs_max(64ul, m_e1->iface_attr().cap.get.min_zcopy));
}(any length > Related: PR #11866 (this change); sibling PRs #11836 (put parser) and #11844 (flush parser) use the same helper and may need the same length guard. |
174dcfd to
7a7881c
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage note: the gtest instantiates rc_mlx5 only; dc_mlx5 and the AV-segment path are untested. Also, only MLX5_OPCODE_RDMA_READ (GET) is implemented — uct_rc_mlx5_op_info_fill ucs_fatal's on any other opcode, so PUT/AM/ATOMIC purge paths are not yet handled; confirm that matches the intended scope of this PR. |
|
🤖 CI Triage Agent — TL;DR: The aarch64 build failed to compile Full analysisSummary: Root cause: New gtest added by this PR uses a non-existent struct member:
Implicated commit: File: Suggested fix: Remove the bogus minimum-bcopy lookup and just clamp the fixed length to the max: UCS_TEST_SKIP_COND_P(test_rc_purge_outstanding, get_bcopy,
!check_caps(UCT_IFACE_FLAG_GET_BCOPY))
{
test_get(UCT_EP_OP_GET_BCOPY,
ucs_min(8ul, m_e1->iface_attr().cap.get.max_bcopy));
}(The Related: PR #11866 (this PR). No existing issue found referencing
|
7a7881c to
22afd1c
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage note (matches prior reviews): the gtest instantiates |
2dc337c to
da77720
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| UCT_INSTANTIATE_RC_DC_TEST_CASE(test_rc_srq); | ||
|
|
||
| class test_rc_purge_outstanding : public test_rc { |
There was a problem hiding this comment.
As discussed, please replace with a stress test.
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: From the log, the run completed normally (8715 tests, 3495 s, continuous output, no hang, no timeout, no crash/signal, no sanitizer report) with a single failing case: Implicated commit: unknown — no evidence links File: test/gtest/ucp/test_ucp_peer_failure.cc:392-429 (assertions in Suggested fix:
Related: #6762 (
|
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Starting review — findings will be posted here when done. |
c77e28c to
719c18f
Compare
|
DC/AV segment: Coverage: only |
|
🤖 Starting review — findings will be posted here when done. |
|
uct_rc_mlx5_op_info_fill_get computes raddr = ctrl + 1 and wqe_size - sizeof(ctrl) - sizeof(raddr), ignoring the datagram/AV segment that DC WQEs place after ctrl. The callback buffer is sized with UCT_RC_MLX5_RMA_MAX_IOV(0) while DC posts use UCT_RC_MLX5_RMA_MAX_IOV(UCT_IB_MLX5_AV_FULL_SIZE) (dc_mlx5_ep.c:596/658). So for DC the iov count can exceed callback_data->iov, and the ucs_assert(iovcnt <= ucs_static_array_size(...)) in uct_rc_mlx5_callback_data_fill_iov would be the only guard.
minor: the struct wraps iov/memh in a union { struct { ... }; } with a single member, which adds no value. Suggest dropping the union: typedef struct { Coverage gap (also noted in prior reviews): no dc_mlx5 instantiation and no non-RDMA_READ opcode coverage; confirm this matches the intended PR scope. Could not verify uct_ep_op_info_t::rma.rkey buffer is >= 8 bytes for the uct_ib_md_pack_rkey write; worth a spot check by the author if not already covered elsewhere. |
|
🤖 CI Triage Agent — TL;DR: The aarch64 build failed compiling Full analysisSummary: Root cause: Two compile errors in the merged tree:
Reading the source confirms the inconsistency: Implicated commit: File: Suggested fix: Rebase branch - void purge_outstanding(purge_ctx *ctx)
+ void purge_outstanding(purge_ctx *ctx, bool with_rx_token)Verify any other Related: PR #11866 (this PR); #12006 (
|
UCT/IB/MLX5: fix delivery check and comp UCT/IB/MLX5: fix op comp and format UCT/IB/MLX5: assert comp for get UCT/IB/MLX5: refactor UCT/IB/MLX5: fix build and assert UCT/IB/MLX5: wqe seg helper UCT/IB/MLX5: release get helper UCT/IB/MLX5: iov verify helper UCT/IB/MLX5: len assertion and unify arg UCT/IB/MLX5: unify seg helper UCT/IB/MLX5: fix assertions GTEST/UCT/IB: rma helper
a6f916e to
36de2d2
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The gtest shard hung for ~14.6 minutes inside Full analysisSummary: Root cause: This is a hang, not a slow test. Timestamp analysis of the log shows one dominant gap:
That gap equals the The last meaningful operation is the send in Two concrete defects in that new code are consistent with a permanent stall:
Implicated commit: File: Suggested fix:
Related: PR #11866 (this PR, "UCT/IB/MLX5: wqe parser for uct get"); sibling PRs on the same purge series: #11873, #12019, #12006, #11874, #11836. Note
|
|
🤖 Starting review — findings will be posted here when done. |
evgeny-leksikov
left a comment
There was a problem hiding this comment.
LGTM besides couple of minor comments
|
🤖 Starting review — findings will be posted here when done. |
What?
Add wqe parse support for uct get.
Depends on #11874Why?
FT need extract info from wqe before replay uct get.
How?
Add parser for uct get bcopy/zcopy.