UCP/RMA: Add AM-based software emulation protocol for SGL put - #11943
michal-shalev wants to merge 8 commits into
Conversation
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Starting review — findings will be posted here when done. |
|
No blockers found; residual coverage gap worth noting: the new tests only run under forced |
|
🤖 Starting review — findings will be posted here when done. |
|
was the sporadic CUDA failure root-caused? with the default |
|
🤖 Starting review — findings will be posted here when done. |
|
minor: passing the fragment length as |
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage gap: the emulation protocol is only exercised with forced Not repeated here (already raised in previous rounds): the CUDA skip removal without a root cause, and passing the fragment length as |
|
🤖 Starting review — findings will be posted here when done. |
| UCP_SGL_EMULATION_PROTOS) { | ||
| m_require_zcopy_lane = false; | ||
| test_put_sgl(10, UCS_KBYTE, true, true); | ||
| } |
There was a problem hiding this comment.
minor: with the emulation proto the put normally completes in place, so the callback is never invoked and this test adds nothing over put_emulation. can we set UCP_OP_ATTR_FLAG_NO_IMM_CMPL here to actually cover the callback path?
With the emulation protocol every fragment is sent synchronously by uct_ep_am_bcopy(), so ucp_request_send() drains the whole SGL in one call and ucp_proto_request_send_op_common() returns via ucp_request_imm_cmpl_param() — a status, not a request. In that case UCX does not invoke the send callback, and the new !UCS_PTR_IS_PTR(sptr) branch in test_sgl() skips the cb.completed/cb.status checks entirely. For 10×1KB this is the normal outcome.
|
The CUDA Generic SGL data-verification tests still skip on tcp via the No test asserts that |
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| ucs_assertv(lpriv->max_frag > sizeof(ucp_put_hdr_t), "max_frag=%zu", | ||
| lpriv->max_frag); | ||
| max_payload = lpriv->max_frag - sizeof(ucp_put_hdr_t); |
There was a problem hiding this comment.
can we add a short comment on why ucp_proto_multi_max_payload() is not used here? for SGL dt_iter->length is the element count, so the weight scaling inside the helper would cap the payload by the number of elements — without a note someone is likely to "simplify" this back to the helper.
| ucp_proto_t ucp_put_sgl_offload_sw_proto = { | ||
| .name = "put/sgl/offload_sw", | ||
| .desc = "sgl " UCP_PROTO_RMA_EMULATION_DESC, | ||
| .desc = "sgl per-element zcopy", |
There was a problem hiding this comment.
minor: use the existing macro, like the other protos in this file:
| .desc = "sgl per-element zcopy", | |
| .desc = "sgl per-element " UCP_PROTO_ZCOPY_DESC, |
This branch has not been deployed
2 participants
What?
Add
put/sgl/am/bcopy, an active-message software emulation protocol for SGL put, re-enable the SGL CUDA gtests and add tests that force the new protocol.Why?
SGL was the only datatype without an emulation fallback, so
ucp_put_nbxfailed with "cannot find remote protocol" whenever lane selection left no RMA_BW lane (e.g. a cross-socket HCA derated belowMULTI_LANE_MAX_RATIO), while contig and IOV fall back toput/am/bcopyin the same configuration.How?
Reuse
ucp_proto_put_am_bcopy_probeand send one AM PUT per SGL element, since each element carries its own remote address,ucp_proto_sw_rma_cfg_threshkeeps it a last resort so hardware SGL protocols are unaffected.