Skip to content

fix(hrr): keep a hipHostRegister replay buffer alive while a graph node points into it - #15100

Merged
andraghetti merged 2 commits into
developfrom
users/andraghetti/hrr-3081-host-register-keep
Nov 6, 2026
Merged

andraghetti merged 2 commits into
developfrom
users/andraghetti/hrr-3081-host-register-keep

Conversation

@andraghetti

Copy link
Copy Markdown
Contributor

Motivation

A graph memcpy node keeps the host pointer it was built with. Replay freed the buffer behind a hipHostRegister on hipHostUnregister, and again when the same address was registered a second time, so the next launch of that graph read or wrote freed heap. The copy size and direction come from the archive.

Technical Details

  • The register buffers now live in host_reg_blocks, kept with the per-address block mechanism fix(hrr): keep graph landing buffers stable until teardown #14156 added for graph landing buffers. Its body moves into PlaybackContext::host_block, shared by both maps (LandingBuffer is renamed HostBlock).
  • Registering the same address again reuses its block; an outgrown block is kept. hipHostUnregister drops the mapping only. Every block stays charged to the host budget until the context is destroyed, so replay memory at an address grows only when a registration outgrows its block.
  • A block whose unregister fails while being replaced is set aside, and the new registration gets a fresh one.
  • remove_alloc no longer gives back a hipHostRegister charge. The blocks are new[] arrays owned by the context, so the posix_memalign/_aligned_malloc allocation and its six free/_aligned_free sites are gone.
  • DESIGN.md and the comments describe the new lifetime.

Issue Tracking

JIRA ID: AIRUNTIME-3081

Test Plan

  • New cases in hrr_unregister_fail_test.cc (hrr-handler-tests, AddressSanitizer build): GraphNodeKeepsBuffer (register, take the pointer a memcpy node is given, register again, unregister, write through it) and FailedReplaceTakesNewBlock. Both need a GPU and skip without one.
  • MI350 gate pAIRUNTIME-3081-0c8cd41 on head 0c8cd41, base develop b09c3ab.
  • The gate runs hrr-handler-tests without a GPU, so the new cases were also run by hand on an MI350 for the head and for a negative control (a598f489a2: unregister frees the block, and a still-registered block is reused).

Test Result

  • Gate: GREEN, no failure beyond develop's own.
  • On the GPU, head: all 54 handler test cases pass. Negative control: GraphNodeKeepsBuffer aborts with heap-use-after-free and FailedReplaceTakesNewBlock fails; every other case passes.

Submission Checklist

…de points into it

A graph memcpy node keeps the host pointer it was built with. Replay freed
the buffer behind a hipHostRegister on hipHostUnregister, and when the same
address was registered again, so the next launch of that graph read or wrote
freed heap.

The register buffers now live in host_reg_blocks, kept with the per-address
block mechanism the graph landing buffers use (host_block): registering the
same address again reuses its block, an outgrown block is kept, unregister
drops only the mapping, and every block stays charged to the host budget
until the context is destroyed. A block whose unregister fails is not
reused.

AIRUNTIME-3081
@andraghetti
andraghetti requested a review from a team as a code owner November 5, 2026 10:31
@andraghetti
andraghetti requested review from danielbolsson and a balanced review from Copilot November 5, 2026 10:31
@therock-pr-bot

therock-pr-bot Bot commented Nov 5, 2026

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

Copilot AI 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.

🟡 Changes recommended

Critical buffer-lifetime and alignment defects can free still-registered or GPU-in-use memory and cause corruption between otherwise independent cross-GPU buffers.

4 open issues
What changed in this PR

This PR extends the stable host-buffer lifetime introduced in #14156 to hipHostRegister replay buffers, preventing graph nodes from referencing freed storage after re-registration or unregister.

Changes:

  • Shares context-owned block storage between registration and graph landing buffers, retaining outgrown blocks and their budget charges.
  • Updates unregister and teardown handling to separate registration cleanup from buffer ownership.
  • Adds GPU-dependent lifetime and failed-replacement tests, and updates documentation.
File Description
projects/​hrr/​tests/​unit/​hrr_unregister_fail_test.cc Adds buffer-lifetime and failed-replacement regression tests.
projects/​hrr/​tests/​unit/​hrr_host_budget_test.cc Updates comments describing retained registration charges.
projects/​hrr/​playback/​hrr_playback.cpp Removes direct registration-buffer frees during teardown.
projects/​hrr/​playback/​hip_playback.h Introduces shared host-block storage and retained budget accounting.
projects/​hrr/​playback/​hip_playback.cpp Reuses registration blocks and retains storage after unregister.
projects/​hrr/​DESIGN.md Documents registration-buffer ownership and lifetime.

Review effort: Balanced

Comment thread projects/hrr/playback/hip_playback.cpp Resolved
Comment thread projects/hrr/playback/hip_playback.cpp Resolved
Comment thread projects/hrr/playback/hip_playback.h Resolved
Comment thread projects/hrr/playback/hip_playback.h Resolved

@danielbolsson danielbolsson 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.

LGTM: host_block reuse keeps the registered buffer alive past hipHostUnregister, charged once, and the new GraphNodeKeepsBuffer test covers it. Nits: projects/hrr/playback/hrr_playback.cpp:2035 — the fatal-error return skips cleanup(), so ~PlaybackContext frees blocks still registered (unregister them first); projects/hrr/playback/hip_playback.h:925 — a throwing make_unique after charge_host leaves the charge taken, catch and release.

Host blocks are 64-byte aligned, an allocation failure returns
hipErrorMemoryAllocation instead of throwing, and the fatal path exits
without destroying the context while queued work may still use a block.

AIRUNTIME-3081
@andraghetti
andraghetti merged commit b67e20d into develop Nov 6, 2026
59 of 60 checks passed
@andraghetti
andraghetti deleted the users/andraghetti/hrr-3081-host-register-keep branch November 6, 2026 10:14
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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.

3 participants