Skip to content

[AMD][DSV4] Skip the paged SWA page return under the per-request ring - #38571

Merged
HaiShaw merged 3 commits into
sgl-project:mainfrom
ankith117:fix/swa-ring-page-return-assert
Sep 9, 2026
Merged

HaiShaw merged 3 commits into
sgl-project:mainfrom
ankith117:fix/swa-ring-page-return-assert

Conversation

@ankith117

@ankith117 ankith117 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Motivation

--attention-backend dsv4 with --page-size 256 crashes both TP ranks in the
warmup decode, immediately after the server reports it is ready:

File "python/sglang/srt/mem_cache/allocator/swa.py", line 559, in free_group_end
    assert self.swa_attn_allocator.available_size() <= self.swa_attn_allocator.size
AssertionError

Deterministic, 3/3 runs, both ranks identically. The HTTP layer binds and then
the scheduler dies underneath it, so the process exits via SIGQUIT rather than
failing at startup.

Under the per-request SWA ring, the paged swa_attn_allocator is vestigial:
available_size() returns the full-attn allocator's size and
swa_available_size() computes req_slots * ring_cost, so nothing consults the
paged allocator's internal counter. Ring slots are owned by the req slot for its
lifetime and recycled with it -- they are never lent out per free. Returning
pages to the paged allocator therefore credits capacity it never lent, its
available_size() climbs past size, and the assert at the end of
free_group_end fires.

The two halves reached main a day apart and each is correct alone:

Triggering it requires page_size > 1 and the ring simultaneously, which is
exactly DSV4 on ROCm -- the backend forces page_size to 256 and the ring comes
from the model's sliding_window. No CI lane covers that intersection, and the
reland landed the day after the free path changed underneath it.

Modifications

Skip the page return when the ring is active, at both free sites in
SWATokenToKVPoolAllocator:

  • _free_swa_pages: return early in ring mode, after
    clear_full_to_swa_mapping. The mapping clear must still run -- skipping it
    leaves stale peer indices for translate_loc_from_full_to_swa, which would
    turn a clean crash into wrong KV reads.
  • free_group_end: guard the grouped drain with not self._swa_req_ring. The
    list is still drained; only the page return is skipped.

test/registered/unit/mem_cache/test_swa_ring_page_return.py (base-a-test-cpu,
no GPU) covers both free paths in ring and non-ring mode. The two ring cases
fail on the pre-fix code with the same swa.py assert seen on the GPU and pass
on the fix; the non-ring cases guard against disabling the page return outright,
and one case pins the mapping clear that must still run in ring mode.

+7 -1 in swa.py, no API or behavior change off the ring path.

Accuracy Tests

DeepSeek-V4-Flash, 2x MI45x (gfx1250), --tp 2 --attention-backend dsv4 --page-size 256 --kv-cache-dtype fp8_e4m3, ROCm 10.1.

Booting is not sufficient evidence here: the warmup request is 19 tokens and
never wraps the 256-token ring. Two checks that do exercise it, both against a
reference build that predates #38159/#38192 (v0.5.19.dev20260906+g5bebe7a033):

  • 5 greedy prompts (temperature=0), including two of 614 and 620 tokens that
    force the ring to wrap: output_ids bit-identical to the reference on all
    5. Greedy decoding is deterministic, so this is an exact-match oracle on the
    KV path, not a smoke test.
  • 32 concurrent 600+-token requests, exercising ring-slot recycling across the
    256 slots: 32/32 correct answers, zero asserts or exceptions in the log.

End-to-end on the fixed build, GSM8K full set (1319 questions, 5-shot,
temperature=0, --parallel 128):

Accuracy:          0.936
Invalid:           0.000
Latency:           447.437 s
Output throughput: 265.407 token/s

Unit coverage reproduces the failure without hardware: the two ring-mode cases
in test_swa_ring_page_return.py fail on the pre-fix code at the same
swa.py:559 assert seen on the GPU, and pass on the fix (5 passed, 7 s, CPU).

Without the fix the same config cannot serve a single request.

Speed Tests and Profiling

No performance work; the change removes calls on the ring path and touches
nothing else. Worth noting the fix restores the intended sizing rather than
degrading it -- max_total_num_tokens is 44,482,560 on the fixed build vs
28,960,000 on the pre-#38192 build used as the accuracy reference (+54%), since
falling back to that build also gives up the unified-KV pool sizing.

Serving throughput on the fixed build, from the GSM8K run above: 265.407 token/s
output at --parallel 128.

Checklist

cc @hnyls2002 @ispobock @alphabetc1 (allocator CODEOWNERS), @yuttian1 (#38192)


CI States

Latest PR Test (Base): ❌ Run #34306364007
Latest PR Test (Extra): ❌ Run #34306363799
Latest PR Test (AMD ROCm 7.2): ❌ Run #34306363963

free_group_end and _free_swa_pages return SWA pages to the paged
swa_attn_allocator unconditionally. Under the per-request SWA ring the
paged allocator is vestigial -- available_size() reads the full-attn
allocator and swa_available_size() counts req slots * ring cost, so
nothing consults its internal counter -- and pages are owned by the req
slot rather than lent per free. Returning them over-credits
available_size() past size and trips the assert at the end of
free_group_end.

Skip the page return when the ring is active, at both the grouped drain
and the direct free. The full_to_swa mapping clear still runs in ring
mode: skipping it would leave stale peer indices for
translate_loc_from_full_to_swa.

Repro: DeepSeek-V4-Flash on 2x MI45x (gfx1250), --attention-backend dsv4
--page-size 256 --tp 2. Both TP ranks die in the warmup decode right
after 'The server is fired up and ready to roll', 3/3 runs. Needs
page_size > 1 and the ring at once, which is why the two paths landing a
day apart did not collide in CI.

Verified on that config: 5 greedy prompts, including two over 600 tokens
that force the 256-token ring to wrap, produce output_ids bit-identical
to a build without the page-return path; 32 concurrent 600+-token
requests all return correct answers. max_total_num_tokens stays at
44482560 (the sizing without the fix's build is 28960000).
…est-cpu,

no GPU) covers both free paths in ring and non-ring mode. The two ring cases
fail on the pre-fix code with the same swa.py assert seen on the GPU and pass
on the fix; the non-ring cases guard against disabling the page return outright,
and one case pins the mapping clear that must still run in ring mode.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-08T22:34:21.522633Z 153e825 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@HaiShaw

HaiShaw commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

cc @amd-danli103 @yuttian1

@1am9trash 1am9trash left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Add a guard to avoid non-existed page-id free.
Only impact the amd side.

# python/sglang/srt/mem_cache/deepseek_v4_memory_pool.py
if self._unified_kv:
    self.swa_req_ring_size = self.unified_swa_ring_size

# python/sglang/srt/mem_cache/allocator/swa.py
ring_size = kvcache.swa_req_ring_size
self._swa_req_ring = ring_size is not None

if self._swa_req_ring:
    # ......

LGTM.

@amd-danli103

Copy link
Copy Markdown
Contributor

#38192 gated alloc on the ring and #38159 added the page_size>1 page return; DSV4 on ROCm is that intersection, so free_page_ids over-credits. Skipping the return after clear_full_to_swa_mapping is the right cut.
LGTM, but please fix lint first.

@yuttian1

yuttian1 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Looked through the allocation and free paths. This fix makes sense—the ring owns these SWA pages, so they shouldn’t be returned to the paged allocator. The mapping is still cleared, and the non-ring path is unchanged. LGTM.

@HaiShaw HaiShaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fix bug.

@HaiShaw
HaiShaw merged commit eb42598 into sgl-project:main Sep 9, 2026
105 of 121 checks passed
amd-danli103 added a commit to amd-danli103/sglang that referenced this pull request Sep 11, 2026
…covers it

Ours returned before clear_full_to_swa_mapping; the landed version clears the
mapping first, which test_direct_free_clears_mapping_in_ring_mode pins.
allocator/swa.py is now byte-identical to main.
mqhc2020 pushed a commit to mqhc2020/sglang that referenced this pull request Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants