Skip to content

UCP/DT: Register user-supplied SGL memory handles on all required MDs - #11888

Merged
michal-shalev merged 5 commits into
openucx:masterfrom
michal-shalev:sgl-user-memh-reg
Sep 5, 2026
Merged

michal-shalev merged 5 commits into
openucx:masterfrom
michal-shalev:sgl-user-memh-reg

Conversation

@michal-shalev

Copy link
Copy Markdown
Contributor

What?

Register user-supplied SGL memory handles on all the MDs the protocol requires.

Why?

ucp_datatype_iter_sgl_mem_reg() skipped registration for user-supplied handles, which are not necessarily registered on all the selected MDs, so the transport got UCT_MEM_HANDLE_NULL and crashed in uct_ib_memh_get_lkey().

How?

Register every SGL element regardless of handle ownership, like the contig and IOV paths already do, and use the checked ucp_datatype_iter_uct_memh() accessor in put_offload.c.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread src/ucp/dt/datatype_iter.c Resolved
Comment thread test/gtest/ucp/test_ucp_dt.cc Resolved
Comment thread test/gtest/ucp/test_ucp_dt.cc Resolved
tomerg-nvidia
tomerg-nvidia previously approved these changes Sep 3, 2026
iyastreb
iyastreb previously approved these changes Sep 3, 2026

@iyastreb iyastreb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm that it fixes the crash in nixlbench

@svc-ucx

svc-ucx commented Sep 3, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (AddressSanitizer roce on worker 2) · commit 53b73476

TL;DR: The ASan gtest job aborted on a fatal assertion in all/test_ucp_rma_sgl.put_multi_rail — the SGL PUT protocol looks up the local memory handle using the raw EP lane MD index (ucp_ep_md_index()) instead of the protocol's registered lpriv->super.md_index, so on a multi-rail lane whose MD is not part of reg_md_map (memh_index=8, md_map=0xf1) it dereferences a never-registered MD slot. Fix: use lpriv->super.md_index (which is UCP_NULL_RESOURCE → UCT_MEM_HANDLE_NULL for unregistered MDs) in the SGL send functions.

Full analysis

Summary: gtest aborted (core dumped, make: *** [Makefile:4713: test] Aborted) on all/test_ucp_rma_sgl.put_multi_rail/0 <all/host> with datatype_iter.inl:587 Assertion '(UCS_BIT(memh_index) & memh->md_map) || ucp_memh_is_zero_length(memh)' failed: memh_index=8 md_map=0xf1, reached via ucp_put_nbx.

Root cause: In src/ucp/rma/put_offload.c the SGL PUT protocols compute the local MD index as ucp_md_index_t md_index = ucp_ep_md_index(ep, lpriv->super.lane) (lines 405, 442, 555) and pass it to ucp_datatype_iter_uct_memh(). That is the raw EP/topology MD index, not the protocol's registered MD index. ucp_proto_common_reg_md_map() (src/ucp/proto/proto_common.c:738-771) deliberately excludes MDs that lack UCT_MD_FLAG_NEED_MEMH, are absent from context->reg_md_map[mem_type], or whose required_mem_flags are unmet; for such lanes ucp_proto_common_lane_priv_init() (proto_common.c:93-97) sets lane_priv->md_index = UCP_NULL_RESOURCE so that ucp_datatype_iter_uct_memh() returns UCT_MEM_HANDLE_NULL. Every other zcopy path uses lpriv->super.md_index (e.g. put_offload.c:264, proto_multi.inl:363). Because the SGL path bypasses this, the multi-rail all/host variant picks a lane on MD 8 which was never registered (memh->md_map=0xf1 = MDs {0,4,5,6,7}, whether the memh is the user-supplied one or one registered with reg_md_map), and the assertion fires. The new put_multi_rail test added in this PR is the first case that exercises a multi-lane SGL PUT where the lane set spans MDs outside reg_md_map, so the latent bug became a hard failure.

(The earlier ib/test_md_fork.fork/1 LeakSanitizer report — 56 bytes from ucs_async_alloc_handler via uct_ib_device_init — is a pre-existing, non-fatal artifact: that test still reported [ OK ]. It is not the cause of the build failure.)

Implicated commit: [REDACTED:Hex High Entropy String] — Michal Shalev, "UCP/DT: Register user-supplied SGL memory handles on all required MDs" (PR #11888); the faulty MD-index lookup dates back to the SGL PUT protocol work (efcc834 / d65ffe7, same author).

File: src/ucp/rma/put_offload.c:405, :442, :555 (assertion raised at src/ucp/dt/datatype_iter.inl:587)

Suggested fix: In ucp_proto_put_sgl_offload_send_frag(), ucp_proto_put_sgl_offload_send_func() and ucp_proto_put_sgl_offload_sw_send_func(), replace ucp_md_index_t md_index = ucp_ep_md_index(ep, lpriv->super.lane); with ucp_rsc_index_t md_index = lpriv->super.md_index; so that lanes whose MD is not in mpriv->reg_md_map correctly yield UCT_MEM_HANDLE_NULL. If the SGL offload really requires a memh on every lane, the alternative is to filter SGL lane selection to reg_md_map members (add a lane filter in ucp_proto_put_sgl_offload_probe) — but the lpriv->super.md_index change is the minimal, consistent fix. Also consider asserting in ucp_datatype_iter_sgl_mem_reg() that ucs_test_all_flags(memhs[i]->md_map, md_map) after registration, since ucp_memh_register_internal() (ucp_mm.c:591-606) can silently skip MDs whose required_mem_flags are unmet and still return UCS_OK.

Related: #11888 (this PR); prior SGL work: #11647, #11761, #11344

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 8ecb533e-f157-4516-a674-0c3dc3427eb0 in the triage console for the audit trail.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

tomerg-nvidia
tomerg-nvidia previously approved these changes Sep 3, 2026
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

@michal-shalev
michal-shalev enabled auto-merge (squash) September 3, 2026 11:53
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

iyastreb
iyastreb previously approved these changes Sep 3, 2026
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

@michal-shalev
michal-shalev enabled auto-merge (squash) September 3, 2026 13:01
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants