UCP/RNDV: Handle pre-init mtype abort - #11960
svc-ucx-bot wants to merge 1 commit into
Conversation
|
🤖 Starting review — findings will be posted here when done. |
| { | ||
| if (!(req->flags & UCP_REQUEST_FLAG_PROTO_INITIALIZED)) { | ||
| req->send.state.uct_comp.status = status; | ||
| ucp_proto_rndv_put_common_complete(req); |
There was a problem hiding this comment.
blocker: the uninitialized path ignores the ppln-fragment case. put/mtype is also probed with support_ppln=1, and for a ppln fragment req->send.rndv.rkey is owned by the parent request and completion must go through ucp_proto_rndv_ppln_send_frag_complete(). Here ucp_proto_rndv_rkey_destroy(req) destroys the parent's rkey (still used by other in-flight fragments passing it to uct_ep_put_zcopy(), and destroyed again in ucp_proto_rndv_ppln_frag_complete() — double free / use-after-free), and ucp_proto_rndv_request_zcopy_complete() completes the fragment as a standalone send request, so the parent's completed_size is never advanced (hang) and the fragment is never returned to the mpool (leak). Pls handle ucp_proto_rndv_request_is_ppln_frag(req) separately. This is exactly the low-memory scenario the PR targets, so the fragment path is as likely as the non-fragment one.
| context()->cache_md_map[mem_type]; | ||
| } | ||
|
|
||
| UCS_TEST_P(test_ucp_proto, rndv_put_mtype_allocation_failure_abort) |
There was a problem hiding this comment.
The test does not test the fix: EXPECT_TRUE(ucp_rndv_put_mtype_proto.abort != ucp_proto_rndv_stub_abort) is a tautology of the one-line .abort assignment. It does not exercise the allocation failure or abort behavior despite the test name, and it will keep passing if the abort implementation is wrong. Per REVIEW.md, a bug fix should carry a focused regression test. Can we make it exercise the real path, e.g. fail the rndv frag allocation and verify the request completes with an error instead of asserting?
| context()->cache_md_map[mem_type]; | ||
| } | ||
|
|
||
| UCS_TEST_P(test_ucp_proto, rndv_put_mtype_allocation_failure_abort) |
There was a problem hiding this comment.
minor: placement — the UCS_TEST_P is inserted between the test_ucp_proto member-function definitions (get_md_map() and do_mem_reg()); all other tests in this file come after the definitions. Pls move it down with the other UCS_TEST_P definitions.
|
Residual coverage gap: the abort path needs a GPU/mtype-capable job ( |
2 participants
What?
Add a dedicated rndv/put/mtype abort handler that directly completes requests when protocol initialization has not finished, while retaining the existing stub-abort path for initialized requests. Add a focused callback guard test.
Why?
A staging allocation failure could occur before
req->send.state.uct_compwas initialized. Dispatchingucp_proto_rndv_stub_abortthen invoked an uninitialized completion callback and caused a NULL-address SIGSEGV instead of propagating the allocation error.How?
The new handler records the allocation status and calls the common completion path for pre-initialized requests. Validation repeated the original 96-rank reproducer three times; every run retained the CUDA OOM, propagated
MPI_ERR_OTHER, and produced no SIGSEGV. The callback guard passed all 10 instantiated transport suites, the initialized-path peer-failure test passed 1/1, and the build, final gtest link, staged install, and diff check passed.