Repository navigation
UCP/RMA: Apply ep_based weak fence once per worker fence - #12029
michael-beebe wants to merge 2 commits into
Conversation
With UCX_FENCE_MODE=ep_based, an operation that needs a fence on a single-lane endpoint applies a weak fence but does not advance the endpoint fence sequence. Every later operation on that endpoint therefore keeps requesting the fence, applies another weak fence, and is excluded from the put short fast path until a strong fence or new worker fence. Advance the endpoint sequence after a successful weak fence, or when there are no earlier operations to order. Record the weakly fenced lane; a weak fence orders only that lane, so a later operation on another lane still applies a strong fence, as before. The put short fast path falls back to the protocol path for that case. Add gtests that check the endpoint sequence after a fenced put, get, and atomic operation.
|
🤖 Starting review — findings will be posted here when done. |
| } else { | ||
| status = ucp_ep_fence_strong(ep); | ||
| } | ||
| } else if (ucs_unlikely(lane_map & ~ep->ext->fenced_lanes) && |
There was a problem hiding this comment.
pls test fenced_lanes != 0 first: in the common case it is 0, so lane_map & ~fenced_lanes is non-zero and the ucs_unlikely hint is on the wrong sub-expression.
| } else if (ucs_unlikely(lane_map & ~ep->ext->fenced_lanes) && | |
| } else if (ucs_unlikely(ep->ext->fenced_lanes != 0) && | |
| (lane_map & ~ep->ext->fenced_lanes)) { |
There was a problem hiding this comment.
Done in ca6fc76. The NULL check now comes first, inside the new ucp_ep_rma_is_strong_fence_required() helper, so the ucs_unlikely hint applies to the rare case. The field is now a single fenced_lane index, so the check is ucs_unlikely(fenced_lane != UCP_NULL_LANE) && (lane_map & ~UCS_BIT(fenced_lane)). Testing for NULL first also avoids computing UCS_BIT(UCP_NULL_LANE).
| if (ucs_unlikely(req->flags & UCP_REQUEST_FLAG_FENCE_REQUIRED)) { | ||
| if (ucs_unlikely(ep->ext->unflushed_lanes == 0)) { | ||
| status = UCS_OK; | ||
| /* No earlier operation to order */ |
There was a problem hiding this comment.
In the unflushed_lanes == 0 branch: fenced_lanes is always a subset of unflushed_lanes, so this store is redundant. maybe replace it with an assert to document the invariant that ucp_ep_fence_strong() and the new branch rely on?
| /* No earlier operation to order */ | |
| /* No earlier operation to order */ | |
| ucs_assert(ep->ext->fenced_lanes == 0); | |
| ep->ext->fence_seq = ep->worker->fence_seq; | |
| status = UCS_OK; |
There was a problem hiding this comment.
Done in ca6fc76. The store is replaced with ucs_assert(ep->ext->fenced_lane == UCP_NULL_LANE).
| } else if (ucs_likely( | ||
| ucs_is_pow2_or_zero(ep->ext->unflushed_lanes | lane_map))) { | ||
| status = ucp_ep_fence_weak(ep); | ||
| if (status == UCS_OK) { |
There was a problem hiding this comment.
In the weak fence branch: why set fence_seq to the current worker->fence_seq and not to the value that was pending when the request was created? if the user calls ucp_worker_fence() again after this request was submitted but before it is progressed, the EP is marked as satisfying the newer fence too, and a later operation on the same lane will not be fenced against this one. (same assumption already exists in ucp_ep_fence_strong(), so maybe worth a follow-up rather than this PR.)
There was a problem hiding this comment.
Agreed, but I would keep this out of this PR. The weak branch makes the same assumption that ucp_ep_fence_strong() already makes, so this change is consistent with the current code. Recording the sequence number pending at request creation would have to change both paths, plus the request state, so it seems better as a separate follow-up.
| tl_rkey = ucp_rkey_get_tl_rkey(rkey, rkey_config->put_short.rkey_index); | ||
|
|
||
| if (ucs_unlikely(ucp_ep_rma_is_fence_required(ep))) { | ||
| if (ucs_unlikely(ucp_ep_rma_is_fence_required(ep) || |
There was a problem hiding this comment.
minor: can we hide this in an inline helper in rma.inl next to ucp_ep_rma_is_fence_required()? the fence bookkeeping is otherwise fully contained there, and put_send_short now open-codes part of it.
There was a problem hiding this comment.
Done in ca6fc76. Added ucp_ep_rma_is_strong_fence_required(ep, lane_map) next to ucp_ep_rma_is_fence_required() in rma.inl. Both ucp_ep_rma_handle_fence() and ucp_put_send_short() use it now.
| sender().ep()->ext->fenced_lanes = 0; | ||
| } | ||
|
|
||
| void do_rma_op_with_fence_applied_once(op_type_t op, void *sbuf, |
There was a problem hiding this comment.
the second EXPECT_EQ(worker_fence_seq(), ep_fence_seq()) cannot fail — re-applying a fence does not change ep_fence_seq, so the test does not actually verify that the fence is applied only once. pls also cover the new path: weak fence on one lane followed by an operation on another lane (should trigger the strong fence), e.g. small op + fence + small op, then a large multi-lane op.
There was a problem hiding this comment.
Done in ca6fc76.
- The
*_oncetests now mockep_fenceon each lane's iface. They check that the fence count does not change on the next op after the fence, which is a real check. I confirmed they fail without thefence_sequpdate. - Added
test_ep_based_fence_other_lane_{put,get}. Each does op + fence + op, then a 1-byte put (which can take the short path) and a large multi-lane put, and expects a strong fence (fenced_lanereset) whenever an op lands on a lane other than the fenced one. - I left out an atomic variant. When AMOs are emulated in software, the strong fence blocks until the receiver makes progress, which a single-threaded test cannot provide.
- On a single-lane config (e.g. local
cma) the other-lane check is trivially satisfied. It takes effect withMAX_RMA_LANES=2on multi-lane transports such asrc.
|
Residual coverage gap: Residual coverage gap: When the planned |
|
🤖 CI Triage Agent — TL;DR: Full analysisSummary: Release-config gtest Root cause: The only failing assertion is the object-size guard on Implicated commit: File: Suggested fix: Preferred: avoid growing the hot Related: PR #12029 (this change); prior fence work in PR #10474 / commit
|
Track the weakly fenced lane in a one-byte field placed in existing padding, keeping ucp_ep_ext_t at its previous size. Move the cross-lane fence check into an inline helper used by both the request and put-short paths, assert the fenced-lane invariant when no lane is unflushed, and count transport fences in the test to verify a fence is applied once. Add tests for an operation on another lane after a weak fence.
|
🤖 Starting review — findings will be posted here when done. |
|
Pushed ca6fc76 to address the review:
|
|
🤖 CI Triage Agent — TL;DR: The ASAN gtest job aborted in Full analysisSummary: Root cause: Environmental, not a code regression. The log shows the failing WQE was Implicated commit: [REDACTED:Hex High Entropy String] "CI/FT: Adds fault tolerance CI test (#11570)" — william-gallagher-nv (introduced File: contrib/test_jenkins.sh:471-476 and 494, 546-566 (drop-rule teardown); symptom surfaces at test/gtest/uct/ib/test_ib.cc:728-742 Suggested fix:
Related: PR #12029 (build under test, unrelated to the failure); PR #11570 / commit [REDACTED:Hex High Entropy String] (added the FT test with the
|
|
/azp run UCX PR |
|
Commenter does not have sufficient privileges for PR 12029 in repo openucx/ucx |
What
Apply an
ep_basedweak fence once per worker fence instead of on everysubsequent RMA/AMO operation on the endpoint.
Why
With
UCX_FENCE_MODE=ep_based,ucp_ep_rma_handle_fence()applies a weakfence when only one lane has unflushed operations, but it does not advance
ep->ext->fence_seq. Every later operation on that endpoint therefore stillsees the fence as required, calls
uct_ep_fence()again, and is excludedfrom the
put_shortfast path. This lasts until a strong fence or a newworker fence.
This hurts fence-heavy workloads such as OpenSHMEM
shmem_fence()followedby many small puts. On an InfiniBand (
rc_mlx5) system, an OpenSHMEMalltoallsbenchmark built from per-element puts ran about 4.4x faster at4 KiB per PE with this change.
How
there are no earlier operations to order.
ucp_ep_ext_t::fenced_lanes. A weakfence orders only that lane, so a later operation on a different lane still
applies a strong fence, as before.
fenced_lanesis cleared by the strongfence (it is always a subset of
unflushed_lanes).ucp_put_send_short()falls back to the protocol path when another lanewas weakly fenced, so that path applies the strong fence.
test_ep_based_fence_applied_once_{put,get,atomic}gtests that checkthe endpoint fence sequence after a fenced operation. They fail without the
fix (the endpoint sequence stays behind the worker sequence).
Tested locally with gtest filters
*test_ucp_ep_based_fence*:*test_ucp_fence*:*test_ucp_rma_order*:*test_ucp_rma.*(116 tests) and
*amo*:*rma*:*flush*:*fence*:-*test_ucp_rma.*(764 tests).