Fix: size CoreTracker by the device's clusters, not by a uint64_t - #1477
Merged
ChaoWao merged 1 commit intoJul 26, 2026
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCoreTracker capacity now derives from platform limits and uses 128-bit bitmasks across the a2a3 and a5 schedulers. Bit operations and MIX placement masks were updated, while cluster ownership now rejects counts exceeding tracker capacity. ChangesCoreTracker capacity and scheduling safety
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_types.h`:
- Around line 199-213: The CoreTracker validation accepts negative cluster
counts and indices. Update CoreTracker::init and CoreTracker::set_cluster in
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_types.h:199-213,
src/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_types.h:210-224,
and
src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_types.h:200-214
to require cluster_count and cluster_idx to be non-negative as well as below
MAX_CLUSTERS, preserving the existing assertions and initialization behavior for
valid values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 579c2387-d4c6-4da1-a3d5-7107d5b2c4c0
📒 Files selected for processing (5)
src/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_types.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_types.hsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_types.h
ChaoWao
force-pushed
the
fix-coretracker-cluster-capacity
branch
from
July 26, 2026 01:03
b21b394 to
a8be9d4
Compare
CoreTracker packs three state bits per cluster (AIC, AIV0, AIV1) into a single uint64_t, so it held floor(64/3) = 21 clusters — and the two literals that encoded that, MAX_CORE_PER_THREAD = 63 and core_id_map_[63], came from the width of the backing word rather than from any device. a2a3 has 24 clusters and a5 has 36, so neither fits. assign_cores_to_threads() checks the ceiling; assign_own_clusters(), the barrier-free path the decoupled scheduler actually takes, does not. A run that gives one scheduler thread more than 21 clusters therefore writes past core_id_map_ into the next CoreTracker, which on that path is the orchestrator's. For 24 clusters the store lands exactly on core_trackers_[1].cluster_count_, so the orchestrator — whose tracker is never assigned and whose shutdown() documents "core_num() == 0 -> no-op" — reports 210 cores, walks a zeroed id map, and sends AICORE_EXIT to core 0 two hundred times. If the scheduler still has a task dispatched there it polls that core's COND for a FIN that can no longer arrive, and the run dies on the scheduler's forward-progress timeout. Nothing reaches this today because every scene test pins block_dim low enough to stay under 21 clusters per thread, but the bound is a property of the hardware, not of the test suite. - MAX_CLUSTERS now derives from PLATFORM_MAX_BLOCKDIM and MAX_CORE_PER_THREAD from it, so one scheduler thread can own a whole device on either arch. A static_assert ties the pair to the storage. - BitStates is backed by unsigned __int128 (a2a3 needs 72 bits, a5 108). The hot path shifts the whole mask by 1 and 2 to test cluster co-residency, and letting the compiler generate those crossings is what keeps the widening honest; only popcount and ctz split by hand. - Callers can no longer build a mask with `1ULL << offset`, undefined once offset reaches 64. BitStates::bit() owns the shift. - assign_own_clusters() gets the guard its serial sibling already had, and init()/set_cluster() assert the bound at both ends, so exceeding it fails by name instead of corrupting a neighbour. The lower bound matters as much as the upper one: a negative cluster_idx would index core_id_map_ backwards, and a negative cluster_count would leave every dispatch loop visiting no clusters at all.
This was referenced Jul 26, 2026
Closed
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Jul 27, 2026
The guide covered capacity codes and stalls but had nothing for an AICore addressing fault, which is the third way a run reaches the same generic 507018. hw-native-sys#1489 spent an investigation re-deriving the procedure from scratch and still could not close, so record it. Three steps, ordered so the cheap disqualifications come first: - F1 separates "the kernel computed a bad address" from "the core was made to execute something that is not the kernel". `binSize` in GetBinAndKernelNameExceptionArgs matching the runtime's own aicore_kernel.o means the report is naming the polling-dispatch executor, so the fault is a dispatch-payload problem and no amount of reading the kernel's arithmetic will find it. hw-native-sys#1036 is the worked example. - F2 rules the kernel in or out statically. Constant TASSIGN bases plus template tile extents cannot produce an out-of-range address, and runtime values that only shrink a tile move nothing — so summing the highest byte reached against the UB size settles it without instrumenting anything. This is what cleared the allreduce collectives in hw-native-sys#1489. - F3 separates a real fault from a post-mortem register dump of a core reaped mid-spin, by counting which detector actually fired. hw-native-sys#1489 has since been closed as very likely fixed by hw-native-sys#1477 — a CoreTracker whose uint64_t state overran past 21 clusters while those cases ran at the device's full 24. F2's verdict held: the kernels were never at fault, and the answer was in F1's second case all along. The section says so, because a worked example that names its own outcome is worth more than one that trails off. Also state where the device log has to be redirected and why the harness will not do it: outputs/<case>_<ts>/ exists only when a DFX flag is on, so a plain onboard run leaves its log in the shared ~/ascend/log/debug/ with every other user's, and the evidence is unattributable once the run ends. That is how hw-native-sys#1489's only real occurrence lost the one artifact that would have decided F1. Renumber the trailing section and fix the in-page link that pointed at its old number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Jul 27, 2026
The guide covered capacity codes and stalls but had nothing for an AICore addressing fault, which is the third way a run reaches the same generic 507018. hw-native-sys#1489 spent an investigation re-deriving the procedure from scratch and still could not close, so record it. Three steps, ordered so the cheap disqualifications come first: - F1 separates "the kernel computed a bad address" from "the core was made to execute something that is not the kernel". `binSize` in GetBinAndKernelNameExceptionArgs matching the runtime's own aicore_kernel.o means the report is naming the polling-dispatch executor, so the fault is a dispatch-payload problem and no amount of reading the kernel's arithmetic will find it. hw-native-sys#1036 is the worked example. - F2 rules the kernel in or out statically. Constant TASSIGN bases plus template tile extents do not make an out-of-range address impossible — the constants can overrun on their own — but they make it decidable on paper, because runtime values that only shrink a tile move no base and grow no extent. Summing the highest byte reached and comparing against the UB size settles it either way without touching the device. This is what cleared the allreduce collectives in hw-native-sys#1489. - F3 separates a real fault from a post-mortem register dump of a core reaped mid-spin, by counting which detector actually fired. hw-native-sys#1489 has since been closed as very likely fixed by hw-native-sys#1477 — a CoreTracker whose uint64_t state overran past 21 clusters while those cases ran at the device's full 24. F2's verdict held: the kernels were never at fault, and the answer was in F1's second case all along. The section says so, because a worked example that names its own outcome is worth more than one that trails off. Also state where the device log has to be redirected and why the harness will not do it: outputs/<case>_<ts>/ exists only when a DFX flag is on, so a plain onboard run leaves its log in the shared ~/ascend/log/debug/ with every other user's, and the evidence is unattributable once the run ends. That is how hw-native-sys#1489's only real occurrence lost the one artifact that would have decided F1. Renumber the trailing section and fix the in-page link that pointed at its old number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ChaoWao
added a commit
that referenced
this pull request
Jul 27, 2026
The guide covered capacity codes and stalls but had nothing for an AICore addressing fault, which is the third way a run reaches the same generic 507018. #1489 spent an investigation re-deriving the procedure from scratch and still could not close, so record it. Three steps, ordered so the cheap disqualifications come first: - F1 separates "the kernel computed a bad address" from "the core was made to execute something that is not the kernel". `binSize` in GetBinAndKernelNameExceptionArgs matching the runtime's own aicore_kernel.o means the report is naming the polling-dispatch executor, so the fault is a dispatch-payload problem and no amount of reading the kernel's arithmetic will find it. #1036 is the worked example. - F2 rules the kernel in or out statically. Constant TASSIGN bases plus template tile extents do not make an out-of-range address impossible — the constants can overrun on their own — but they make it decidable on paper, because runtime values that only shrink a tile move no base and grow no extent. Summing the highest byte reached and comparing against the UB size settles it either way without touching the device. This is what cleared the allreduce collectives in #1489. - F3 separates a real fault from a post-mortem register dump of a core reaped mid-spin, by counting which detector actually fired. #1489 has since been closed as very likely fixed by #1477 — a CoreTracker whose uint64_t state overran past 21 clusters while those cases ran at the device's full 24. F2's verdict held: the kernels were never at fault, and the answer was in F1's second case all along. The section says so, because a worked example that names its own outcome is worth more than one that trails off. Also state where the device log has to be redirected and why the harness will not do it: outputs/<case>_<ts>/ exists only when a DFX flag is on, so a plain onboard run leaves its log in the shared ~/ascend/log/debug/ with every other user's, and the evidence is unattributable once the run ends. That is how #1489's only real occurrence lost the one artifact that would have decided F1. Renumber the trailing section and fix the in-page link that pointed at its old number.
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
CoreTrackerpacks three state bits per cluster (AIC, AIV0, AIV1) into a singleuint64_t, so it holdsfloor(64/3) = 21clusters. The two literals thatencoded that came from the width of the backing word, not from any device:
a2a3 has 24 clusters and a5 has 36. Neither fits.
assign_cores_to_threads()checks the ceiling (scheduler_cold_path.cpp:990).assign_own_clusters()— the barrier-free path the decoupled scheduler actuallytakes — does not. Give one scheduler thread more than 21 clusters and
set_cluster()writes pastcore_id_map_into the nextCoreTracker, which onthat path is the orchestrator's.
At 24 clusters the store lands exactly on
core_trackers_[1].cluster_count_(
sizeof(CoreTracker)==320,offsetof(core_id_map_)==40, socore_id_map_[70]→ byte 320). The orchestrator's tracker is never assigned andits
shutdown()documents the assumption it breaks:// Orchestrator threads have core_trackers_[thread_idx].core_num() == 0 -> no-op.It instead reports 210 cores, walks a zeroed id map, and sends
AICORE_EXITtocore 0 two hundred times. If the scheduler still has a task dispatched there it
polls that core's COND for a
FINthat can never arrive:cond_tok=2147483646=0x7FFFFFFE=AICORE_EXIT_TASK_ID. Surfaces assched_error_code=100 SCHEDULER_TIMEOUT→ host507018. No deadlock orcapacity detector fires — counts of
Task Allocator Deadlock, SPINTimeout (N cycles)andHandleTaskTimeoutare all 0 — which is why this readsas a mystery stall rather than an overflow.
Bisected onboard by clamping the resolved width:
Exactly the byte-offset prediction: only
cluster_idx == 23reachescore_id_map_[70]. 22 and 23 are already corrupt — the bitmask aliases oncepast 63 bits (
1ULL << 66becomesLSL #2on aarch64) — the write just landsin tail padding and survives.
Nothing reaches this today because every scene test pins
block_dimlow enoughto stay under 21 clusters per thread. The bound is a property of the hardware,
not of the test suite, so any caller passing
aicpu_thread_num: 2on a24-cluster device hits it.
The fix
MAX_CLUSTERSderives fromPLATFORM_MAX_BLOCKDIMandMAX_CORE_PER_THREADfrom it, so one scheduler thread can own a whole deviceon either arch. A
static_assertties the pair to the storage width.(
MAX_CORE_PER_THREADwas also a mutablestatic inlineused as a constant —now
constexpr.)BitStatesis backed byunsigned __int128(a2a3 needs 72 bits, a5 108).The hot path shifts the whole mask by 1 and 2 to test cluster co-residency
(
scheduler_types.h:260/262/427/432); letting the compiler generate thosecrossings is what keeps the widening honest. Only
popcountandctzsplit byhand.
__uint128_tis already used on this target insrc/common/platform/include/aicpu/device_time.h.1ULL << offset, undefined onceoffset reaches 64 — there were 13 such sites and the widening would have made
every one of them UB.
BitStates::bit()owns the shift.assign_own_clusters()gets the guard its serial sibling already had(a2a3 + a5), and
init()/set_cluster()assert the bound, so exceeding itfails by name instead of corrupting a neighbour.
Verification
Repro before the fix:
dummy_task,predicated_dispatch(tmr and hbg),dep_gen_chainataicpu_thread_num: 2with the width auto-resolved to 24 —100% reproducible. After: 5/5 pass, and
Shutting down 210 coresis gone fromthe device log.
pytest examples tests/st --platform a2a3(onboard)pytest examples tests/st --platform a2a3simpytest examples tests/st --platform a5simpytest tests/ut -m "not requires_hardware"ctestC++ unit testshbg reaches the same ceiling through the guarded path, so it fails cleanly today
(
Can't assign more then 64 cores in per scheduler) rather than corrupting —the widening lifts that limit too.