Repository navigation
UCP/RMA: Fix PUT-SGL protocol selection length - #11778
usberkeley wants to merge 14 commits into
Conversation
|
@usberkeley thanks for the patch! Do you have CLA signed? |
Hi @brminich Just to clarify, I’ve added my name to the |
According to the Guidance for contributors:
you need to sign the UCX CLA and send it to admin@ucfconsortium.org. |
Thanks for the detailed guidance — Done, I’ve sent the CLA email |
Signed-off-by: Mason Pitt <bradley.b.pitt@gmail.com>
Use byte-based SGL iterators, inline length calculation, and verify protocol selection only after validating the request. Signed-off-by: Mason Pitt <bradley.b.pitt@gmail.com>
align and rename variable Co-authored-by: Michal Shalev <mshalev@nvidia.com>
Signed-off-by: Mason Pitt <bradley.b.pitt@gmail.com>
Signed-off-by: Mason Pitt <bradley.b.pitt@gmail.com>
|
Hi @michal-shalev This PR will first fix the data size issue. Would it make sense to re-enable the skipped CUDA test first, and fix the actual root cause in a separate PR? Root cause analysisThe test uses two UCP workers in the same process. cuda_ipc rejects same-process communication, so it should fall back to cuda_copy. However, each cuda_copy iface gets a different random ID (cuda_copy_iface.c:341), while connection setup requires the IDs to match (cuda_copy_iface.c:89). As a result, the two workers are incorrectly considered unreachable. Also, although the CI driver supports CUDA 13.0, UCX is built with CUDA 12.8. CUDA IPC only advertises native SGL support when built with CUDA >= 13.0. |
Thanks for digging into this. I can indeed see that the sporadic CUDA failure is still reproduced in this PR. Since the CUDA failure has a separate root cause, I think this PR should stay focused on fixing the SGL byte-length semantics, while keeping the CUDA test skipped for now. Then the Could you also update the PR description accordingly? |
Got it, no problem -- Done ❤️ |
Signed-off-by: Mason Pitt <bradley.b.pitt@gmail.com>
|
@usberkeley can you please merge master? |
Signed-off-by: Mason Pitt <bradley.b.pitt@gmail.com>
Got it --Done |
Signed-off-by: Mason Pitt <bradley.b.pitt@gmail.com>
"This branch is out-of-date with the base branch" |
Got it -- I’ll keep that in mind next time |
michal-shalev
left a comment
There was a problem hiding this comment.
LGTM. Thanks for contributing to UCX!
@brminich, can we merge this, or do you have any other comments on your end?
|
🤖 Starting review — findings will be posted here when done. |
| @@ -101,8 +101,6 @@ static ucs_status_t ucp_proto_reconfig_progress(uct_pending_req_t *self) | |||
| req->send.proto_config->rkey_cfg_index, | |||
| &req->send.proto_config->select_param, | |||
| ucp_operation_names, &strb); | |||
There was a problem hiding this comment.
this log removal looks unrelated to the SGL length change — why drop it? this branch aborts the request and fails the EP, so the error message is useful for diagnosing the no-protocol case. if it's intentional, pls move it to a separate PR.
|
🤖 Starting review — findings will be posted here when done. |
| @@ -101,8 +101,6 @@ static ucs_status_t ucp_proto_reconfig_progress(uct_pending_req_t *self) | |||
| req->send.proto_config->rkey_cfg_index, | |||
| &req->send.proto_config->select_param, | |||
| ucp_operation_names, &strb); | |||
There was a problem hiding this comment.
why is the cannot find remote protocol for error removed here? it looks unrelated to the SGL length fix, and it drops the diagnostic that reported strb. can this go in a separate PR (or be kept)?
|
🤖 CI Triage Agent — TL;DR: Full analysisSummary: gtest crash (core dumped, Root cause: The backtrace shows the abort path is protocol re-selection:
Implicated commit: File: Suggested fix: Don't use the proto-v1 msg_length = (req->send.state.dt_iter.dt_class == UCP_DATATYPE_SGL) ?
ucp_datatype_iter_sgl_total_length(&req->send.state.dt_iter) :
req->send.state.dt_iter.length;(or set Related: PR #11778 (
|
# Conflicts: # src/ucp/dt/datatype_iter.c # src/ucp/dt/datatype_iter.h # src/ucp/dt/datatype_iter.inl # src/ucp/rma/put_offload.c # test/gtest/common/test_obj_size.cc # test/gtest/ucp/test_ucp_dt.cc
|
🤖 Starting review — findings will be posted here when done. |
|
This removal looks unrelated to the SGL length change, and it leaves |
|
Hi @michal-shalev I should have it ready in the next couple of days. I’ll clean things up and double-check everything before submitting. |
|
🤖 CI Triage Agent — TL;DR: The ASAN/RoCE gtest shard didn't time out because it was slow — it hung: the multi-fragment AM eager-zcopy send over Full analysisSummary: Root cause: Evidence from the job log:
So a single UCP request in the AM eager zcopy multi-fragment path over Implicated commit: Not provable from the log alone. Candidates, in order of suspicion:
File: Suggested fix:
Related: PR #11778 "UCP/RMA: Fix PUT-SGL protocol selection length"; earlier SGL work #11765, #11677, #11647; possibly relevant recent DC/mlx5 PRs #11829, #11821, #11847.
|
What?
Use the total SGL byte length for PUT-SGL protocol selection and reconfiguration. Keep the CUDA SGL tests skipped for now.
Why?
The SGL iterator length is an element count, but protocol selection treated it as bytes. For example, four 2 KiB entries were selected as a 4-byte message instead of 8 KiB. This could lead to incorrect protocol ranges and reconfiguration decisions.
The sporadic CUDA failure reproduced in this PR has a separate root cause in
cuda_copyreachability and isoutside the scope of this fix.
How?
Calculate the total byte length once, with overflow checking, and store it in the existing request length field. Use that value for initial selection, reconfiguration, and error reporting. Add focused regression tests for the byte length and overflow cases.
Leave the CUDA SGL tests skipped. They will be re-enabled in a separate change that fixes the
cuda_copyreachability issue.