Repository navigation
Conversation
|
🤖 Starting review — findings will be posted here when done. |
| #define ucp_trace_req(_sreq, _message, ...) \ | ||
| ucs_trace_req("req %p: " _message, (_sreq), ## __VA_ARGS__) | ||
|
|
||
| #define UCP_STATUS_FENCE_DEFER ((ucs_status_t)(UCS_ERR_LAST - 2)) |
There was a problem hiding this comment.
UCP_STATUS_FENCE_DEFER is outside the valid ucs_status_t range. UCS_ERR_LAST is -100, so UCP_STATUS_FENCE_DEFER = UCS_ERR_LAST - 2 = -102, which is more negative than UCS_ERR_LAST. Any accidental flow into UCS_STATUS_PTR/UCS_PTR_IS_ERR would misclassify it as a valid pointer (since UCS_PTR_IS_ERR tests >= UCS_ERR_LAST), and it is out of range for ucs_status_string. All current paths appear to intercept it explicitly, but this is fragile. Can we use a value inside the reserved error range, or a dedicated non-status sentinel that can't be confused with a status pointer?
|
PR exceeds the 500 added-line hard limit set in REVIEW.md. It mixes several separable concerns: the send.rma/send.amo → fenced_req struct refactor, the RMA-RNDV remote-op tracking (rma_rndv_ops), the flush lane-generation/lane_mask rework, and the new fence-pending queue engine. Please split — at minimum the mechanical fenced_req field rename and the flush lane-tracking refactor should be their own PRs with no functional change, leaving the fence engine as a focused feature PR.
Adding uint8_t flags to the packed ucp_cmpl_hdr_t changes the on-wire UCP_AM_ID_CMPL header from 8 to 9 bytes. ucp_rma_cmpl_handler reads putackh->flags with no length check, so a completion sent by an older peer (8-byte header) makes the new receiver read flags out of bounds. If that stray byte has UCP_CMPL_FLAG_RMA_RNDV set, it calls ucp_ep_rma_rndv_remote_request_completed, which asserts rma_rndv_ops > 0 and decrements it — an assert failure / underflow across mixed-version peers. Please confirm how this is version-negotiated, or carry the RNDV distinction without growing the wire header. Question: does this break wire compatibility with peers that don't send the new flags byte?
ucp_ep_fence_dispatch_request: when req->send.uct.func returns UCS_INPROGRESS after the batch loop, the request is pushed back to the head of the fence-pending queue and the loop breaks. A normal multi-fragment send that is legitimately in progress is now only re-driven when the fence queue is next resumed (flush completion / signal), not by the usual pending/progress path. Please confirm an in-progress (non-fence-blocked) request cannot get stuck waiting only on ucp_ep_fence_pending_resume.
ucp_ep_fence_pending_add: the epoch-ordered insert scans the whole queue on every add. Under many deferred fenced ops this is O(n²). Given the stated goal is to avoid blocking, worth a short note on expected queue depth, or use a structure that keeps ordering cheaply. Test coverage: the new synthetic tests (fresh_defer_without_uct_lane, pending_queue_orders_fence_epochs, lane-state units) are good, but they use stack requests and defer_once. There is no end-to-end test that exercises the real strong-fence-nb + fence_inflight_req + spin + resume path across multiple lanes, nor the mixed-version CMPL header path. Please add coverage for the multi-lane strong-fence drain, or identify the CI job that covers it. Rebase risk: the diff moves send.rma into send.fenced_req.rma (dropping the rma.sgl sub-struct from the new layout) but does not touch the req->send.rma.sgl.* uses in rma_send.c and put_offload.c. If those SGL uses exist on this PR's target base, this will not compile — please rebase and confirm the SGL PUT path still builds and works. |
641d2bd to
bf3246c
Compare
|
🤖 Starting review — findings will be posted here when done. |
| #define ucp_trace_req(_sreq, _message, ...) \ | ||
| ucs_trace_req("req %p: " _message, (_sreq), ## __VA_ARGS__) | ||
|
|
||
| #define UCP_STATUS_FENCE_DEFER ((ucs_status_t)(UCS_ERR_LAST - 2)) |
There was a problem hiding this comment.
UCP_STATUS_FENCE_DEFER value is fragile w.r.t. ucs_status_ptr_t classification. It is defined as UCS_ERR_LAST - 2 (= -102), which is outside the error range that UCS_PTR_IS_ERR recognizes: UCS_PTR_IS_ERR(-102) is false (because (uintptr_t)(-102) < (uintptr_t)UCS_ERR_LAST), while UCS_STATUS_IS_ERR(-102) is true and ucp_request_send_state_advance treats any UCS_STATUS_IS_ERR as fatal-fastforward. The current call paths keep this code inside uct_pending_callback_t returns that are explicitly filtered (ucp_request_try_send, ucp_proto_multi_*, ucp_ep_fence_dispatch_request), so it does not leak into a UCS_STATUS_PTR()/ucp_request_send_state_advance path today. But this is a landmine: any future progress path that forwards an unfiltered status into UCS_STATUS_PTR or ucp_request_send_state_ff will silently misbehave. Suggest defining it inside the reserved error range (e.g. between UCS_ERR_LAST and the endpoint-failure band, or a dedicated internal sentinel) and/or adding a compile-time assert that UCS_PTR_IS_ERR(UCP_STATUS_FENCE_DEFER) holds, so the invariant is enforced.
| { | ||
| ucs_assert(lane_index != UCP_NULL_LANE); | ||
|
|
||
| ++ep->ext->lane_generation; |
There was a problem hiding this comment.
ucp_ep_set_lane marks fence tracking dirty even for initial/NULL lane sets. It now unconditionally does ++lane_generation; fence_lanes_dirty = 1;, but it is called during normal wireup and teardown with uct_ep == NULL (e.g. ucp_ep.c allocation, cleanup, several wireup.c NULL sets). Setting an initially-empty lane or clearing a lane on teardown flips fence_lanes_dirty, which forces the next fence to conservatively flush all live lanes (ucp_ep_fence_lane_map_normalize). Correctness is preserved (normalize resets the flag), but this over-eagerly widens the fence flush set for benign topology events. Consider only marking dirty when replacing a non-NULL lane with a different non-NULL EP, which is the case the dirty flag is meant to guard.
|
Observation on Coverage gap: the multi-lane strong-fence-nb drain, the |
|
🤖 CI Triage Agent — TL;DR: The Coverity Full analysisSummary: Azure Pipelines job "coverity devel on coverity_rh7" failed with Root cause: Not a build/test/timeout failure — a static-analysis gate. In UCX the This PR ( const ucp_proto_multi_priv_t *mpriv = req->send.proto_config->priv; /* <-- downcast moved HERE */
...
if (!(req->flags & UCP_REQUEST_FLAG_PROTO_INITIALIZED) &&
!ucp_proto_rma_fence_progress(req, mpriv->lane_map, &status)) {
return status;
}
/* coverity[tainted_data_downcast] */ /* <-- annotation stranded */
return ucp_proto_multi_zcopy_progress(req, mpriv, ...);Coverity's event Implicated commit: File: Suggested fix: Move the existing suppression comment to immediately precede the declaration that now performs the downcast, in all four functions, and drop it from the (no longer downcasting) call site. E.g. in ucp_request_t *req = ucs_container_of(self, ucp_request_t, send.uct);
+/* coverity[tainted_data_downcast] */
const ucp_proto_multi_priv_t *mpriv = req->send.proto_config->priv;
ucs_status_t status;
if (!(req->flags & UCP_REQUEST_FLAG_PROTO_INITIALIZED) &&
!ucp_proto_rma_fence_progress(req, mpriv->lane_map, &status)) {
return status;
}
-/* coverity[tainted_data_downcast] */
return ucp_proto_multi_zcopy_progress(req, mpriv, ...);Apply the same edit at Related: PR #11871 (this PR). Prior refactors of the same functions that preserved the annotation on the call-site line:
|
|
🤖 Starting review — findings will be posted here when done. |
|
|
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
…e ops Post-fence requests are queued in per-EP pending queue and drain asynchronously using callback once the preceding epoch's flush completes. Add lane mask to flush only needed lanes.
Make the private fence-defer status compatible with UCS error pointers. Track lane generations only for real endpoint changes, initialize new and recycled lane storage, and mark fence lane state dirty only when replacing a lane with outstanding work. Preserve ownership of an already scheduled fence-pending callback when purging its request queue.
Decode both legacy 8-byte and current 9-byte RMA completion headers, treating a missing flags byte as zero and rejecting truncated endpoint IDs. Require a peer version that understands the flagged completion header and keep RMA/RNDV disabled for failover mode, where completion accounting cannot identify individual operations.
Keep a request-owned visibility reference while an RMA rendezvous operation is being prepared, retried, transferred to an internal request, or aborted. Move the operation into remote-completion accounting only after the wire send starts, and restore the request-owned reference if the send must be retried. This keeps strong fence and flush from completing while the operation is still active.
Add deterministic coverage for a strong fence that owns one in-flight request across two asynchronous lane flushes, including reference lifetime and final fence state. Place Coverity tainted-downcast annotations directly on the protocol-private-data reads they describe.
52b8e2b to
4727d13
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The only failing test is the newly-added Full analysisSummary: gtest Root cause: Log evidence:
The two sibling tests without Implicated commit: 8e881fc "UCP/RMA: Preserve rendezvous completion compatibility" — Nathan Bellalou (most recent commit touching File: test/gtest/ucp/test_ucp_proto.cc:439 (helper at 404-447; failing test at 796-804); related selection logic in src/ucp/rndv/proto_rndv.inl:168-182 and src/ucp/rndv/proto_rndv.c:474-499 Suggested fix: Make
Related: PR #11871 (this PR); prior protocol-selection triage PRs #11858, #11861, #11864; #11902 ("UCP/PROTO: Keep direct-access protocols eligible for CPU-accessible memory") |
Signed-off-by: Nathan Bellalou <nbellalou@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
What?
This PR is an updated replacement for PR #11199, not an incremental follow-up to the old branch. It carries the original endpoint-based, non-blocking fence design forward and adapts it to the current UCP protocol, flush-pipeline, and endpoint-failover behavior.
It makes the strong path of
UCX_FENCE_MODE=ep_basednon-blocking for one-sided RMA and atomic operations:ucp_worker_fence()as an epoch increment.Why?
The worker-wide strong fence is a synchronous remote flush across active endpoints and rails. It blocks the caller and over-serializes multi-rail traffic, since unrelated endpoints and faster rails must wait for the slowest outstanding work.
Endpoint-based enforcement keeps
ucp_worker_fence()lightweight and pays the ordering cost only on endpoints that have both pre-fence and post-fence work. The strong operation is further limited to the lanes that carried the preceding epoch whenever the endpoint topology remains stable.The original implementation in PR #11199 did not account for the current flush pipeline and endpoint failover model. This updated version ensures that:
The behavior remains opt-in with
UCX_FENCE_MODE=ep_basedand requires Proto v2.How?
The worker and each endpoint maintain fence sequence numbers. A post-fence request snapshots its required epoch before protocol-specific progress. The endpoint then takes one of three paths:
Each in-flight flush snapshots the endpoint lane generation and tracks its known, started, and selected lanes. If failover changes the topology, the completion accounting is reconciled and current live lanes are conservatively included so that a same-index replacement cannot be mistaken for the original transport endpoint.
Testing
make -j64ASAN_OPTIONS=protect_shadow_gap=0 ./test/gtest/gtest --gtest_filter='*fence*:*flush*'— 1222/1222 passedgit diff --check master...HEAD