Skip to content

feat(rocr): Bind asyncHandler thread to same NUMA domain as main thread - #4830

Open
Sundance636 wants to merge 5 commits into
developfrom
sclement/AILIROCR-373
Open

Sundance636 wants to merge 5 commits into
developfrom
sclement/AILIROCR-373

Conversation

@Sundance636

@Sundance636 Sundance636 commented Apr 8, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Bind asyncHandler thread to same NUMA domain as main thread, so when HSA_OVERRIDE_CPU_AFFINITY_DEBUG is unset (default), ROCr now checks the main thread's CPU affinity:

  • If restricted: background threads are bound to the same NUMA domain
  • If unrestricted: background threads remain unrestricted.

This prevents the asyncHandler thread from being scheduled on CPU cores
in a different NUMA domain than the GPU, which was causing performance
degradation on MI-300A systems.

When HSA_OVERRIDE_CPU_AFFINITY_DEBUG=0, ROCr abides by OS-set affinity.

JIRA ID

JIRA ID : AILIROCR-373

Test plan

  • Build hsa-runtime on Linux with libnuma found through pkg-config and through the ROCm NUMA CMake package
  • Confirm helper-thread affinity stays on the creator thread's NUMA domain when that thread is restricted, and remains unrestricted otherwise

@Sundance636 Sundance636 self-assigned this Apr 8, 2026
@Sundance636
Sundance636 requested a review from kentrussell as a code owner April 8, 2026 17:57
Copilot AI review requested due to automatic review settings April 8, 2026 17:57

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.

Pull request overview

This PR updates ROCr’s Linux thread creation logic to derive a NUMA-aware CPU affinity mask from the creating thread’s current CPU affinity and apply it to background threads, aiming to keep async/background work scheduled within the same NUMA domain (notably improving MI-300A performance).

Changes:

  • Added libnuma-based helpers to compute allowed NUMA nodes from the current thread’s CPU affinity and to build a CPU set from those nodes.
  • Updated os_thread CPU affinity setup to use the derived NUMA-based CPU set (with fallbacks).
Comments suppressed due to low confidence (1)

projects/rocr-runtime/runtime/hsa-runtime/core/util/lnx/os_linux.cpp:269

  • When HAVE_PTHREAD_ATTR_SETAFFINITY_NP is not defined, the cpuset allocated earlier in the override_cpu_affinity() block is never freed (since CPU_FREE(cpuset) is inside this #ifdef). Later, the non-attr path allocates another cpuset, so the original allocation leaks. Ensure cpuset is freed on all build configurations (or reuse the same allocation in the non-attr path).
#ifdef HAVE_PTHREAD_ATTR_SETAFFINITY_NP
      err = pthread_attr_setaffinity_np(&attrib, CPU_ALLOC_SIZE(cores), cpuset);
      CPU_FREE(cpuset);
      if (err != 0) {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Sundance636
Sundance636 marked this pull request as draft April 9, 2026 01:26
@Sundance636
Sundance636 force-pushed the sclement/AILIROCR-373 branch 2 times, most recently from 7ecbfe7 to fbf4e28 Compare April 21, 2026 18:40
@Sundance636
Sundance636 marked this pull request as ready for review April 21, 2026 18:41
@Sundance636
Sundance636 force-pushed the sclement/AILIROCR-373 branch 4 times, most recently from f37a41e to c018242 Compare April 28, 2026 14:42
@Sundance636
Sundance636 force-pushed the sclement/AILIROCR-373 branch 3 times, most recently from 7b1966d to f434c38 Compare May 5, 2026 19:07
@Sundance636
Sundance636 requested a review from dayatsin-amd May 5, 2026 19:07
@Sundance636
Sundance636 force-pushed the sclement/AILIROCR-373 branch 3 times, most recently from cd1fab2 to 779ed02 Compare May 19, 2026 16:31
@Sundance636
Sundance636 force-pushed the sclement/AILIROCR-373 branch from 3a7942d to a99b387 Compare August 27, 2026 19:38
@Sundance636 Sundance636 removed the Stale PR has no activity for 25+ days label Aug 27, 2026

@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) — re-review after 2026-08-27 rework

Two new commits substantially rework both NUMA detection (CMakeLists.txt) and the NUMA affinity implementation (os_linux.cpp).

shwetagkhatri's CHANGES_REQUESTED concerns — all addressed:

  • bool/bitmask mix: fixed (separate all_nodes_allowed bool and allowed_node_bitmask)
  • 64 NUMA nodes UB: fixed (explicit bounds check with stderr fallback warning)

  • TheRock build: fixed (3-tier detection: pkg-config → CMake package → manual find_library, rocm_sysdeps_numa name included, INTERFACE_INCLUDE_DIRECTORIES on IMPORTED target)
  • numa_available() guard: added

Original Important concern (CMakeLists/HAS_NUMA coupling) — mitigated. The new IMPORTED target sets INTERFACE_INCLUDE_DIRECTORIES, which propagates include dirs when target_link_libraries(... numa) is used. __has_include(<numa.h>) should now resolve correctly in practice. The cleaner fix (CMake-defined HAVE_NUMA=1) is still preferable but no longer blocking.

Remaining (non-blocking): FATAL_ERROR for all Linux builds (original Optional unchanged); indentation inconsistency in FillCpuSetFromNumaNodes and double GetAllowedNumaNodeMask() call in the #ifndef HAVE_PTHREAD_ATTR_SETAFFINITY_NP path (original Nits, unchanged).

Note: shwetagkhatri still has an outstanding CHANGES_REQUESTED. The rework addresses their specific concerns — worth pinging for a re-review.

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been inactive for 25 days and will be marked as stale.

If you would like to keep this PR open, please:

  • Add new commits
  • Add a comment explaining why it should remain open

This PR will be automatically closed in 5 days if no further activity occurs.

@github-actions github-actions Bot added the Stale PR has no activity for 25+ days label Sep 26, 2026
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

This pull request has been automatically closed due to inactivity (30 days with no updates).

If you'd like to continue working on this, feel free to reopen the PR or create a new one.

@github-actions github-actions Bot closed this Oct 1, 2026
@dayatsin-amd dayatsin-amd reopened this Oct 19, 2026
@dayatsin-amd
dayatsin-amd force-pushed the sclement/AILIROCR-373 branch from a99b387 to 2d5d8b9 Compare October 19, 2026 16:12
@dayatsin-amd
dayatsin-amd self-requested a review October 19, 2026 16:30
@dayatsin-amd

Copy link
Copy Markdown
Contributor

@shwetagkhatri @cfreeamd I made some changes to this PR:

  • When CMake finds the ROCm NUMA package, its numa::numa target is forwarded through the numa target used by the link lines.
  • If a restricted NUMA mask yields no CPUs, the helper thread falls back to the full CPU set instead of receiving an empty affinity mask.

Please review

@dayatsin-amd dayatsin-amd changed the title rocr: Bind asyncHandler thread to same NUMA domain as main thread (AILIROCR-373) feat(rocr): Bind asyncHandler thread to same NUMA domain as main thread Oct 19, 2026
@github-actions github-actions Bot removed the Stale PR has no activity for 25+ days label Oct 20, 2026

@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)

Re-review after the 2026-10-19 reopen + 94f951c3 ("Fix NUMA package linking and empty affinity fallback") and the 2026-10-21 develop merge.

The two fixes in 94f951c3 look correct:

  • numa::numa is now forwarded through an INTERFACE numa target with ROCM_PATH hints — resolves the TheRock linking concern cleanly.
  • The added_cpu guard falls back to the full CPU set when a restricted NUMA mask yields zero CPUs, preventing pthread_setaffinity_np failure → helper thread never created. Good defensive fix.

Two things still open, both one-line fixes (inline). The FATAL_ERROR now contradicts three graceful-degradation paths that the same PR added, and there's a cpuset leak in the non-pthread_attr build path.

(Note: this PR still carries the "Not ready to Review" label.)

Bind asyncHandler thread to same NUMA domain as main thread, so when
HSA_OVERRIDE_CPU_AFFINITY_DEBUG is unset (default), ROCr now checks the
main thread's CPU affinity:

- If restricted: background threads are bound to the same NUMA domain
- If unrestricted: background threads remain unrestricted.

This prevents the asyncHandler thread from being scheduled on CPU cores
in a different NUMA domain than the GPU, which was causing performance
degradation on MI-300A systems.

When HSA_OVERRIDE_CPU_AFFINITY_DEBUG=0, ROCr abides by OS-set affinity.

Signed-off-by: Sunday Clement <Sunday.Clement@amd.com>
TheRock provides dependencies like numactl via pkg-config rather than
as system packages. Update CMake to detect NUMA using pkg-config first,
with fallbacks to CMake package config and manual search, ensuring NUMA
library is properly detected and linked on both standard Linux builds
and TheRock builds, preventing build failures when IMPORTED_LOCATION
receives unresolved pkg-config variables.

Signed-off-by: Sunday Clement <Sunday.Clement@amd.com>
@bimaniamd
bimaniamd force-pushed the sclement/AILIROCR-373 branch from 43c496d to 54097aa Compare October 22, 2026 17:43
Link the ROCm numa::numa package target, and fall back to the full CPU
set when a restricted NUMA mask yields no CPUs so helper thread creation
does not fail.

Signed-off-by: David Yat Sin <David.YatSin@amd.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@bimaniamd
bimaniamd force-pushed the sclement/AILIROCR-373 branch from 54097aa to 70e31ed Compare October 22, 2026 17:50
claude-code added 2 commits October 26, 2026 11:17
…ymbols

os_linux.cpp gated its numa_* calls on __has_include(<numa.h>), a decision
made independently from CMake's NUMA_FOUND, which decides whether libnuma is
linked. When the header is visible but the library is not linked (e.g. the
pkg-config INTERFACE fallback, or libnuma present without its -dev symlink),
the translation unit compiled numa_* calls that were never linked, producing
unresolved numa_* symbols when libhsa-runtime64 is linked.

Have CMake emit HSA_HAS_NUMA=1 only in the NUMA_FOUND branch that also links
numa, and gate the C++ on HSA_HAS_NUMA instead of __has_include. The compiled
calls and the linked library are now driven by the same condition and cannot
diverge, matching how other optional dependencies in this file are gated
(HAVE_PTHREAD_ATTR_SETAFFINITY_NP, HAVE_MEMFD_CREATE).

Signed-off-by: claude-code <cfreeamd@amd.com>
The pkg-config detection tier accepted a link-flags-only INTERFACE target
when it could not resolve an actual libnuma file under pkg-config's
LIBRARY_DIRS. That target links a bare "numa" item, which resolves back to
the target itself and is silently dropped by CMake, leaving NUMA_FOUND=TRUE
while nothing links libnuma. Combined with HSA_HAS_NUMA (which follows
NUMA_FOUND), the translation unit then compiled numa_* calls with no library
on the link line -> unresolved numa_* symbols in libhsa-runtime64.

Drop that acceptance: when pkg-config reports numa but no library file
resolves, leave NUMA_FOUND FALSE and fall through to the find_package and
manual-search tiers, which resolve a real library file, or finally to the
all-CPU degrade path. NUMA_FOUND now always means a real libnuma file was
resolved and linked, so numa_* is compiled only when the library is linked.

Signed-off-by: claude-code <cfreeamd@amd.com>
@dayatsin-amd

Copy link
Copy Markdown
Contributor

This is what we say in our docs about HSA_OVERRIDE_CPU_AFFINITY_DEBUG -

$ grep -r "HSA_OVERRIDE_CPU_AFFINITY_DEBUG" runtime/hsa-runtime/core/util/flag.h: var = os::GetEnvVar("HSA_OVERRIDE_CPU_AFFINITY_DEBUG"); runtime/docs/data/env_variables.rst: * - | HSA_OVERRIDE_CPU_AFFINITY_DEBUG

    • | HSA_OVERRIDE_CPU_AFFINITY_DEBUG
      | Controls whether ROCm helper threads inherit the parent process's CPU affinity mask.

      • 1
      • | 0: Enable inheritance. Helper threads use the parent process's core affinity mask, which should be set with enough cores for all threads.
        | 1: Disable inheritance. Helper threads spawn on all available cores, ignoring the parent's affinity settings, which may affect performance in certain environments.

The patch is preserving the override_cpu_affinity guard, but it changes the documented behavior of HSA_OVERRIDE_CPU_AFFINITY_DEBUG=1 /unset above... The docs says that this mode disables affinity inheritance and lets helper threads run on all cores, while this patch restricts helper threads to the creator thread’s NUMA domain when the creator is restricted.

I think we need to also update the docs and commit message to explicitly state that default behavior is changing from full-core override to NUMA-domain inheritance.

The new behavior was discussed and agree'd here:
https://amd-hub.atlassian.net/browse/AILIROCR-373?focusedCommentId=261376

@bimaniamd Can you please check whether the current docs align with the description in that comment. Please update the docs if needed.

saleelk added a commit that referenced this pull request Nov 3, 2026
…#14742)

## Motivation

When `HSA_OVERRIDE_CPU_AFFINITY_DEBUG` is unset or `1` (the default),
ROCr helper threads (async signal handler, exception/VM-fault handler,
IPC server, PC sampling) are allowed on every CPU. On multi-socket
systems such as MI300A the OS can then schedule them on a NUMA node far
from the application and its GPU, and performance drops for applications
that pin their ranks to GPU-local cores.

This implements the behavior agreed in AILIROCR-373 and supersedes
#4830.

## Technical Details

All helper threads are created through `os::CreateThread` in
`os_linux.cpp`, so the change is made there:

- Default (`HSA_OVERRIDE_CPU_AFFINITY_DEBUG` unset or `1`): helper
threads get every CPU of each NUMA node the creating thread may run on.
An unrestricted creator spans all nodes and so still gets every CPU, as
before. If NUMA information is unavailable (no NUMA support in the
kernel, unreadable sysfs), helper threads fall back to all CPUs.
- `HSA_OVERRIDE_CPU_AFFINITY_DEBUG=0`: unchanged, helper threads inherit
the creator's exact mask.

The node CPU lists are read from
`/sys/devices/system/node/node*/cpulist` and the creator's mask from
`sched_getaffinity`, both libc only. libnuma is not used because
`os_linux.cpp` is also compiled into targets without libnuma, such as
the WSL `rocdxg` library. Unlike #4830, there is no limit on the number
of NUMA nodes, and nodes without memory are handled.

`env_variables.rst` is updated to describe the new default behavior.

Known limitation: a helper thread's mask is computed once, when the
thread is created. The exception handler thread is created inside
`hsa_init`, so pinning that the application applies after `hsa_init`
(for example OpenMP binding at the first parallel region) is not
reflected. Launcher pinning (`srun --cpu-bind`, `mpirun --bind-to`,
`taskset`, `numactl`) happens before the process starts and is covered.

Alternatives considered: binding to the GPU's NUMA node (one async
thread serves all GPUs, and ROCr cannot tell at init which GPUs the
application uses), one async thread per GPU (larger redesign of signal
waiting), and re-syncing the mask after creation (adds polling for an
uncommon case).

## Issue Tracking

JIRA ID : AILIROCR-373

## Test Plan

Built `libhsa-runtime64`. Ran a test that starts the async signal
handler and prints every thread's `Cpus_allowed_list`, on a 2-node
system (node 0: 0-55,112-167; node 1: 56-111,168-223).

## Test Result

| Launch | Helper threads allowed on |
|---|---|
| no pinning | 0-223 |
| `taskset -c 2-3` | 0-55,112-167 (node 0) |
| `taskset -c 60` | 56-111,168-223 (node 1) |
| `taskset -c 2,60` | 0-223 (both nodes) |
| `HSA_OVERRIDE_CPU_AFFINITY_DEBUG=0 taskset -c 2-3` | 2-3 (exact
inheritance) |

## Submission Checklist

- [x] Look over the contributing guidelines at
https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

This branch has not been deployed

No deployments
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