Repository navigation
UCP/RMA: Sample multi-rail zcopy throughput - #12002
Conversation
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage gap: no end-to-end test enables Residual coverage gap: the restart/failover interaction described above is exercised by |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: Commit 136cb21 ("UCP/RMA: Sample multi-rail zcopy throughput") adds a new inline header Implicated commit: [REDACTED:Hex High Entropy String] — Leonid Genkin, "UCP/RMA: Sample multi-rail zcopy throughput" File: src/ucp/Makefile.am:72-74 (missing entry); consumer at src/ucp/rma/get_offload.c:13 and src/ucp/rma/put_offload.c:13 Suggested fix: Add the new header to the distributed-source list in Then re-run Related: PR #12002 (this PR); the include sites were introduced alongside fe0ea85 "UCP/RMA: Make put/rndv and get/rndv a fallback of zcopy (#11940)" refactor of the offload protocols.
|
|
🤖 Starting review — findings will be posted here when done. |
|
This PR adds ~680 lines, which is over the 500 added-line limit, and the measurement result currently only ends up in a Residual coverage gaps (not posted, already reported previously): no test verifies that a sample is actually admitted/validated on a real 2-rail zcopy path, and the reset/restart (failover, wireup replay) interaction with |
|
🤖 CI Triage Agent — TL;DR: Coverity flagged 4 new Full analysisSummary: The Root cause: Not a real bug — a broken Coverity suppression annotation. Upstream (see This PR refactored both RMA offload progress functions to hoist the downcast into a local:
The Implicated commit: File: Suggested fix: Move the single suppression to the taint source and drop the two now-dead ones in each file: static ucs_status_t ucp_proto_put_offload_zcopy_progress(uct_pending_req_t *self)
{
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;
if (!(req->flags & UCP_REQUEST_FLAG_PROTO_INITIALIZED)) {
ucp_rma_bw_sample_start(req, mpriv);
}
if (req->flags & UCP_REQUEST_FLAG_RMA_BW_SAMPLE) {
return ucp_proto_multi_zcopy_progress(req, mpriv, ...);
}
return ucp_proto_multi_zcopy_progress(req, mpriv, ...);
}Apply the identical change in Related: PR #12002 (branch |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Starting review — findings will be posted here when done. |
| ucs_offsetof(ucp_context_config_t, prefer_offload), UCS_CONFIG_TYPE_BOOL}, | ||
|
|
||
| {"RMA_BW_MEASURE", "n", | ||
| "Sample completed payload throughput of two-rail PUT/GET zcopy operations. " |
There was a problem hiding this comment.
maybe "multi-lane"?
There was a problem hiding this comment.
could you please point me where? I would suggest to unify everywhere if we are limited by 2 rails at this phase
| if (measure) { | ||
| comp = ucp_rma_bw_frag_start(req, req->send.multi_lane_idx, &frag); | ||
| } | ||
|
|
||
| status = uct_ep_get_zcopy(ucp_ep_get_lane(req->send.ep, lpriv->super.lane), | ||
| &iov, 1, req->send.rma.remote_addr + offset, | ||
| tl_rkey, comp); | ||
| if (measure) { | ||
| ucp_rma_bw_frag_posted(frag, iov.length, status); | ||
| } |
There was a problem hiding this comment.
why do we need 2 timestamps? should not doorbell of zcopy be constant?
I think we need it only if status == INPROGRESS
if status is err, then sampling probably does not make sense
There was a problem hiding this comment.
The return-time clock now runs only for sync UCS_OK. An async frag still needs the pre-post time and later callback time. Err contributes no bytes.
| for (i = 0; i < UCP_RMA_BW_MAX_ACTIVE; ++i) { | ||
| sample = &worker->rma_bw_samples[i]; | ||
| if (ucp_rma_bw_sample_is_free(sample)) { | ||
| break; | ||
| } | ||
| } | ||
| if (i == UCP_RMA_BW_MAX_ACTIVE) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
maybe organize lifo stack with limited depth?
There was a problem hiding this comment.
Completions can release slots out of order. The current scan is bounded to 4 slots and runs only for a new sample.
There was a problem hiding this comment.
I meant free list as a stack, see uct_rc_iface_send_op_t *free_ops for example if we need > 4 elements.
There was a problem hiding this comment.
IMO keep it simple as now, cause only 4 are needed.
| if ((frag == NULL) || ((status != UCS_OK) && | ||
| (status != UCS_INPROGRESS))) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
| if ((frag == NULL) || ((status != UCS_OK) && | |
| (status != UCS_INPROGRESS))) { | |
| return; | |
| } | |
| if ((frag == NULL) || UCS_STATUS_IS_ERR(status)) { | |
| return; | |
| } |
| ++sample->pending; | ||
| } else if (now > lane->last_comp) { | ||
| lane->last_comp = now; | ||
| } |
There was a problem hiding this comment.
ucs_get_time() is the most expensive part here, let's call it only if needed
| } | |
| if (lane->num_frags == 0) { | |
| lane->first_post = frag->post_time; | |
| } | |
| lane->bytes += bytes; | |
| ++lane->num_frags; | |
| if (ucs_likely(status == UCS_INPROGRESS)) { | |
| ++lane->num_async; | |
| ++sample->num_frags; | |
| ++sample->pending; | |
| return; | |
| } | |
| /* measure sync completion */ | |
| ucs_assert(status == UCS_OK); | |
| now = ucs_get_time(); | |
| if (now > lane->last_comp) { | |
| lane->last_comp = now; | |
| } |
| for (i = 0; i < UCP_RMA_BW_MAX_ACTIVE; ++i) { | ||
| sample = &worker->rma_bw_samples[i]; | ||
| if (ucp_rma_bw_sample_is_free(sample)) { | ||
| break; | ||
| } | ||
| } | ||
| if (i == UCP_RMA_BW_MAX_ACTIVE) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I meant free list as a stack, see uct_rc_iface_send_op_t *free_ops for example if we need > 4 elements.
| ucs_offsetof(ucp_context_config_t, prefer_offload), UCS_CONFIG_TYPE_BOOL}, | ||
|
|
||
| {"RMA_BW_MEASURE", "n", | ||
| "Sample completed payload throughput of two-rail PUT/GET zcopy operations. " |
There was a problem hiding this comment.
could you please point me where? I would suggest to unify everywhere if we are limited by 2 rails at this phase
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The Static_check job failed because clang's static analyzer flagged a dead store at Full analysisSummary: Root cause: The build itself succeeded (all This is new code: the loop at lines 73–81 was introduced by the head commit of this PR branch, which split the single-lane logic into a per-lane trace loop. The follow-on Implicated commit: File: Suggested fix: Remove the redundant temporary in the trace-only loop so there is no store for the analyzer to flag: if (i == sample->num_lanes) {
for (i = 0; i < sample->num_lanes; ++i) {
ucp_trace_req(req, "rma bw sample lane %u/%u: %zu bytes / "
"%.3f us", i, sample->num_lanes,
sample->lanes[i].bytes,
ucs_time_to_sec(sample->lanes[i].last_comp -
sample->lanes[i].first_post) * 1e6);
}
}
Related: PR #12002 (this build). Prior example of the same static-check gate blocking a merge: #11857
|
|
🤖 Starting review — findings will be posted here when done. |
evgeny-leksikov
added
Ready for Review
and removed
WIP-DNM
| ucp_rma_bw_sample_try_start(ucp_request_t *req, | ||
| ucp_lane_index_t num_lanes) | ||
| { | ||
| if (ucs_unlikely(req->send.ep->worker->context->config.ext.rma_bw_measure)) { |
There was a problem hiding this comment.
ucp_rma_bw_sample_start already has this condition
AFAIU currently ucp_rma_bw_sample_try_start is needed only for inlining, but ucp_rma_bw_sample_startrequires defactoringrequires refactoring. @gleon99 minor for now, pls update in separate PR as well as code style - alignments by /*and=` in few places
What?
Add opt-in per-rail completed-payload sampling for multi-rail PUT/GET zcopy.
Why?
Foundation for dynamic load-balancing, based on actual port performance measurement.
How?
Track accepted fragments through UCT completion.