Skip to content

fix(rocr): fix incorrect ALLOC_MAX_SIZE reporting for HSA_HEAPTYPE_DEVICE_SVM pools - #11355

Merged
lamikr merged 7 commits into
ROCm:developfrom
prihosur-amd:users/prihosur-amd/fix-device-svm-alloc-max-size
Oct 29, 2026
Merged

lamikr merged 7 commits into
ROCm:developfrom
prihosur-amd:users/prihosur-amd/fix-device-svm-alloc-max-size

Conversation

@prihosur-amd

@prihosur-amd prihosur-amd commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Device-SVM/AIE memory pools advertised the wrong maximum allocation
size, causing concurrent allocate/free stress to fail on AIE pools
(e.g. aie2p Pool 8) on gfx1151 (Strix Halo APU).

Technical Details

MemoryRegion::GetInfo() returned max_sysmem_alloc_size_ for
HSA_HEAPTYPE_DEVICE_SVM regions instead of the pool's own
max_single_alloc_size_. MemoryRegion::AllocateImpl() had the same
issue in its IsSystem() size-cap check. As a result, device-SVM/AIE
pools advertised the system-memory allocation limit instead of their
real per-pool limit, letting allocators request sizes the pool cannot
satisfy and fail with HSA_STATUS_ERROR_OUT_OF_RESOURCES.

Fix: use max_single_alloc_size_ for HSA_HEAPTYPE_DEVICE_SVM in both
GetInfo()'s HSA_REGION_INFO_ALLOC_MAX_SIZE case and
AllocateImpl()'s size-cap check, matching the value already used for
local-memory allocations.

Issue Tracking

JIRA ID: ROCM-21660

Test Plan

Built hsa-runtime64 with the fix and ran the rocrtst reproducer
before/after on gfx1151 (Strix Halo APU) hardware.

Test Result

Test Before After
rocrtstStress.Memory_Concurrent_Allocate_Test FAIL (HSA_STATUS_ERROR_OUT_OF_RESOURCES, Pool 8) PASS
rocrtstStress.Memory_Concurrent_Free_Test FAIL (HSA_STATUS_ERROR_OUT_OF_RESOURCES, Pool 8) PASS

Submission Checklist

@therock-pr-bot

therock-pr-bot Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/rocr-runtime/runtime/hsa-runtime/core/driver/xdna/amd_xdna_driver.cpp, projects/rocr-runtime/runtime/hsa-runtime/core/runtime/amd_aie_agent.cpp, projects/rocr-runtime/runtime/hsa-runtime/core/runtime/amd_memory_region.cpp; no test file found
🚫 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?

@therock-pr-bot

therock-pr-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

…SVM pools

MemoryRegion::GetInfo() and AllocateImpl() used max_sysmem_alloc_size_
for HSA_HEAPTYPE_DEVICE_SVM regions, so device-SVM/AIE pools advertised
the system-memory allocation cap instead of their own per-pool limit.
This let allocators request sizes the pool cannot actually satisfy,
which fails during concurrent alloc/free on AIE pools (e.g. aie2p Pool 8)
with HSA_STATUS_ERROR_OUT_OF_RESOURCES.

Report max_single_alloc_size_ for HSA_HEAPTYPE_DEVICE_SVM instead, matching
the same value already used in AllocateImpl()'s own local-memory size check.

Verified on gfx1151 (Strix Halo APU) with rocrtst:
- rocrtstStress.Memory_Concurrent_Allocate_Test: FAIL -> PASS
- rocrtstStress.Memory_Concurrent_Free_Test: FAIL -> PASS
@prihosur-amd
prihosur-amd force-pushed the users/prihosur-amd/fix-device-svm-alloc-max-size branch from 169aa7f to e4f386c Compare September 9, 2026 10:12

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

@shwetagkhatri
shwetagkhatri self-requested a review October 1, 2026 18:05

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

These are non blocking comments - but while you are owning this change - i would like to fix the following corresponding files as well -

2 comments in other files below are - now saying the opposite of what the code is doing and whoever reads them next - is going to think this bug is still open.

amd_aie_agent.cpp -
is currently saying:
// The system will report the max allocatable size as MemoryRegion::max_sysmem_alloc_size_, but
// this is incorrect. The pool is exactly XdnaDriver::GetDevHeapByteSize() bytes.

this can be fixed now:

// The pool reports its max allocatable size as MemoryRegion::max_single_alloc_size_, which is exactly
// XdnaDriver::GetDevHeapByteSize() bytes.

amd_xdna_driver.cpp -
is currently saying:

// While this is already checked in MemoryRegion::AllocateImpl, the max size is
// MemoryRegion::max_sysmem_alloc_size_ for HSA_HEAPTYPE_DEVICE_SVM which is incorrect
// for dev heap.

can be fixed now:

  // MemoryRegion::AllocateImpl already caps this at MemoryRegion::max_single_alloc_size_, which
  // is dev_heap_size for HSA_HEAPTYPE_DEVICE_SVM. Kept as a defensive duplicate returning the
  // same status, so an oversized CREATE_BO cannot reach the driver if that cap ever regresses.

Comment on lines 241 to 245
case HSA_HEAPTYPE_FRAME_BUFFER_PRIVATE:
case HSA_HEAPTYPE_FRAME_BUFFER_PUBLIC:
case HSA_HEAPTYPE_GPU_SCRATCH:
*((size_t*)value) = max_single_alloc_size_;
break;

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.

Actually looking into the full source code - the new lines 238-240 have the code body same as 241-245. Please fold the label into the existing group here as below:

      case HSA_HEAPTYPE_DEVICE_SVM:
      case HSA_HEAPTYPE_FRAME_BUFFER_PRIVATE:
      case HSA_HEAPTYPE_FRAME_BUFFER_PUBLIC:
      case HSA_HEAPTYPE_GPU_SCRATCH:
        *((size_t*)value) = max_single_alloc_size_;
        break;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

resolved

@shwetagkhatri
shwetagkhatri self-requested a review October 1, 2026 18:32

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

🤖 AI-assisted pre-review (for @cfreeamd)

Verdict: approve-with-nits — the fix is correct. One Required item on the PR title before merge.


What this PR does

Fixes two related bugs where HSA_HEAPTYPE_DEVICE_SVM pools were using the system-memory allocation cap (max_sysmem_alloc_size_) instead of the device allocation cap (max_single_alloc_size_):

  1. GetInfo(HSA_REGION_INFO_ALLOC_MAX_SIZE) — HSA_HEAPTYPE_DEVICE_SVM was grouped with HSA_HEAPTYPE_SYSTEM and returned max_sysmem_alloc_size_. Now split out and returns max_single_alloc_size_.
  2. AllocateImpl() — The IsSystem() allocation-size guard was using max_sysmem_alloc_size_ even when IsDeviceSVM() is true. Fixed to use max_single_alloc_size_ when IsDeviceSVM().

Both sites are now consistent: HSA_HEAPTYPE_DEVICE_SVM is VRAM-backed (device memory), so reporting and enforcing max_single_alloc_size_ is correct.


[Required] PR title missing type(component): format

Current title: rocr: fix incorrect ALLOC_MAX_SIZE reporting for HSA_HEAPTYPE_DEVICE_SVM pools

Suggested: fix(rocr): fix incorrect ALLOC_MAX_SIZE reporting for HSA_HEAPTYPE_DEVICE_SVM pools

ROCm squash-merges, so the PR title becomes the permanent commit title. The type(component): prefix is required by the commit convention.


What looks correct

  • Both GetInfo() and AllocateImpl() are updated consistently — they now agree on which cap applies to HSA_HEAPTYPE_DEVICE_SVM.
  • The IsDeviceSVM() check in AllocateImpl() is correctly placed before the max_sysmem_alloc_size_ guard, so the ternary selects the right cap.
  • No performance impact — both changed paths are in the cold allocation path.

@cfreeamd

cfreeamd commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Please address Shweta's comment and change the PR title to the github friendly format; e.g.,

fix(rocr): fix incorrect ALLOC_MAX_SIZE reporting for HSA_HEAPTYPE_DEVICE_SVM pools

Otherwise, LGTM.

@ypapadop-amd ypapadop-amd 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, only one comment.

For some context, I was hesitant to change what SVM was reporting for size (thus the comments in XdnaDriver) because I did not have any insight on if someone was expecting the system memory as a result.

@prihosur-amd prihosur-amd changed the title rocr: fix incorrect ALLOC_MAX_SIZE reporting for HSA_HEAPTYPE_DEVICE_SVM pools fix(rocr): fix incorrect ALLOC_MAX_SIZE reporting for HSA_HEAPTYPE_DEVICE_SVM pools Oct 8, 2026
@lamikr

lamikr commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

Merging, only failed test is the system-pr bot which failed for existing issue: rocm-systems #11156

(open, created 2026-09-02): "therock-pr-bot fails on every develop PR:
required 'pre-commit' check is never published (pre-formatting.yml is gated to testbranch)".****

@lamikr
lamikr merged commit 415459e into ROCm:develop Oct 29, 2026
1086 of 1184 checks passed
@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.

6 participants