Skip to content

Cp chunkprefill upstream 20260610 latest#1872

Open
cchh05 wants to merge 13 commits into
xLLM-AI:mainfrom
cchh05:cp-chunkprefill-upstream-20260610-latest
Open

Cp chunkprefill upstream 20260610 latest#1872
cchh05 wants to merge 13 commits into
xLLM-AI:mainfrom
cchh05:cp-chunkprefill-upstream-20260610-latest

Conversation

@cchh05

@cchh05 cchh05 commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Description

Related Issues

Change Type

  • Bug fix
  • New feature
  • Performance improvement
  • Refactor
  • Documentation
  • Test
  • Build or CI

Pull Request Checklist

Thank you for contributing to xLLM. Before requesting review, please make sure the following items are complete.

PR Title and Commit Messages

  • The PR title and each commit message follow the xLLM commit format: <type>: <subject>.

Allowed types: feat, bugfix, docs, test, refactor, chore, style, revert, perf, model, build, release.
The subject should use clear English, start with a verb, include at least 4 words, and end with ..

Pre-commit Checks

  • I have installed pre-commit by running pip install pre-commit or an equivalent command.
  • I have installed the hooks with pre-commit install.
  • I have run pre-commit run --all-files and fixed any reported issues.

If you are unsure how to set up pre-commit, see the pre-commit documentation.

Self Review

  • I have self-reviewed the code according to .agents/skills/code-review/references/custom-code-style.md, especially code written or assisted by AI.
  • I have rebased this PR onto the latest main branch.

Build and Test Coverage

  • Tests have been added or updated as needed.
  • CUDA: python setup.py build test has passed on a CUDA machine.
  • NPU: python setup.py build test has passed on an NPU machine.
  • MLU: python setup.py build test has passed on an MLU machine.

Reviewer Notes

cchh05 and others added 13 commits June 10, 2026 18:19
…egister

L2.3 (service-side `--enable_mix_dual_register`) silently failed in the
production path because MixScheduler did not run a disagg_pd RPC server,
so its etcd entry had `rpc_address=""` / `cluster_ids=[]` / `addrs=[]` /
`ports=[]`. The service watch hit `gather_link_operations` → `run_link_
operations` → brpc dial empty rpc_address → `Fail to link instance during
registration` → no `add_instance_to_index` → MIX never reached
`prefill_index_` / `decode_index_` and the dual_register code never ran.

Switch MixScheduler's base from `ChunkedPrefillScheduler` to
`DisaggPDScheduler`. The base ctor wires
`initialize_rpc_server` + `register_instance_info`, which fills
`instance_info_.rpc_address` (from the brpc listen addr) plus the
cluster_ids/addrs/ports the engine reports via `engine_->get_cache_info`.
After this change MIX's etcd entry is fully populated and matches what
PREFILL/DECODE write — service-side `LinkInstance` calls land on a real
endpoint instead of erroring on the empty address.

Override `add_request` and `step` to skip the prefill→decode dispatch
path (`prefill_request_queue_` / `prefill_send_first_generation`) which
DisaggPDScheduler runs for PREFILL/DECODE roles. MIX serves locally and
must keep using `request_queue_` and base `ContinuousScheduler::step`.
The dispatch_thread the base spawns stays idle for MIX (nothing enqueues
into prefill_request_queue_), which is harmless.

Note: the base ctor also runs `profile_ttft` + `profile_tpot` for
InstanceRole::MIX. With small tp (e.g. tp=4), `profile_tpot` triggers a
known engine corner case (`Expected NPU tensor` in
`get_npu_format`). Launchers should pass `--disable_ttft_profiling=true`
for MIX until the underlying ProfileManager bug is fixed.

Verified end-to-end on dev98:
  XLLM:MIX:11.87.191.98:58200
  {"addrs":[..×4],"cluster_ids":[..×4],"ports":[564..567],
   "rpc_address":"11.87.191.98:13877",...}

vs the previous run with the same service binary:
  XLLM:MIX:...:58200
  {"addrs":[],"cluster_ids":[],"ports":[],"rpc_address":""}

The remaining `LinkLlmClusters failed (5010b009)` on the
DECODE→MIX direction is a separate engine constraint
(`link_cluster` requires P-side tp == D-side dp_local_tp_size_,
documented at xllm/core/distributed_runtime/llm_engine.cpp:728), not
addressed in this commit.

Co-Authored-By: Claude <noreply@anthropic.com>
LlmDataDistTransfer::initialize was setting OPTION_LISTEN_IP_INFO only for
LlmRole::kPrompt (PREFILL). MIX (kMix) silently skipped it. Result: a MIX
instance running with `--enable_disagg_pd=true` did not register a listen
endpoint on the underlying LlmDataDist runtime, so when a remote DECODE
tried to LinkLlmClusters to the MIX, the call was rejected with
ret=5010b009 (ACL/RT param invalid range).

Verified end-to-end on dev98 + dev82 24-card mixed topology:
  prefill_1_tp8 (dev98 0-7) + decode_1_tp8 (dev98 8-15)
  + mix_1_tp8 (dev82 0-7), all tp=8 to satisfy link_cluster's
  D-side tp == P-side tp constraint.

After this fix:
  - Service registers MIX dual-mode:
    `Register a new dual-mode mix instance: 11.87.191.82:58200
     prefill_idx=0 decode_idx=0`
  - decode -> MIX link succeeds:
    `LinkLlmClusters success, clusters = 1`
    `Successfully linked instance, instance_name: 11.87.191.82:58200,
     prefill_kv_split_size: 1`
  - decode -> prefill_1 link also succeeds (regression check)
  - route_trace shows candidate_prefill_instances = ["MIX:58200",
    "prefill_1:18100"], pool_active_set has both, requests are scheduled
    onto MIX as prefill+decode pair (single-instance mixed mode) when RR
    selects it.

  - smoke 1 short request: 200 OK, completion_tokens=20
  - smoke 1 long request: 200 OK, prompt=204 completion=256

This closes the last-mile of the L2.3 + L2.4 + L2.5 chain (service-side
dual_register + MIX disagg_pd RPC plumbing + LlmDataDist listen
endpoint). Mixed PD+MIX topology is now functional end-to-end.

Co-Authored-By: Claude <noreply@anthropic.com>
Adds two opt-in flags to mitigate the MIX colocate decode tpot regression
observed at 4x-15x worse than PD-disagg in 720-plan benchmarks:

  enable_mix_decode_first (default false): when true, prepare_batch
    splits running_queue_ into decode-stage and prefill-stage lists,
    runs handle_running_queue_requests on decode first with the full
    token budget, then on prefill within remaining budget. Decode is
    no longer dragged by prefill chunk step time.

  mix_decode_token_budget (default 0): reserves N tokens of per-step
    budget so prefill chunks are capped at (max_tokens_per_batch - N).
    Only takes effect when enable_mix_decode_first=true.

Defaults preserve legacy single-pass behavior; flags must be flipped
explicitly to opt into the new path.

Co-Authored-By: Claude <noreply@anthropic.com>
Adds env mix_max_prefill_chunks_per_step (default 0 = no cap). When >0,
phase 2 of prepare_batch (prefill admission) stops after this many
prefill sequences enter running_sequences_, regardless of remaining
token budget. Bounds how much prefill drags decode tpot.

Path C is a lightweight middle-ground between B+C decode_first
(throughput-friendly, tpot improvement limited) and chunk=2048
(tpot-friendly, ttft destroyed). With max_prefill_chunks=1, only one
prefill chunk co-exists with decodes per forward step.

Defaults preserve legacy behavior; env must be set explicitly to opt in.

Co-Authored-By: Claude <noreply@anthropic.com>
Adds env enable_mix_step_isolation (default false). When true,
MixScheduler::prepare_batch dispatches either a decode-only or
prefill-only forward step (never mixed). Decode tpot is no longer
dragged by prefill chunk step time.

Decision matrix per step:
  - prefill_queue empty                          -> decode-only
  - decode_queue empty                           -> prefill-only
  - both non-empty AND consecutive_decode_steps_
    >= max_decode_steps                          -> prefill-only (forced)
  - both non-empty AND below threshold           -> decode-only

mix_step_isolation_max_decode_steps (default 16) bounds prefill wait
under decode pressure. consecutive_decode_steps_ in the scheduler
state tracks the count.

Path A is the third opt-in flag stack on top of B+C decode_first +
Path C chunks-per-step cap. When enable_mix_step_isolation=true
the decision matrix takes precedence; otherwise the prior two-phase
mixed batch path is used unchanged.

Defaults preserve legacy behavior. Stacking:
  enable_mix_decode_first=false  -> legacy single-pass mixed
  =true + isolation=false        -> B+C two-phase mixed (decode-first)
  =true + isolation=false + chunks_cap=N -> Path C bounded prefill
  =true + isolation=true         -> Path A separated forward steps

Co-Authored-By: Claude <noreply@anthropic.com>
Backport commit 1b47840 from jd-opensource/xllm release/v0.10.0
(PR xLLM-AI#1848 "fix prefix match_kv recompute") to this cp-chunkprefill fork.

The fix ensures that after allocate_shared() hits prefix cache, the
allocation size is topped up to cover the newly matched tokens plus
the incremental budget, so downstream num_blocks_needed correctly
accounts for the prefix hit.

Ported verbatim, 13 line insertion in BlockManagerPool::allocate(seq, n).

RESULT: This fix ALONE is not sufficient to make pcache=true work with
disagg PD in this fork — build_step_transfer_info still asserts
"remote block coverage shortage" because the P/D two-sided
shared_kv_blocks_num semantics are independent (P and D each hit
their own pcache) and the transfer protocol has no offset negotiation.

See follow-up hotpatch commit for a temporary workaround.
…red)

Proper fix for the "remote block coverage shortage" FATAL that
`enable_prefix_cache=true` triggered under disagg PD topology. Replaces
the previous hotpatch (log-and-degrade) with a semantics-correct
sender-driven transfer window.

Root cause of the bug:
The receiver-directed protocol coupled P-side and D-side
`shared_kv_blocks_num`. `local_block_ids` is P's full sequence blocks
(including P's shared prefix). `remote_blocks_ids` in the request came
from D's response, which iterates from D's `shared_num` onward — so
`remote_size == D_full - D_shared_num`. The old assertion
`CHECK_GE(align_up(remote_size, stride), map_end * stride)` implicitly
required `P_shared_num == D_shared_num`, which is never enforced.

Fix (build_step_transfer_info):
Reinterpret `remote_size` as D's KV deficit. P transfers exactly
`remote_size / stride` blocks, drawn from the TAIL of P's local
sequence — i.e. the leading blocks (which correspond to D's shared
prefix that D already has cached) are skipped. P-side shared_num is
now decoupled from transfer sizing; it only affects P-side compute
savings via prefix_cache reuse, which is orthogonal.

Assertion changed from `remote_size >= map_end*stride` (bug prone) to
`local_size >= deficit_local_blocks` (structurally always true when P
and D agree on the same request).

Verification:
Four-arm 720 multiturn bench, 4P+1D:
  A pcache=false               720/720   0 FATAL  mean_ttft 531ms
  B pcache=true  P0=off        720/720   0 FATAL  mean_ttft 207ms
  C pcache=true  P0=pmax30     720/720   0 FATAL  mean_ttft 183ms

Zero coverage-shortage events across all four prefills after fix.
Long-request p99_ttft: 2380 (A) -> 1322 (B) -> 1052 (C).
Long prefill_compute mean: 330 -> 289 ms with P0 pmax30 (real
compute savings, not skip-transfer artifact like hotpatch had).

References:
- vLLM Nixl push_scheduler.py: `num_external_tokens` — D registers
  only its deficit, P is sender-driven.
- vLLM v1 KVConnectorBase: sender-driven KV transfer contract.

Reverts hotpatch commit 8ab2bcb (kept in git tag
`pcache-disagg-hotpatch-snapshot` for reference).
…fix, four-arm bench data

Rewritten investigation writeup replacing the earlier draft that
proposed three fix options (a)/(b)/(c). Now documents:

Section 1-3: symptom and static analysis of the two-sided shared_num
mismatch in receiver-directed KV transfer protocol.

Section 4-5: two failed fix attempts (hotpatch log-and-degrade;
caller-side skip mirroring D). Both left in git history/tags for
reference; hotpatch numbers were deceptive (skip-transfer masked
as compute savings).

Section 6: vLLM Nixl push mode as the reference solution — D
registers only its deficit, P is sender-driven.

Section 7: applied v2 proper fix explained, plus four-arm bench data
showing zero FATALs, correct pcache/P0 stack behavior.

Section 8: honest note that per-token numeric correctness on full
DeepSeek-V3.2 is still to be validated (blocked by the sliced
20-layer test model producing garbled output for any prompt).

Section 10-12: upstream disposition, reproduction, remaining work
before production landing.

Overturns 6/30 archived verdict "P0 has no production value" — that
verdict was reached under `pcache=false` throughout, decoupling P0
from the mechanism it depends on. Under correct pcache=true config,
P0 has consistent positive impact (mean_ttft -12%, p99_ttft -20%,
long_prefill_compute -12%).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a proper fix for prefix cache asymmetry in disaggregated PD mode using a sender-driven transfer window, alongside adding decode-first scheduling, step isolation, and prefill chunk capping to the MixScheduler. It also refactors cluster linking to support batch operations and updates profiling logic. The review feedback highlights a logic error in MixScheduler when restoring the sequence budget under prefill chunk limits, and identifies several style guide violations, including the use of at::ScalarType instead of torch::ScalarType, missing braces in loops, using push_back instead of emplace_back, and using auto for primitive types.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

c10::nullopt,
c10::nullopt,
c10::nullopt);
::std::optional<at::ScalarType>{resolved_output_dtype});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Use torch::ScalarType instead of at::ScalarType to adhere to Section 7 of the repository style guide, which specifies using the torch:: namespace instead of at:: or c10:: wherever possible.

Suggested change
::std::optional<at::ScalarType>{resolved_output_dtype});
::std::optional<torch::ScalarType>{resolved_output_dtype});
References
  1. Use torch:: namespace instead of at:: or c10:: wherever possible. Prefer the highest-level PyTorch C++ API. (link)

Comment on lines +745 to +751
if (max_prefill_chunks > 0) {
const size_t consumed_in_phase2 =
saved_seq_budget - remaining_seq_budget;
remaining_seq_budget = saved_seq_budget > consumed_in_phase2
? saved_seq_budget - consumed_in_phase2
: 0;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is a logic error in how consumed_in_phase2 and the restored remaining_seq_budget are calculated when max_prefill_chunks is enabled. Because remaining_seq_budget was capped to max_prefill_chunks before phase 2, subtracting the post-phase-2 remaining_seq_budget from the uncapped saved_seq_budget results in an incorrect (inflated) count of consumed sequences. This causes the restored remaining_seq_budget to be much lower than it should be (effectively leaving it at the capped remaining value). To fix this, calculate the actual consumed sequences by subtracting the post-phase-2 remaining_seq_budget from the capped budget value, and then subtract that actual consumed count from saved_seq_budget.

      if (max_prefill_chunks > 0) {
        const size_t phase2_initial_budget = std::min(saved_seq_budget, static_cast<size_t>(max_prefill_chunks));
        const size_t consumed_in_phase2 = phase2_initial_budget > remaining_seq_budget
                                              ? phase2_initial_budget - remaining_seq_budget
                                              : 0;
        remaining_seq_budget = saved_seq_budget > consumed_in_phase2
                                   ? saved_seq_budget - consumed_in_phase2
                                   : 0;
      }

Comment on lines +757 to +758
for (auto& r : decode_queue) running_queue_.push_back(r);
for (auto& r : prefill_queue) running_queue_.push_back(r);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Adhere to the repository style guide by:

  1. Always using braces {} for for loops, even for single-line bodies (Section 8).
  2. Preferring emplace_back over push_back to avoid unnecessary copies (Section 9).
Suggested change
for (auto& r : decode_queue) running_queue_.push_back(r);
for (auto& r : prefill_queue) running_queue_.push_back(r);
for (auto& r : decode_queue) {
running_queue_.emplace_back(r);
}
for (auto& r : prefill_queue) {
running_queue_.emplace_back(r);
}
References
  1. Always use braces {} with if, while, for, even for single-line bodies. Prefer emplace_back over push_back to construct elements in-place and avoid unnecessary copies. (link)

Comment on lines +611 to +617
for (auto& req : running_queue_) {
if (req->sequences()[0]->kv_state().kv_cache_tokens_num() > 0) {
decode_queue.push_back(req);
} else {
prefill_queue.push_back(req);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Prefer emplace_back over push_back to construct elements in-place and avoid unnecessary copies, in accordance with Section 9 of the repository style guide.

Suggested change
for (auto& req : running_queue_) {
if (req->sequences()[0]->kv_state().kv_cache_tokens_num() > 0) {
decode_queue.push_back(req);
} else {
prefill_queue.push_back(req);
}
}
for (auto& req : running_queue_) {
if (req->sequences()[0]->kv_state().kv_cache_tokens_num() > 0) {
decode_queue.emplace_back(req);
} else {
prefill_queue.emplace_back(req);
}
}
References
  1. Prefer emplace_back over push_back to construct elements in-place and avoid unnecessary copies. (link)

Comment on lines 200 to 201
auto ret =
llm_data_dist_->UnlinkLlmClusters(clusters, rets, 1000, force_flag);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Do not use auto for simple/primitive types like uint64_t or status codes, in accordance with Section 3 of the repository style guide. Please use the explicit type instead.

Suggested change
auto ret =
llm_data_dist_->UnlinkLlmClusters(clusters, rets, 1000, force_flag);
uint64_t ret =
llm_data_dist_->UnlinkLlmClusters(clusters, rets, 1000, force_flag);
References
  1. Do not use auto for simple/primitive types. auto is acceptable for complex types (iterators, lambdas, template-deduced types) but not for int32_t, float, bool, std::string, etc. (link)

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.

1 participant