Skip to content

UCP/PROTO: Fix buffer copy estimation for CPU-accessible memory - #11902

Open
yafshar wants to merge 9 commits into
openucx:masterfrom
intel-staging:fix/proto-cpu-accessible-memtype-eligibility
Open

yafshar wants to merge 9 commits into
openucx:masterfrom
intel-staging:fix/proto-cpu-accessible-memtype-eligibility

Conversation

@yafshar

@yafshar yafshar commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

What?

ucp_proto_init_add_buffer_copy_time() modeled a plain memcpy only when both buffers are UCS_MEMORY_TYPE_HOST. For the other memory types in UCS_MEMORY_TYPES_CPU_ACCESSIBLE — ze-host, ze-managed and rocm-managed — this went wrong in two different ways:

  • Direct-access protocols, which set memtype_op == UCT_EP_OP_LAST, fell through to the memtype endpoint lookup and returned UCS_ERR_UNSUPPORTED, so the candidate could be rejected. ucp_proto_buffer_copy_factor_id() also
    asserted that the operation is GET_ZCOPY or PUT_ZCOPY, which aborted:

    proto_init.c:292  Assertion `(memtype_op == UCT_EP_OP_GET_ZCOPY) || (memtype_op == UCT_EP_OP_PUT_ZCOPY)' failed: memtype_op=16
  • Eager bcopy protocols, which set UCT_EP_OP_GET_SHORT or UCT_EP_OP_PUT_SHORT, were costed with the copy interface bandwidth, so the candidate was mis-ranked.

Neither matches runtime behavior: ucp_dt_contig_pack()/unpack() use memcpy for CPU-accessible memory and reach the memtype endpoint only for non-CPU-accessible memory.

Add ucp_proto_buffer_copy_is_memcpy() to decide when the copy is performed by the CPU, and use it for the memcpy estimation. Keep the explicit UCT_EP_OP_LAST branch in ucp_proto_buffer_copy_factor_id() so a contract violation reports both memory types instead of producing the misleading zero-copy-operation assertion.

Why?

With ze_copy, initiating a 4 MB tag send from ze-host memory aborted while initializing protocol candidates. Even with the assertion silenced, the direct-access candidate was dropped, and the eager bcopy candidates were priced by the copy interface rather than by memcpy.

How?

The predicate treats the copy as a memcpy for host/host, and for UCT_EP_OP_LAST, UCT_EP_OP_GET_SHORT and UCT_EP_OP_PUT_SHORT when both memory types are CPU accessible. GET_ZCOPY/PUT_ZCOPY keep the copy interface
estimation, because the rendezvous mtype protocols perform a real memtype endpoint copy.

This changes protocol selection, not only the abort. In the tested configuration, tag send from ze-host previously selected eager short at size 0 and rendezvous from one byte onward. It now selects:

      0..115  eager short
    116..404  eager copy-in copy-out
   405..8247  eager zero-copy copy-out
 8248..25731  multi-frag eager zero-copy copy-out
  25732..inf  rendezvous zero-copy read from remote

The crossover values are hardware dependent; the shape of the change is that eager is no longer priced out of small and mid-size messages.

Tests, in separate ZE and ROCm fixtures so an unsupported environment reports a skip rather than passing without assertions:

  • cpu_accessible_direct_proto_eligible verifies egr/short is selected for each supported CPU-accessible memory type under RNDV_THRESH=inf. Reverting only the eligibility change reproduces the original assertion failure.
  • cpu_accessible_eager_costed_as_memcpy verifies an eager protocol is selected at one byte with rendezvous enabled. Reverting only the SHORT handling makes it fail with proto=tag/rndv.
  • device_memory_uses_mtype_copy verifies non-CPU-accessible memory still selects rendezvous, covering the CPU-accessible gate.

The ZE fixture covers ze-host and ze-managed; a separate fixture covers rocm-managed over rc,rocm_copy. This is protocol-selection coverage only; it does not exercise data movement.

Tested on Intel Arc Pro B60, rc_mlx5 over RoCE:

  • Protocol initialization completes. The thresholds above are observed for ze-host. ze-device is not CPU accessible and is unchanged.
  • ucx_perftest tag_lat -V passes for ze-host and ze-managed, which aborted before.

Remaining gaps: CPU-accessible GET_ZCOPY/PUT_ZCOPY costing is not covered, because protocol selection reaches that path only above a hardware-dependent threshold and neither RNDV_THRESH nor RNDV_SCHEME reliably forces it for
these memory types; covering it would need a lower-level test that constructs and inspects buffer-copy performance directly with a configured memtype endpoint. The same SHORT reclassification applies to rocm-managed, but its protocol-selection impact was not validated on ROCm hardware.

…emory

Protocols which access the payload directly do not set a memtype
operation, and ucp_proto_common_check_mem_access() explicitly permits
UCT_EP_OP_LAST when the memory is accessible from the CPU. The
performance estimation path did not follow that contract:

- ucp_proto_buffer_copy_factor_id() asserted that the operation is
  GET_ZCOPY or PUT_ZCOPY, so any CPU-accessible memory type other than
  host aborted with "memtype_op=16".

- ucp_proto_init_add_buffer_copy_time() modeled a plain memcpy only when
  both buffers are UCS_MEMORY_TYPE_HOST. Other CPU-accessible types fell
  through to the memtype endpoint lookup and returned
  UCS_ERR_UNSUPPORTED, which silently dropped the protocol.

Both affect the memory types in UCS_MEMORY_TYPES_CPU_ACCESSIBLE which
are not plain host memory: ze-host, ze-managed and rocm-managed.

Add ucp_proto_buffer_copy_is_memcpy() to decide when the copy is
performed by the CPU, and use it for the memcpy estimation. Keep the
explicit UCT_EP_OP_LAST branch in ucp_proto_buffer_copy_factor_id() so
that a non-CPU-accessible pair still reports the memory types rather
than the misleading zero-copy assertion.

With rc_x and ze_copy, initiating a 4 MB tag send from ze-host memory
aborted while initializing protocol candidates at proto_init.c:292.
After the fix, protocol initialization completes. For messages in the
0..2038-byte range, ze-host and ze-managed select "eager short" instead
of "eager copy-in copy-out", confirming that direct-access protocols
remain eligible. ze-device is not CPU accessible and is unchanged.

Add test_ucp_proto_ze.cpu_accessible_direct_proto_eligible, which asserts
that egr/short is selected for ze-host and ze-managed. Reverting only
proto_init.c makes it reproduce the original assertion failure.

Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread src/ucp/proto/proto_init.c
@yafshar
yafshar marked this pull request as ready for review September 5, 2026 11:38
@svc-ucx

svc-ucx commented Sep 5, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests gpu on worker 3) · commit 71fac44b

TL;DR: The GPU gtest job failed on a single test — shm_ib/test_ucp_proto_am_rndv.am_explicit_scheme_disables_force_provenance/0 — because the protocol chosen for a 1 MB CUDA AM send is no longer the hard‑expected am/rndv; the commit under test widened protocol eligibility for CPU‑accessible memory (ucp_proto_common_check_mem_access() / ucp_proto_buffer_copy_is_memcpy()), which changes protocol selection/costing, so either the eligibility rule must be narrowed to pairwise CPU‑accessible copies or the test's rigid protocol‑name expectation must be updated.

Full analysis

Summary: make test in the "gpu on worker 3" Azure job exited with error after 1 of 10257 gtests failed: shm_ib/test_ucp_proto_am_rndv.am_explicit_scheme_disables_force_provenance/0, GetParam() = shm,ib,cuda_copy,rocm_copy (all other tests passed, 20 skipped).

Root cause: The failing test (test/gtest/ucp/test_ucp_proto.cc:779) runs with RNDV_PIPELINE_SHM_CUDA_STAGING_FORCE=y + RNDV_SCHEME=get_zcopy and then calls check_am_rndv_remote_proto_config(0, 0). That helper goes through select_am_rndv_remote_proto_config(), which requires that the protocol selected for a 1 MB UCP_OP_ID_AM_SEND with UCS_MEMORY_TYPE_CUDA be literally "am/rndv" (EXPECT_STREQ("am/rndv", ...) at line 439, then ASSERT_NE(nullptr, remote_proto_config) at line 455). Nothing in the forced‑provenance path can trigger for AM (ucp_proto_rndv_shm_pipeline_force_scope() in src/ucp/rndv/proto_rndv.inl:30 bails out when TAG_RNDV op flag is absent and when rndv_mode != AUTO), so the only assertions that can fail here are the protocol‑selection ones.

The commit under test changes exactly that selection logic:

  • ucp_proto_common_check_mem_access() (src/ucp/proto/proto_init.c:517‑532) now keeps protocols with memtype_op == UCT_EP_OP_LAST (direct buffer access) eligible for any UCP_MEM_IS_ACCESSIBLE_FROM_CPU() memory instead of host‑only.
  • ucp_proto_buffer_copy_is_memcpy() (proto_init.c:283‑293) now costs such copies as a plain CPU memcpy at bcopy_bw, and ucp_proto_buffer_copy_factor_id() gained an ucs_assertv() (proto_init.c:313) that assumes both local and remote types are CPU accessible — while ucp_proto_init_add_buffer_copy_time() still returns UCS_ERR_UNSUPPORTED for UCT_EP_OP_LAST (line 388) when they are not.

Widening eligibility and making direct‑access protocols look cheaper changes the winner/thresholds of protocol selection, so with an explicit RNDV_SCHEME=get_zcopy on this shm+ib+cuda_copy worker a non‑am/rndv protocol is now returned at 1 MB and the test's hard‑coded name check fails. (Caveat: the Azure log only exposes the tail of the gtest output, so the exact EXPECT_* line is not quoted in the log; the failure is confined to this code path by the summary line plus the test source.)

Implicated commit: [REDACTED:Hex High Entropy String] — Yaser Afshar, "UCP/PROTO: Keep direct access protocols eligible for CPU-accessible memory" (touches both src/ucp/proto/proto_init.c and test/gtest/ucp/test_ucp_proto.cc)

File: src/ucp/proto/proto_init.c:517-532 (and proto_init.c:283-293, proto_init.c:313, proto_init.c:388); test expectation at test/gtest/ucp/test_ucp_proto.cc:439 used by test/gtest/ucp/test_ucp_proto.cc:779

Suggested fix:

  1. Narrow the new rule so it only applies when the copy is genuinely CPU‑doable end‑to‑end: gate ucp_proto_common_check_mem_access() on the pair of memory types (local select_param->mem_type and rkey_config_key->mem_type / reg_mem_info.type) being CPU accessible, keeping the memtype_op != UCT_EP_OP_LAST requirement otherwise. This removes the inconsistency between the new ucs_assertv at proto_init.c:313 and the UCT_EP_OP_LAST → UCS_ERR_UNSUPPORTED path at proto_init.c:388, and limits the selection perturbation to managed memory only.
  2. If the broader eligibility is intentional, make the test robust instead of pinning a name: in select_am_rndv_remote_proto_config() skip/accept when the selected protocol is not am/rndv (or use UCS_TEST_SKIP_R with the selected protocol name and ucp_ep_print_info() dump) so protocol‑selection changes on GPU workers don't produce a bare EXPECT_STREQ failure.
  3. Re‑run this job with UCX_LOG_LEVEL=debug/ucp_ep_print_info for the CUDA AM 1 MB selection to confirm which protocol replaced am/rndv before merging.

Related: PR #11902 (this PR); prior CI triage of the same tests: #11685, #11858, #11861, #11864

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 5dacb1c1-31a9-435a-addf-e57e2788ab64 in the triage console for the audit trail.

@yafshar yafshar closed this Sep 5, 2026
@yafshar yafshar reopened this Sep 5, 2026
@yafshar

yafshar commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

🤖 CI Triage Agent — UCX PR (Tests gpu on worker 3) · commit 71fac44b

TL;DR: The GPU gtest job failed on a single test — shm_ib/test_ucp_proto_am_rndv.am_explicit_scheme_disables_force_provenance/0 — because the protocol chosen for a 1 MB CUDA AM send is no longer the hard‑expected am/rndv; the commit under test widened protocol eligibility for CPU‑accessible memory (ucp_proto_common_check_mem_access() / ucp_proto_buffer_copy_is_memcpy()), which changes protocol selection/costing, so either the eligibility rule must be narrowed to pairwise CPU‑accessible copies or the test's rigid protocol‑name expectation must be updated.

Full analysis
Summary: make test in the "gpu on worker 3" Azure job exited with error after 1 of 10257 gtests failed: shm_ib/test_ucp_proto_am_rndv.am_explicit_scheme_disables_force_provenance/0, GetParam() = shm,ib,cuda_copy,rocm_copy (all other tests passed, 20 skipped).

Root cause: The failing test (test/gtest/ucp/test_ucp_proto.cc:779) runs with RNDV_PIPELINE_SHM_CUDA_STAGING_FORCE=y + RNDV_SCHEME=get_zcopy and then calls check_am_rndv_remote_proto_config(0, 0). That helper goes through select_am_rndv_remote_proto_config(), which requires that the protocol selected for a 1 MB UCP_OP_ID_AM_SEND with UCS_MEMORY_TYPE_CUDA be literally "am/rndv" (EXPECT_STREQ("am/rndv", ...) at line 439, then ASSERT_NE(nullptr, remote_proto_config) at line 455). Nothing in the forced‑provenance path can trigger for AM (ucp_proto_rndv_shm_pipeline_force_scope() in src/ucp/rndv/proto_rndv.inl:30 bails out when TAG_RNDV op flag is absent and when rndv_mode != AUTO), so the only assertions that can fail here are the protocol‑selection ones.

The commit under test changes exactly that selection logic:

  • ucp_proto_common_check_mem_access() (src/ucp/proto/proto_init.c:517‑532) now keeps protocols with memtype_op == UCT_EP_OP_LAST (direct buffer access) eligible for any UCP_MEM_IS_ACCESSIBLE_FROM_CPU() memory instead of host‑only.
  • ucp_proto_buffer_copy_is_memcpy() (proto_init.c:283‑293) now costs such copies as a plain CPU memcpy at bcopy_bw, and ucp_proto_buffer_copy_factor_id() gained an ucs_assertv() (proto_init.c:313) that assumes both local and remote types are CPU accessible — while ucp_proto_init_add_buffer_copy_time() still returns UCS_ERR_UNSUPPORTED for UCT_EP_OP_LAST (line 388) when they are not.

Widening eligibility and making direct‑access protocols look cheaper changes the winner/thresholds of protocol selection, so with an explicit RNDV_SCHEME=get_zcopy on this shm+ib+cuda_copy worker a non‑am/rndv protocol is now returned at 1 MB and the test's hard‑coded name check fails. (Caveat: the Azure log only exposes the tail of the gtest output, so the exact EXPECT_* line is not quoted in the log; the failure is confined to this code path by the summary line plus the test source.)

Implicated commit: [REDACTED:Hex High Entropy String] — Yaser Afshar, "UCP/PROTO: Keep direct access protocols eligible for CPU-accessible memory" (touches both src/ucp/proto/proto_init.c and test/gtest/ucp/test_ucp_proto.cc)

File: src/ucp/proto/proto_init.c:517-532 (and proto_init.c:283-293, proto_init.c:313, proto_init.c:388); test expectation at test/gtest/ucp/test_ucp_proto.cc:439 used by test/gtest/ucp/test_ucp_proto.cc:779

Suggested fix:

  1. Narrow the new rule so it only applies when the copy is genuinely CPU‑doable end‑to‑end: gate ucp_proto_common_check_mem_access() on the pair of memory types (local select_param->mem_type and rkey_config_key->mem_type / reg_mem_info.type) being CPU accessible, keeping the memtype_op != UCT_EP_OP_LAST requirement otherwise. This removes the inconsistency between the new ucs_assertv at proto_init.c:313 and the UCT_EP_OP_LAST → UCS_ERR_UNSUPPORTED path at proto_init.c:388, and limits the selection perturbation to managed memory only.
  2. If the broader eligibility is intentional, make the test robust instead of pinning a name: in select_am_rndv_remote_proto_config() skip/accept when the selected protocol is not am/rndv (or use UCS_TEST_SKIP_R with the selected protocol name and ucp_ep_print_info() dump) so protocol‑selection changes on GPU workers don't produce a bare EXPECT_STREQ failure.
  3. Re‑run this job with UCX_LOG_LEVEL=debug/ucp_ep_print_info for the CUDA AM 1 MB selection to confirm which protocol replaced am/rndv before merging.

Related: PR #11902 (this PR); prior CI triage of the same tests: #11685, #11858, #11861, #11864

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 5dacb1c1-31a9-435a-addf-e57e2788ab64 in the triage console for the audit trail.

This analysis is based on a change that isn't in this PR. ucp_proto_common_check_mem_access() is untouched here — its body is byte-identical at master and 71fac44b; the UCP_MEM_IS_ACCESSIBLE_FROM_CPU() predicate is pre-existing. Eligibility was not widened.

The failing test uses UCS_MEMORY_TYPE_CUDA (not CUDA_MANAGED), which is not CPU accessible, so the new is_memcpy() arm evaluates exactly as the old host-to-host check and UCT_EP_OP_LAST still returns UCS_ERR_UNSUPPORTED.

No costing or selection change for CUDA — this cannot alter the protocol chosen for a 1 MB CUDA AM send.

The failure is pre-existing: same test failed on #11685 (093ce688) and #11858 (cc5c6916). The #11858 triage found the real cause — with RNDV_SCHEME=get_zcopy, am/rndv may not be selected and the helper hard-asserts the name via an unguarded EXPECT_STREQ. That's suggested fix (2), and it belongs in its own PR.

Suggested fix (1) would regress this PR: pair-gating the eligibility check makes direct-access protocols ineligible for ZE_HOST / ZE_MANAGED, the bug being fixed here.

yao531441 added a commit to yao531441/llm-d that referenced this pull request Sep 16, 2026
Add modelserver/xpu/vllm/base/ mirroring the GPU OffloadingConnector
P2P-tier setup, adapted for Intel XPU:

- DRA ResourceClaimTemplate (gpu.intel.com), 1 XPU per pod, matching
  the pattern used by precise-prefix-cache-routing and
  tiered-prefix-cache's Intel XPU overlays, instead of the GPU
  overlay's nvidia.com/gpu device-plugin request.
- CI-sized functional check: 2 replicas (1 source + 1 receiver) of
  Qwen/Qwen3-0.6B instead of the GPU overlay's 16x gpt-oss-120b -
  validates the same P2P pull mechanism without requiring 24GB+ of
  device memory per pod or reproducing the GPU benchmark's scale.
  cpu_bytes_to_use and the shm tier are sized down to match.
- Same routing-sidecar (patch-sidecar.yaml) and P2P/kv-events port
  layout as the GPU overlay - both are accelerator-agnostic.

No RDMA overlay (xpu/vllm/rdma) is added. Direct RDMA over Intel XPU
device memory currently hits an upstream UCX ze_copy/DMA-BUF
data-correctness bug (silently wrong bytes, reported success) that
guides/modelexpress-p2p's Intel XPU PR (llm-d#2461) already ran into and
documented; tracked at openucx/ucx#11902 and #11903. The TCP-only
side channel here does not depend on that fix and is safe to ship now.

README changes:
- Supported Hardware Backends: document the Intel XPU variant, its
  scope (functional check, not a benchmark), and why RDMA is deferred.
- Step 3 (Deploy the Model Server): ACCELERATOR_TYPE now documents
  xpu, and TRANSPORT=base is called out as the only option for xpu.

Verified guides/p2p-kv-cache-sharing/modelserver/xpu/vllm/base renders
cleanly with 'kubectl kustomize' (ServiceAccount, Deployment with
sidecar + engine container merged correctly, port 8000->8200 renamed,
ResourceClaimTemplate name-referenced correctly). Not yet deployed to
a live cluster.

Signed-off-by: Yao, Qing <qing.yao@intel.com>
yao531441 added a commit to yao531441/llm-d that referenced this pull request Sep 16, 2026
Traced the actual vLLM OffloadingConnector/TieringOffloadingSpec P2P
data path: every transfer is staged through its CPU-mmap-backed
offload tier, and NIXL registers only that host memory (DRAM) with
UCX. XPU device memory is never handed to UCX/NIXL directly, unlike
guides/modelexpress-p2p's Intel XPU variant (llm-d#2461), which
does register device memory for its weight transfer and genuinely
needs UCX_TLS=tcp,ze_copy.

So for this overlay:
- UCX_TLS=tcp was already the right pin, but the comment overstated
  the risk (implying UCX could still reach device memory here).
- The claim that a missing RDMA overlay is 'blocked' by the upstream
  UCX ze_copy/DMA-BUF bug (openucx/ucx#11902, #11903) doesn't hold for
  this connector: an RDMA transport here would still only move
  CPU-to-CPU DRAM, not touch XPU VRAM directly. It just hasn't been
  built/validated yet, for unrelated reasons.
- 'TCP-only side channel' conflated the P2P tier's ZMQ control channel
  with the NIXL/UCX data plane; described them separately.

Reworded the patch-vllm.yaml comment, kustomization.yaml comment, and
README Supported Hardware Backends section accordingly, keeping a
pointer to the real UCX bug and llm-d#2461 for readers evaluating direct
XPU-device RDMA elsewhere.

Signed-off-by: Yao, Qing <qing.yao@intel.com>
maugustosilva pushed a commit to llm-d/llm-d that referenced this pull request Sep 16, 2026
* feat(p2p-kv-cache-sharing): add Intel XPU (TCP-only) variant

Add modelserver/xpu/vllm/base/ mirroring the GPU OffloadingConnector
P2P-tier setup, adapted for Intel XPU:

- DRA ResourceClaimTemplate (gpu.intel.com), 1 XPU per pod, matching
  the pattern used by precise-prefix-cache-routing and
  tiered-prefix-cache's Intel XPU overlays, instead of the GPU
  overlay's nvidia.com/gpu device-plugin request.
- CI-sized functional check: 2 replicas (1 source + 1 receiver) of
  Qwen/Qwen3-0.6B instead of the GPU overlay's 16x gpt-oss-120b -
  validates the same P2P pull mechanism without requiring 24GB+ of
  device memory per pod or reproducing the GPU benchmark's scale.
  cpu_bytes_to_use and the shm tier are sized down to match.
- Same routing-sidecar (patch-sidecar.yaml) and P2P/kv-events port
  layout as the GPU overlay - both are accelerator-agnostic.

No RDMA overlay (xpu/vllm/rdma) is added. Direct RDMA over Intel XPU
device memory currently hits an upstream UCX ze_copy/DMA-BUF
data-correctness bug (silently wrong bytes, reported success) that
guides/modelexpress-p2p's Intel XPU PR (#2461) already ran into and
documented; tracked at openucx/ucx#11902 and #11903. The TCP-only
side channel here does not depend on that fix and is safe to ship now.

README changes:
- Supported Hardware Backends: document the Intel XPU variant, its
  scope (functional check, not a benchmark), and why RDMA is deferred.
- Step 3 (Deploy the Model Server): ACCELERATOR_TYPE now documents
  xpu, and TRANSPORT=base is called out as the only option for xpu.

Verified guides/p2p-kv-cache-sharing/modelserver/xpu/vllm/base renders
cleanly with 'kubectl kustomize' (ServiceAccount, Deployment with
sidecar + engine container merged correctly, port 8000->8200 renamed,
ResourceClaimTemplate name-referenced correctly). Not yet deployed to
a live cluster.

* p2p-kv-cache-sharing: document XPU cluster verification results

Tested the xpu/vllm/base overlay on XPU-8xB60-817225 (real Intel Arc Pro
B60 hardware). DRA allocation, sidecar init, and vLLM boot with
OffloadingConnector + NIXL/UCX + P2P secondary tier all verified working.

However, the P2P pull itself is not yet verified: the pinned XPU image
(llm-d-xpu:v0.9.0, vLLM 0.26.0) has no remote_kv_source handling in its
OffloadingConnector (only max_offload_tokens), unlike the GPU overlay's
vllm-openai:v0.27.1. Documented this version-gap limitation in the guide
README.

* p2p-kv-cache-sharing: confirm XPU P2P pull works with nightly vLLM image

Manual pull test on real Intel Arc Pro B60 hardware (XPU-8xB60-817225)
confirms the P2P pull mechanism itself works correctly once the pinned
vLLM version carries vllm/v1/kv_offload/tiering/p2p/. Swapping the test
deployment's image from ghcr.io/llm-d/llm-d-xpu:v0.9.0 (vLLM 0.26.0, no
remote_kv_source support) to docker.io/vllm/vllm-openai-xpu:nightly
(vLLM 0.29.1rc1) made a 4096-token prefix pull succeed end-to-end
(external_prefix_cache_hits_total incremented by exactly 4096,
reproduced twice).

Updates the README's Intel XPU warning to reflect this: the gap is
purely the pinned image's vLLM version, not the manifests or the pull
mechanism, with a workaround (temporarily point the xpu-vllm component
at nightly) documented until ghcr.io/llm-d/llm-d-xpu is rebuilt against
a newer vLLM release.

* p2p-kv-cache-sharing: default XPU overlay to nightly xpu-vllm image

ghcr.io/llm-d/llm-d-xpu:v0.9.0 (vLLM 0.26.0) has no remote_kv_source
handling in its OffloadingConnector, so the P2P pull this guide exists
to demonstrate cannot work on it. Point the overlay at the existing
xpu-vllm/nightly component (vLLM main) instead, which was verified
end-to-end on real Intel Arc Pro B60 hardware, rather than shipping an
overlay that starts cleanly but can't do the one thing the guide is
about. Switch back to the llm-d component once ghcr.io/llm-d/llm-d-xpu
is rebuilt against a vLLM release carrying
vllm/v1/kv_offload/tiering/p2p/.

Also trims the README's Intel XPU warning now that this is the
default rather than a documented manual workaround.

Signed-off-by: Yao, Qing <qing.yao@intel.com>

* p2p-kv-cache-sharing: fix XPU review findings

- Add UCX_TLS=tcp to the XPU overlay's vLLM env, matching the
  pd-disaggregation XPU precedent, so this TCP-only overlay can't
  silently fall back to the buggy ze_copy/DMA-BUF transport.
- Stop citing guides/modelexpress-p2p's README as documenting the
  Intel XPU UCX bug: that content is still an unmerged draft
  (#2461), not on main. Reference the draft PR directly.
- Scope the '16 replicas, TP=1' summary to the GPU overlay; the XPU
  overlay is 2 replicas of a different model.
- Note that this guide's router values hard-code modelName:
  openai/gpt-oss-120b, and must be changed to Qwen/Qwen3-0.6B before
  installing the router when deploying the XPU overlay, or the render
  Service and every downstream verification command target a model
  the XPU pods never load.

All four issues were raised by automated review on the upstream PR;
verified against the actual repo state before fixing.

* p2p-kv-cache-sharing: correct UCX bug attribution for XPU overlay

Traced the actual vLLM OffloadingConnector/TieringOffloadingSpec P2P
data path: every transfer is staged through its CPU-mmap-backed
offload tier, and NIXL registers only that host memory (DRAM) with
UCX. XPU device memory is never handed to UCX/NIXL directly, unlike
guides/modelexpress-p2p's Intel XPU variant (#2461), which
does register device memory for its weight transfer and genuinely
needs UCX_TLS=tcp,ze_copy.

So for this overlay:
- UCX_TLS=tcp was already the right pin, but the comment overstated
  the risk (implying UCX could still reach device memory here).
- The claim that a missing RDMA overlay is 'blocked' by the upstream
  UCX ze_copy/DMA-BUF bug (openucx/ucx#11902, #11903) doesn't hold for
  this connector: an RDMA transport here would still only move
  CPU-to-CPU DRAM, not touch XPU VRAM directly. It just hasn't been
  built/validated yet, for unrelated reasons.
- 'TCP-only side channel' conflated the P2P tier's ZMQ control channel
  with the NIXL/UCX data plane; described them separately.

Reworded the patch-vllm.yaml comment, kustomization.yaml comment, and
README Supported Hardware Backends section accordingly, keeping a
pointer to the real UCX bug and #2461 for readers evaluating direct
XPU-device RDMA elsewhere.

* p2p-kv-cache-sharing: tighten XPU overlay wording

Trim hedging phrasing ('simply because', 'nothing about this path
blocks it', 'anyway') from the README, kustomization.yaml, and
patch-vllm.yaml so the Intel XPU transport description reads as a
final statement of fact rather than a running commentary.

---------

Signed-off-by: Yao, Qing <qing.yao@intel.com>
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread src/ucp/proto/proto_init.c Outdated
Comment thread src/ucp/proto/proto_init.c
Comment thread test/gtest/ucp/test_ucp_proto.cc Outdated
@svc-nvidia-pr-review

Copy link
Copy Markdown

Residual coverage gap: test_ucp_proto_ze needs real ZE hardware, and I did not find a gtest job with ZE (or ROCm) memory in buildlib/azure-pipelines*.yml, so this test will always skip in the PR pipeline; the protocol-selection change for rocm-managed/ze-* is not exercised by CI as it stands. The mock framework (test_ucp_proto_mock.cc) cannot fake memory types, so a HW-free equivalent is not readily available.

Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
Eager bcopy protocols set a SHORT memtype operation, but pack through
ucp_dt_contig_pack(), which uses memcpy for CPU-accessible memory and reaches
the memtype endpoint only for non-CPU-accessible memory. Estimating those
copies with the copy interface bandwidth priced eager out of small messages,
so rendezvous was selected from the first byte. Keep ZCOPY operations on the
copy interface estimation, since rendezvous mtype protocols perform a real
memtype endpoint copy.

Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

@yafshar yafshar changed the title UCP/PROTO: Keep direct-access protocols eligible for CPU-accessible memory UCP/PROTO: Fix buffer copy estimation for CPU-accessible memory Sep 23, 2026
Comment thread test/gtest/ucp/test_ucp_proto.cc Outdated
Comment thread src/ucp/proto/proto_init.c Outdated
@svc-ucx

svc-ucx commented Sep 23, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (AddressSanitizer new on worker 1) · commit 96cc0c06

TL;DR: The ASan gtest run failed on exactly one test — shm_ib/test_ucp_am_nbx_seg_size.multi/1 <shm,ib/proto_v1> — which sat for 201.7 s waiting for a multi-fragment eager AM that never completed reassembly, then failed on the receive-counter check; since that variant runs with PROTO_ENABLE=n it cannot be caused by PR #11902's proto-v2 change, so treat it as a hang in the AM long-message reassembly path (re-run the job, don't raise the timeout).

Full analysis

Summary: make test in the "AddressSanitizer new on worker 1" job exited 1 because 1 of 8589 gtests failed: shm_ib/test_ucp_am_nbx_seg_size.multi/1, which took 201772 ms (vs ~1.1 s for the equivalent rcx/...multi/1).

Root cause: A receive hang, not a slow test. test_ucp_am_nbx_seg_size.multi sends seg_size()*2 bytes (so the message is split into a first + middle eager fragment) from a freshly created entity whose IB/MM/SCOPY/TCP_SEG_SIZE differs from the receiver's, and then blocks in wait_receives() → wait_for_value(&m_recv_counter, m_send_counter) (test/gtest/ucp/test_ucp_am.cc:390-393, 509-543). The ~202 s duration is exactly the ASan-scaled wait_for_value deadline, after which EXPECT_EQ(m_recv_counter, m_send_counter) failed — i.e. the AM callback was never invoked because the receiver never considered the message fully assembled in ucp_am_handle_unfinished(): ucs_interval_tree_is_equal_range(frag_tree, {payload_offset, payload_offset + total_size - 1}) never became true ("not all fragments arrived yet" → return), so a fragment was lost/dropped or the fragment interval bookkeeping never coalesced to a single range. All neighbouring tests (including rcx/test_ucp_am_nbx_seg_size.single|multi/1 and the shm,ib AM DTS tests) passed, and no crash/ASan report is present — consistent with a dropped fragment / reassembly-completion bug rather than memory corruption.

Notably this is the proto_v1 variant (test_ucp_am_base sets PROTO_ENABLE=n for variant value 1, test/gtest/ucp/test_ucp_am.cc:41-54), while PR #11902 ("Fix buffer copy estimation for CPU-accessible memory") only affects proto-v2 protocol selection — so the branch under test is very unlikely to be the trigger.

Implicated commit: unknown (not PR #11902). Most recent changes touching the implicated reassembly machinery: 0385f633 "UCS/DATASTRUCT: Extract red-black tree core from interval tree (#11944)" — tomerg-nvidia, 2026-09-18, five days before this build, and the interval tree is what gates AM fragment completion; earlier suspects 2bafbb02 (Evgeny Leksikov, resend flag / partial-message eviction) and bcc7d08f (zero-length interval insert protection).

File: src/ucp/core/ucp_am.c:1440-1477 (ucp_am_handle_unfinished, completion check) and src/ucp/core/ucp_am.c:1413-1424 (ucp_am_copy_data_fragment); test: test/gtest/ucp/test_ucp_am.cc:1710.

Suggested fix: Do not change the time limit and do not modify PR #11902 — re-run the ASan job to confirm the test is flaky on the target branch as well (ideally also run the test standalone: GTEST_FILTER='shm_ib/test_ucp_am_nbx_seg_size.multi/1' UCX_LOG_LEVEL=debug ./gtest --gtest_repeat=50 with a fixed GTEST_RANDOM_SEED to catch the dropped fragment). For the product code, two concrete hardening items in the reassembly path: (1) in ucp_am_handle_unfinished, the loop over ep_ext->am.started_ams returns on the first incomplete message instead of continue-ing, so a completed message behind an incomplete one is never delivered (head-of-line stall) — change to continue; (2) ucp_am_copy_data_fragment inserts {offset, offset + length - 1} with no zero-length guard, which underflows the range for a 0-byte fragment (the protection added in #11179 is no longer present) — skip the insert when length == 0. Adding a debug print of the frag-tree node count/range when the assembly deadline expires would make this failure self-diagnosing next time.

Related: PR #11902 (branch under test), #11944 (interval tree → rbtree extraction), #11452 (AM resend flag / partial message eviction), #11179 (zero-length interval tree insert protection).

Replace ucp_proto_buffer_copy_is_memcpy() with a condition derived from the
factor selected by ucp_proto_buffer_copy_factor_id(), so the operation
classification lives in a single place rather than being duplicated. The
derived condition preserves host/host ZCOPY as memcpy while keeping non-host
ZCOPY on the memtype copy factor. Preserve the UCT_EP_OP_LAST assertion.

The CPU-accessible protocol selection checks only build a select_param and
never allocate a payload buffer, so drop the hardware support and copy
transport gates and run them over plain rc. This covers ze-host, ze-managed,
and rocm-managed without GPU hardware. Keep the ze-device negative path in
a separate fixture because it requires the copy transport.

Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread src/ucp/proto/proto_init.c
Comment thread src/ucp/proto/proto_init.c Outdated
Comment thread test/gtest/ucp/test_ucp_proto.cc Outdated
@svc-ucx

svc-ucx commented Sep 24, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests BlueField on worker 0) · commit c816c26d

TL;DR: The BlueField gtest job failed on a single unrelated, racy assertion in rcx/test_ucp_fault_tolerance.probe_gated_recovery/6 (probe_armed was false because the test samples a transient probe field that can be armed and completed inside one short_progress_loop()); the fix is to observe probes via a counting uct_ep_check mock (or a persistent counter) instead of polling arg->probe[lane].comp.func.

Full analysis

Summary: make test in the BlueField worker-0 job exited non-zero: 8709/8710 gtests passed, the sole failure being rcx/test_ucp_fault_tolerance.probe_gated_recovery/6 <rc_x/AM> at test/gtest/ucp/test_ucp_fault_tolerance.cc:941 — "RC p2p lane recovery completed without arming an aux probe".

Root cause: Test-side race, not a product bug and not related to this PR. The log shows the recovery flow worked (Attempting AM operation after failure injection on lane 0/2... Success at 13:48:16.099, lanes recovered by 13:48:17.288 — no hangs, no gaps). The assertion fails because of how the test observes the probe: probe_gated_recovery (lines 918–942) polls inside wait_for_cond, and in each iteration it (a) samples ep->ext->recovery_arg->probe[lane].comp.func != NULL and (b) returns ucp_ep_get_failed_lanes(ep) == 0. The progress driver is short_progress_loop(), which spins the worker many times per sample, so an aux probe can be issued and completed (with comp.func cleared) entirely between two samples; likewise if recovery already finished before the first sample the loop exits immediately with probe_armed == false. Note the sibling helper wait_for_recovery_probe_in_flight() (lines 853–874) has exactly the same weakness but uses comp.count. PR #11902 ("UCP/PROTO: Fix buffer copy estimation for CPU-accessible memory") touches protocol memtype eligibility and cannot influence RC p2p lane recovery probing, so this is pre-existing flakiness surfacing on the BlueField worker.

Implicated commit: db208ee — "UCP/FT: probe-gated lane recovery via aux uct_ep_check (#11563)", Evgeny Leksikov (added probe_gated_recovery and the transient-field polling); not commit c816c26 of this PR.

File: test/gtest/ucp/test_ucp_fault_tolerance.cc:941 (sampling loop at 918–939; same pattern at 853–874)

Suggested fix: Make probe observation edge-triggered instead of state-sampled:

  • Reuse the existing mock_recovery_probe() infrastructure with a counting hook (e.g. recovery_probe_count that increments a static counter and returns UCS_OK/delegates), then assert probe_count > 0 after recovery — this cannot miss a short-lived probe.
  • Or expose a cumulative probe counter (the assert-only ep->refcounts.probe at src/ucp/core/ucp_ep.h:631 is in-flight-only, so add a monotonic counter/stat in ucp_ep_recovery_arg_t) and check it after the wait.
  • As a stopgap, drive the wait with a single progress() step per sample rather than short_progress_loop() to shrink (though not eliminate) the sampling window.

Meanwhile, re-run the BlueField job for PR #11902; this failure should not block it.

Related: PR #11902 (the PR under test, unrelated to the failure); PR #11563 / commit db208ee introduced the flaky assertion.

Correct the memcpy estimation comment: a host-to-host copy returns the CPU
factor for any memtype_op, so only zero-copy involving non-host memory keeps
the copy interface estimation.

Derive the CPU-accessible memory type list in the protocol selection test from
UCS_MEMORY_TYPES_CPU_ACCESSIBLE instead of hardcoding it, so it does not go
stale when a new CPU-accessible memory type is added.

Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread src/ucp/proto/proto_init.c
Comment thread test/gtest/ucp/test_ucp_proto.cc
Comment thread test/gtest/ucp/test_ucp_proto.cc
Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread test/gtest/ucp/test_ucp_proto.cc
Comment thread src/ucp/proto/proto_init.c
Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread src/ucp/proto/proto_init.c
@svc-ucx

svc-ucx commented Sep 28, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests roce on worker 2) · commit 3e19741e

TL;DR: ucx_perftest hung for ~89 minutes on the very first case (ucp_contig_tag_lat/16384, UCX_TLS=rc_verbs,rc_x, mlx5_1:1) until the Azure agent was recycled; the PR's new memcpy short-circuit in ucp_proto_init_add_buffer_copy_time() bypasses the worker->mem_type_ep[]/UCT_EP_OP_LAST "unsupported" checks for host↔host copies, so memtype-copy (rndv mtype/ppln) protocols become eligible for host memory and stall at runtime. Restrict the short-circuit to blocking CPU copies (PUT_SHORT/GET_SHORT/UCT_EP_OP_LAST) instead of all host↔host cases.

Full analysis

Summary: "Tests roce on worker 2" (Azure build 137315) was killed by agent shutdown after ucx_perftest hung indefinitely in the first UCP tag-latency test on mlx5_1:1.

Root cause: This is a hang, not a slow test. The last application output is at 18:24:27.348 (the two UCX_TCP_PORT_RANGE unused-env warnings, immediately after the +ucp_contig_tag_lat/16384 banner); the next line is at 19:53:43 — an 89-minute silence covering essentially the whole runtime. At kill time the orphan-process cleanup still lists mpirun (915911) and both ucx_perftest ranks (915925, 915926), so the processes were alive and wedged; zero iterations were ever reported for the 16 KB tag-latency case (i.e. the rendezvous path with rc_verbs,rc_x).

The branch under test rewrites protocol-cost estimation. In src/ucp/proto/proto_init.c:341-350 a new early return was added:

  • ucp_proto_buffer_copy_factor_id() (line 283-291) already maps any host↔host copy to the CPU factor, including memtype_op == UCT_EP_OP_GET_ZCOPY/PUT_ZCOPY (the async rndv-mtype staging copies).
  • The new block therefore fires for those protocols too, returning an optimistic 1/bcopy_bw memcpy estimate and UCS_OK.
  • That skips the two guards below it that previously disabled exactly these protocols for host memory: the worker->mem_type_ep[local/remote] availability check (lines 352-361, added by [REDACTED:Hex High Entropy String] "Disable protocols that need memory type copy") and case UCT_EP_OP_LAST: return UCS_ERR_UNSUPPORTED (line 377-378).

Result: memtype-copy/pipeline rendezvous protocols are now advertised as very cheap for plain host buffers even though worker->mem_type_ep[UCS_MEMORY_TYPE_HOST] is NULL, get selected for the 16 KB rendezvous message, and then never complete — the observed hang.

Implicated commit: 96cc0c06 "UCP/PROTO: Cost CPU-accessible SHORT copies as memcpy" (Yaser Afshar), refined by c816c26d and 501ee9e7; branch head 3e19741e. Related enabling change: 71fac44b "Keep direct access protocols eligible for CPU-accessible memory".

File: src/ucp/proto/proto_init.c:337-350 (early memcpy return), with src/ucp/proto/proto_init.c:278-309 (ucp_proto_buffer_copy_factor_id) as the contributing classifier.

Suggested fix: Gate the memcpy short-circuit on the copy actually being a CPU copy, not merely on the memory being CPU-accessible — e.g. only take it when memtype_op is UCT_EP_OP_PUT_SHORT, UCT_EP_OP_GET_SHORT, or UCT_EP_OP_LAST, and let UCT_EP_OP_{PUT,GET}_ZCOPY fall through to the existing worker->mem_type_ep[] lookup so it still returns UCS_ERR_UNSUPPORTED when no memtype endpoint exists:

if ((memtype_op == UCT_EP_OP_PUT_SHORT) || (memtype_op == UCT_EP_OP_GET_SHORT) ||
    (memtype_op == UCT_EP_OP_LAST)) {
    if ((buffer_copy_factor_id == ucp_proto_buffer_copy_cpu_factor_id(local)) &&
        UCP_MEM_IS_ACCESSIBLE_FROM_CPU(local_mem_type) &&
        UCP_MEM_IS_ACCESSIBLE_FROM_CPU(remote_mem_type)) {
        /* memcpy estimation */
    }
}

Alternatively, keep the mem_type_ep == NULL rejection ahead of the short-circuit. To confirm before/after, re-run ucx_perftest -b test_types_short_ucp -b msg_pow2_short with UCX_NET_DEVICES=mlx5_1:1 UCX_TLS=rc_verbs,rc_x UCX_PROTO_INFO=y and diff the selected protocol for 16 KB tag send against master — the hung config should show an mtype/ppln rndv protocol that master does not select. Adding a timeout wrapper around the perftest loop in contrib/test_jenkins.sh would also turn this 90-minute agent kill into a fast, attributable failure.

Related: #11902 (the PR under test); prior art for the bypassed guard: commit [REDACTED:Hex High Entropy String] "UCP/PROTO: Disable protocols that need memory type copy".

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 4c1c6838-db9e-4cb8-870d-595d2c7e36de in the triage console for the audit trail.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants