UCP/RMA: Make put/rndv and get/rndv a fallback of zcopy - #11940
Conversation
Protocols which implement the same operation by a different data path are currently ranked only by their performance model, so preferring one over another requires artificial cost penalties. Add a protocol class to the protocol definition, and a bitmap of classes which supersede it. A superseded protocol is not selected on message sizes where a protocol of a superseding class is available. No protocol declares a class yet, so protocol selection is unchanged.
The RMA rendezvous protocols were probed only on Vera Rubin, or when UCX_RMA_PPLN_ENABLE was set, because on message sizes where a direct zcopy protocol is available they must not be selected. Declare the direct RMA zcopy protocols as UCP_PROTO_CLASS_RMA_ZCOPY, and mark put/rndv and get/rndv as superseded by that class. Protocol selection now drops them on the message sizes covered by a zcopy protocol, so the platform check is no longer needed and the protocols become available wherever no zcopy protocol can carry the operation. The fallback overhead penalties are kept for now: they only affect message sizes where no zcopy protocol is available, which the supersede rule does not cover.
The configuration was only used to force-enable the RMA rendezvous put/get protocols on non Vera Rubin platforms. Protocol selection now supersedes them by the direct zcopy protocols, so they are always probed and the configuration has no remaining user. UCX_PROTOS can still be used to remove either the rendezvous or the zcopy protocols from selection.
Add a mock test which supersedes the eager short protocol by the eager copy-in protocol, and verifies that the short protocol is not selected anymore, while the message sizes not covered by the superseding protocol keep their selection. put/rndv and get/rndv are probed only when one of the buffers is not in host memory, so they cannot be used to test the rule on any device.
Add a mock device which reads from remote memory much slower than it writes to it, so that get/rndv, which makes the remote side write the data, is cheaper than get/zcopy on large messages. Check that get/zcopy is still selected on all message sizes, because it supersedes get/rndv. Move test_cuda_rma() to the common mock test class, to select an RMA protocol on CUDA memory from both tests which need it.
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage gaps worth flagging: the new |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Azure job Root cause: The test does
So arming → probe OK → rebuild → probe reset → Implicated commit: db208ee "UCP/FT: probe-gated lane recovery via aux uct_ep_check (#11563)", Evgeny Leksikov — added File: test/gtest/ucp/test_ucp_fault_tolerance.cc:922-943 ( Suggested fix:
Related: PR #11563 (probe-gated lane recovery, introduced this test), PR #11940 (the PR under test — no evidence it is the cause); UD keepalive/ep_check changes #11800, 183a66d, 8a38ae2 |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The ASAN "roce" job was aborted by the gtest 900-second watchdog because Full analysisSummary: Test Root cause: Timeline from the log proves a hang, not slowness:
The This is not explained by the PR diff. The branch's commits ( Implicated commit: unknown — not attributable to the PR commits ( File: test/gtest/ucp/test_ucp_sockaddr.cc:2042 (hung test), test/gtest/ucp/ucp_test.cc:300 (request never completed), test/gtest/common/test_helpers.cc:31,57 (900 s watchdog that aborted the run) Suggested fix: Do not raise the watchdog timeout — the process was hung, not slow. Concretely:
Related: PR #11940 (the PR under test); possibly relevant nearby work on selection/rkey stability: #11676 |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Starting review — findings will be posted here when done. |
|
A class cycle (A supersedes B, B supersedes A) is only documented, not enforced — the assert catches self-overlap only, and a cycle would disable every protocol in a range and fall into the "all disabled" path. Not covered by any test (noted as low priority while only one class exists). |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The Static_check job failed because clang's static analyzer reported Full analysisSummary: Root cause: Build itself succeeded; the failure is the clang-analyzer gate. In Implicated commit: 1361d3a ("UCP/PROTO: Apply supersede rule after fallback", Thomas Vegas); series also includes 1bcbbc1, c5eddb0, e6954e5 (same author) File: src/ucs/datastruct/bitmap.h:111-119 (warning site), triggered from src/ucp/proto/proto_select.c:195-201 (and :123-124) Suggested fix: Preferred, one-line and general: static UCS_F_ALWAYS_INLINE void
ucs_bitmap_bits_memset(ucs_bitmap_word_t *bits, int value, size_t num_words)
{
if (num_words == 0) {
return; /* bits may be NULL for an empty dynamic bitmap */
}
memset(bits, value, num_words * sizeof(ucs_bitmap_word_t));
}(keeps behaviour identical, removes the if (!ucs_dynamic_bitmap_is_zero(&disabled_proto_mask) &&
ucs_dynamic_bitmap_is_equal(proto_mask, &disabled_proto_mask)) {
...
ucs_dynamic_bitmap_reset_all(&disabled_proto_mask);
}or reserve the mask up front with Related: PR #11940 (branch |
The supersede test fixture mutates the global ucp_protocols[] table, and restored it only after the base class cleanup. The base cleanup tears down the endpoints and may throw, in which case the mutation would leak into the following tests in the same process and change their protocol selection.
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The Static check (clang static analyzer via Full analysisSummary: Root cause: The secondary error Implicated commit: File: src/ucs/datastruct/bitmap.h:111-119 (warning site: Suggested fix: Make the empty-bitmap case null-safe rather than relying on constant folding: static UCS_F_ALWAYS_INLINE void
ucs_bitmap_bits_memset(ucs_bitmap_word_t *bits, int value, size_t num_words)
{
if (num_words == 0) { /* also covers bits == NULL for empty dynamic bitmaps */
return;
}
memset(bits, value, num_words * sizeof(ucs_bitmap_word_t));
}This keeps the original compiler-warning workaround, is a no-op cost-wise, and removes the analyzer false positive for all dynamic-bitmap callers. Alternatively/additionally, in Related: PR #11940 (the PR under test; no pre-existing issue found for this analyzer warning)
|
Skip the memset() of an empty bitmap at runtime, so that a freshly initialized dynamic bitmap does not pass a NULL pointer to memset() on the path reported by the clang static analyzer. Restore the mocked protocol fields from a scoped object, so that the global 'ucp_protocols[]' entries are restored even when init() fails before cleanup() is reached.
|
🤖 Starting review — findings will be posted here when done. |
|
With the supersede rule, put/rndv and get/rndv can never be selected where put/get zcopy exists, even in the ranges where the cost model says rndv is faster (as the new |
yes, intended. |
|
🤖 CI Triage Agent — TL;DR: The gtest suite failed on a single new test, Full analysisSummary: Root cause: The failing test is a protocol-selection mock test introduced by this PR's series ("UCP/RMA: Supersede put/rndv and get/rndv by zcopy"). It asserts exact selection ranges for CUDA GET: Implicated commit: [REDACTED:Hex High Entropy String] (Thomas Vegas, PR #11940 head); the test itself comes from 40c844b / 4e6057e ("UCP/RMA: Supersede put/rndv and get/rndv by zcopy") in the same series. File: test/gtest/ucp/test_ucp_proto_mock.cc:1131-1142 ( Suggested fix:
Related: PR #11940 (#11940); prior threshold-retuning precedent: commit a79e5a0 "GTEST/UCP: Fix tests on GB300 (#11616)".
|
The expected ranges depended on the message size where the remote side switches from am/zcopy to put/zcopy, which is derived from the device attributes of the host.
|
🤖 Starting review — findings will be posted here when done. |
| } | ||
|
|
||
| proto = &ucs_array_elem(&proto_init->protocols, proto_idx); | ||
| superseded_by = ucp_proto_id_field(proto->proto_id, superseded_by) & |
There was a problem hiding this comment.
can you pls use fllback instead?
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage gap: the new Residual coverage gap: the PUT direction of the rule has no dedicated mock test; it is covered only implicitly by the existing
|
tvegas1
changed the title
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Starting review — findings will be posted here when done. |
Declare the relation on the preferred protocol, which lists the protocol classes it overrides in fallback_class, instead of on the superseded one.
Pair the existing selection check with the same mock configuration where get/zcopy is excluded, to show that get/rndv is a candidate removed by the fallback rule rather than one which was never probed.
|
🤖 Starting review — findings will be posted here when done. |
|
In Residual coverage gap: the first loop's Residual coverage gap: the PUT direction of the new rule still has no dedicated mock test (covered only implicitly by |
|
retested on setup, checked trace are not flooding. @brminich can we go ahead? |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Azure job "new on worker 2" (build 136702) failed at Root cause: Evidence from the log: the build's only failure is that single gtest (no timeout, no crash/SIGTERM, no hang — the run ended normally after 3,254 s with continuous output). The failing test ( Implicated commit: PR #11940 head File: Suggested fix:
Related: PR #11940 (this PR, "UCP/RMA: Make put/rndv and get/rndv a fallback of zcopy"); PR #11912 (p2p lane matching diagnostics); PR #11868 also references this test name.
|
| if (ucs_dynamic_bitmap_get(disabled_proto_mask, proto_idx)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
minor:
Should this compute the transitive fallback closure? For A → B → C, when B is disabled by configuration, this continue prevents collecting B → C, so C can still be selected against A. If fallback chains are intentionally unsupported, pls document.
(one example is zcopy->rndv->am_ sw_emulation)
4 participants
What?
Make
get/zcopyandput/offload/zcopyoverrideget/rndvandput/rndv, which become their fallback, and removeUCX_RMA_PPLN_ENABLE.Why?
UCS_CPU_MODEL_NVIDIA_VERAgate, which made selection platform dependent.cfg_prioritycannot express it: it is inert unlesscfg_threshis configured.How?
proto_class, and the classes it overrides infallback_class. A protocol is dropped on message size ranges where another protocol declares its class as a fallback. Two newucp_proto_tfields, both default 0, so other protocols are unaffected.ucp_proto_select_disable_fallback()runs per message size range inucp_proto_thresholds_next_range(), so the rendezvous protocols stay available wherever zcopy has no range.cfg_threshandcfg_priorityfilters, soUCX_ZCOPY_THRESHis still honored: a zcopy protocol disabled below the configured threshold does not override its fallback.RMA_RNDVclass without affecting each other.GET_ZCOPYdevice.