Repository navigation
Conversation
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| num_frags = ucp_proto_get_offload_bcopy_num_frags(req, max_frag); | ||
| ctx_count = ucs_max(num_frags, (size_t)1); | ||
| req->send.buffer = ucs_malloc(ctx_count * |
There was a problem hiding this comment.
this adds malloc/free per GET request plus O(length/max_frag) memory (~200KB for a 100MB get on an 8KB frag lane), and get/bcopy is selected for small messages too, where docs/OptimizationStyle.md requires no malloc/free on the data path. can we keep the host path allocation-free (pass dest with the plain memcpy unpack cb) and use a pooled ctx only when the local memory is not CPU-accessible?
|
|
||
| static ucs_status_t ucp_proto_get_offload_bcopy_reset(ucp_request_t *req) | ||
| { | ||
| if (req->flags & UCP_REQUEST_FLAG_PROTO_INITIALIZED) { |
There was a problem hiding this comment.
can reset/abort run while fragments posted earlier are still in flight (pending replay from ucp_wireup_replay_pending_requests(), or a send error on a later fragment)? in that case UCT is left with a dangling unpack arg - IMO the array should be released only in the completion callback. (also applies to the free at :201)
| req->send.state.dt_iter.offset, max_frag, | ||
| req->send.state.dt_iter.length); | ||
|
|
||
| unpack_ctx = req->send.buffer; |
There was a problem hiding this comment.
why keep the unpack ctx array in req->send.buffer? it is the user send buffer field and get_am sets it for get_reply processing - pls use a dedicated field or document the aliasing.
| size_t frag_idx; | ||
| void *dest; | ||
|
|
||
| if (ucs_unlikely(max_frag == 0)) { |
There was a problem hiding this comment.
max_frag is always > 0 (asserted in ucp_proto_multi_init_priv()), pls drop this check and the one in the progress function (:119); this one also adds a branch on the send path.
| } | ||
|
|
||
| num_frags = ucp_proto_get_offload_bcopy_num_frags(req, max_frag); | ||
| ctx_count = ucs_max(num_frags, (size_t)1); |
There was a problem hiding this comment.
minor: num_frags is always >= 1 (zero-length get returns early in ucp_get_nbx()), and if it were 0 the frag_idx < num_frags assert in the send function would fail anyway - pls use num_frags directly instead of ucs_max(num_frags, 1).
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: A hang inside UCX during the first UCP tag ping-pong over RC/RoCE. Evidence from the log:
The hung operation is an eager tag send/recv (2048 B is below the RC rendezvous threshold), so the code this PR actually changes — the offloaded Implicated commit: unknown from the log; candidates are File: Suggested fix:
Related: PR #11416 (this branch, "UCP/RMA/BCOPY: Support non-host memory for get/bcopy"); #11843 (wireup token trailer + ACK, prime suspect); #11912 (wireup p2p lane matching diagnostics); #11840 (get/bcopy failover, same protocol as this PR).
|
What?
Add support for copy-out to non-host memory.
Why?
When reading to CUDA memory,
get/bcopycannot be used.How?
Add copy-out and use temporary memory descriptor area, to handle fragments potentially reordered.