Skip to content

[https://nvbugs/6674826][fix] Drop overlap hint from KV cache page-index upload - #18694

Merged
lowsfer merged 1 commit into
NVIDIA:mainfrom
lowsfer:fix/nvbug6674826-drop-overlap-flag
Sep 4, 2026
Merged

[https://nvbugs/6674826][fix] Drop overlap hint from KV cache page-index upload#18694
lowsfer merged 1 commit into
NVIDIA:mainfrom
lowsfer:fix/nvbug6674826-drop-overlap-flag

Conversation

@lowsfer

@lowsfer lowsfer commented Sep 4, 2026

Copy link
Copy Markdown
Member

copyPageIndicesToDevice() uploads page indices from a pageable host buffer using CU_MEMCPY_SRC_ACCESS_ORDER_DURING_API_CALL, whose documented contract is that "all accesses must be complete before the API call returns" — so the caller may reuse the source as soon as the call returns, with no stream sync.

Setting CU_MEMCPY_FLAG_PREFER_OVERLAP_WITH_COMPUTE alongside it breaks that guarantee. The hint makes the driver skip its inline (I2M) path and route the copy through a staged host->device path that reads the source after cuMemcpyBatchAsync has already returned, so the device receives whatever the caller wrote next instead of the page indices.

This surfaced as KvCacheManagerV2StagingBufferTest.EphemeralHostIndicesUploadIntoDeviceRing reading back the caller's sentinel value (nvbugs 6674826). In production the same path carries KV cache page indices, so a cold-page migration would push stale indices and silently corrupt KV cache pages.

The driver defect is tracked as nvbugs 6718200 with a standalone reproducer; this change is the caller-side mitigation.

Only the page-index upload is changed. copyColdPageDataBatch() keeps the hint: it uses CU_MEMCPY_SRC_ACCESS_ORDER_STREAM, which does not go through the defective branch, and it carries the bulk transfers where overlap matters.

Test Coverage

KvCacheManagerV2StagingBufferTest.EphemeralHostIndicesUploadIntoDeviceRing — the existing test that caught this. It fails on an affected driver (CUDA UMD 13.4/13.5) and passes with this change.

PR Checklist

  • PR title follows [JIRA/NVBUG/None][type] summary
  • Commit is signed off (DCO)
  • Existing test covers the change

🤖 Generated with Claude Code

Dev Engineer Review

  • Removed CU_MEMCPY_FLAG_PREFER_OVERLAP_WITH_COMPUTE from CUDA 12.8+ page-index uploads.
  • Retained the source-access-order and memory-location hints.
  • Kept the overlap hint enabled for copyColdPageDataBatch().
  • Added waivers for four Flux1/Flux2 visual-generation end-to-end tests under NVIDIA bug 6720250.
  • Removed the waiver for cpp/test_unit_tests.py::test_unit_tests[batch_manager-80].
  • The changes remain within the intended copy path and test-list scope. No public API changes were made.

QA Engineer Review

  • Modified tests/integration/test_lists/waives.txt.
  • Added four Flux1/Flux2 visual-generation end-to-end test waivers linked to NVIDIA bug 6720250.
  • Removed the cpp/test_unit_tests.py::test_unit_tests[batch_manager-80] waiver.
  • The test-list changes use the expected format and scope. CBTS touchmap coverage is not applicable.
  • Verdict: sufficient.

@lowsfer
lowsfer requested a review from a team as a code owner September 4, 2026 02:43
@lowsfer
lowsfer requested review from eopXD and thorjohnsen September 4, 2026 02:43
@lowsfer

lowsfer commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

/bot run --disable-fail-fast

@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3dfae49a-60e7-4d19-8d7a-4fccf8413595

📥 Commits

Reviewing files that changed from the base of the PR and between f7eb4f0 and 5c0ea83.

📒 Files selected for processing (1)
  • tests/integration/test_lists/waives.txt

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

The page-index device copy uses zero memcpy flags for CUDA 12.8 and newer. The integration waiver list adds skips for four Flux text-to-image end-to-end tests linked to NVIDIA bug 6720250.

Changes

CUDA memcpy update

Layer / File(s) Summary
Page-index copy attributes
cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/coldPageCopy.cu
copyPageIndicesToDevice no longer requests preferred overlap with compute. It uses zero flags and documents the driver issue affecting CU_MEMCPY_SRC_ACCESS_ORDER_DURING_API_CALL.

Integration test waivers

Layer / File(s) Summary
Flux end-to-end test waivers
tests/integration/test_lists/waives.txt
Skip waivers cover synchronous base64 and optional-parameter Flux1 and Flux2 text-to-image end-to-end tests. Each waiver references NVIDIA bug 6720250.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 5c0ea

The change avoids the affected CUDA page-index copy path and adds targeted Flux test waivers. No current merge-blocking risk remains.

Suggested reviewers: bowenfu

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required NVBugs and type format and clearly identifies the main change: removing the overlap hint from KV cache page-index uploads.
Description check ✅ Passed The description clearly explains the defect, mitigation, affected code paths, and relevant test coverage. It includes the required test coverage and checklist sections. The explanation is provided wit…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71404 [ run ] triggered by Bot. Commit: 50072af Link to invocation

@lowsfer
lowsfer force-pushed the fix/nvbug6674826-drop-overlap-flag branch from 50072af to f7eb4f0 Compare September 4, 2026 07:19
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71404 [ run ] completed with state FAILURE. Commit: 50072af
/LLM/main/L0_MergeRequest_PR pipeline #58517 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lowsfer

lowsfer commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71478 [ run ] triggered by Bot. Commit: f7eb4f0 Link to invocation

@lowsfer
lowsfer enabled auto-merge (squash) September 4, 2026 08:19
…dex upload

copyPageIndicesToDevice() uploads page indices from a pageable host buffer
using CU_MEMCPY_SRC_ACCESS_ORDER_DURING_API_CALL, whose documented contract is
that "all accesses must be complete before the API call returns" - so the
caller may reuse the source as soon as the call returns, with no stream sync.

Setting CU_MEMCPY_FLAG_PREFER_OVERLAP_WITH_COMPUTE alongside it breaks that
guarantee. The hint makes the driver skip its inline (I2M) path and route the
copy through a staged host->device path that reads the source after
cuMemcpyBatchAsync has already returned, so the device receives whatever the
caller wrote next instead of the page indices. This surfaced as
KvCacheManagerV2StagingBufferTest.EphemeralHostIndicesUploadIntoDeviceRing
reading back the caller's sentinel value, and in production would push stale
indices during cold-page migration and silently corrupt KV cache pages.

The driver defect is tracked as nvbugs 6718200 with a standalone reproducer;
this change is the caller-side mitigation.

Only the page-index upload is changed. copyColdPageDataBatch() keeps the hint:
it uses CU_MEMCPY_SRC_ACCESS_ORDER_STREAM, which does not go through the
defective branch, and it carries the bulk transfers where overlap matters.

Also remove the waiver this bug added:

    cpp/test_unit_tests.py::test_unit_tests[batch_manager-80] SKIP (nvbugs/6674826)

That waiver skips the whole batch_manager C++ unit suite, so without removing
it the fix is never exercised in CI and the bug cannot move to verify-to-close.

Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
@lowsfer
lowsfer force-pushed the fix/nvbug6674826-drop-overlap-flag branch from f7eb4f0 to 5c0ea83 Compare September 4, 2026 08:25
@lowsfer

lowsfer commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71500 [ run ] triggered by Bot. Commit: 5c0ea83 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71478 [ run ] completed with state ABORTED. Commit: f7eb4f0

Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71500 [ run ] completed with state FAILURE. Commit: 5c0ea83
/LLM/main/L0_MergeRequest_PR pipeline #58591 completed with status: 'UNSTABLE'

CI Report

⚠️ Multi-GPU Label Required:
Multi-GPU tests require the ci: full pre-merge approved label on this PR. Ask a member of NVIDIA/trt-llm-ci-approvers to add the label, then re-trigger CI with the same bot command (no rebase needed).

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

Link to invocation

@lowsfer

lowsfer commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71530 [ run ] triggered by Bot. Commit: 5c0ea83 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71530 [ run ] completed with state SUCCESS. Commit: 5c0ea83
/LLM/main/L0_MergeRequest_PR pipeline #58619 completed with status: 'SUCCESS'
Pipeline passed with automatic retried tests. Check the rerun report for details.

CI Report

Link to invocation

@lowsfer
lowsfer merged commit f7b7f22 into NVIDIA:main Sep 4, 2026
13 checks passed
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.

4 participants