fix(ring-buffer): make structural deadlock detection scope-aware - #1610
Conversation
📝 WalkthroughWalkthroughThe A2A3 and A5 runtimes now identify the oldest task pinned by an open scope during capacity checks. Structural deadlocks produce immediate diagnostics, while other reclaim stalls use timeout handling. Tests and troubleshooting guidance cover both outcomes. ChangesDeadlock detection and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PTO2Orchestrator
participant PTO2TaskAllocator
participant PTO2DepListPool
participant RingHead
PTO2Orchestrator->>PTO2TaskAllocator: pass oldest open task
PTO2TaskAllocator->>RingHead: compare reclaim head
PTO2Orchestrator->>PTO2DepListPool: pass oldest open task
PTO2DepListPool->>RingHead: compare reclaim head
RingHead-->>PTO2TaskAllocator: structural deadlock or timeout path
RingHead-->>PTO2DepListPool: structural deadlock or timeout path
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
The task/heap allocator and dependency-list pool currently infer an immediate structural deadlock from independently sampled task state and reference counts. A reclaim head from an earlier closed scope can still become CONSUMED, so this can race normal scheduler reclaim and report a false fatal. Pass the oldest task retained by any open scope on the blocked ring to both allocation paths. Report an immediate fatal only when that slot is the reclaim head; otherwise preserve the 500 ms no-progress timeout. Cover current-scope and open-ancestor cycles, different-scope timeout fallback, and dependency-pool behavior on a2a3 and a5. Update diagnostics to distinguish proven structural cycles from reclaim timeouts without adding locks, scans, or shared-memory fields. Co-authored-by: sunkaixuan2018 <baiyi@mail.ustc.edu.cn>
19fca94 to
b7ad6e9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.h (1)
408-416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe structural deadlock check is implemented twice per architecture.
head_is_oldest_open_taskproves the structural deadlock by comparing a pointer to the current head-task slot. Each architecture defines this check once as a named method onPTO2TaskAllocatorand reimplements the identical comparison inline insidePTO2DepListPool::ensure_space. Two independent copies of this safety-critical proof, per architecture, raise the risk that a future edit updates one copy and not the other.
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.h#L408-L416: keep this as the canonical implementation for a2a3.src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.cpp#L205-L210: replace the inlinebool head_is_oldest_open_task = ...computation with a call to a shared a2a3 helper (taking the slot-state array/index andoldest_open_task), instead of reimplementing the comparison.src/a5/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.h#L408-L416: keep this as the canonical implementation for a5.src/a5/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.cpp#L205-L210: replace the inline computation with a call to the shared a5 helper, mirroring the a2a3 fix.🤖 Prompt for 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. In `@src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.h` around lines 408 - 416, The structural deadlock check is duplicated between the canonical PTO2TaskAllocator::head_is_oldest_open_task method and PTO2DepListPool::ensure_space. Keep the named method unchanged in src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.h:408-416 and src/a5/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.h:408-416; in src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.cpp:205-210 and src/a5/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.cpp:205-210, replace each inline comparison with a call to the corresponding shared architecture helper using the slot-state array/index and oldest_open_task.
🤖 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.
Nitpick comments:
In `@src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.h`:
- Around line 408-416: The structural deadlock check is duplicated between the
canonical PTO2TaskAllocator::head_is_oldest_open_task method and
PTO2DepListPool::ensure_space. Keep the named method unchanged in
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.h:408-416 and
src/a5/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.h:408-416; in
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.cpp:205-210
and src/a5/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.cpp:205-210,
replace each inline comparison with a call to the corresponding shared
architecture helper using the slot-state array/index and oldest_open_task.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e7729690-69d1-4d96-a963-b02a98a7db81
📒 Files selected for processing (18)
.claude/rules/running-onboard.mddocs/troubleshooting/device-error-codes/capacity.mdsrc/a2a3/runtime/host_build_graph/runtime/pto_ring_buffer.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_runtime2_types.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_shared_memory.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/pto_scheduler.hsrc/a5/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.hsrc/a5/runtime/tensormap_and_ringbuffer/runtime/pto_runtime2_types.hsrc/a5/runtime/tensormap_and_ringbuffer/runtime/pto_shared_memory.htests/ut/cpp/CMakeLists.txttests/ut/cpp/a2a3/test_orchestrator_fanin.cpptests/ut/cpp/a5/test_orchestrator_fanin.cpptests/ut/cpp/common/test_scope_deadlock_detection.cpp
|
Regarding the CodeRabbit maintainability nit: I am keeping the two checks local to their allocation paths. The task allocator derives the head slot from its private |
2 participants
Summary
This change makes the
tensormap_and_ringbufferstructural-deadlock check usescope identity: it reports an immediate fatal only for a cycle that the
blocked orchestrator itself makes impossible to break. Other no-progress
states remain covered by the existing timeout.
Issue
The affected allocation paths are:
PTO2TaskAllocator::alloc(), which reserves both a task-window slot and thetask's output bytes from the ring heap and diagnoses task-window
(
PTO2_ERROR_FLOW_CONTROL_DEADLOCK) or heap(
PTO2_ERROR_HEAP_RING_DEADLOCK) exhaustion;PTO2DepListPool::ensure_space(), which reserves dependency-list entries fora task with live fanin and reports
PTO2_ERROR_DEP_POOL_OVERFLOWwhen itgives up.
Insufficient space does not immediately mean allocation has failed. Both paths
wait for the scheduler to make the ring's reclaim head
CONSUMED, advancelast_task_alive, reclaim space, and then retry. This is normal backpressure.They return failure only after another fatal is observed or their own
structural/timeout detector concludes that waiting cannot continue.
The task window, ring heap, and dependency-list pool hold different data, but
all three reclaim in task order using the same ring-local
last_task_alivewatermark. Therefore the same head task controls whether any of these blocked
allocations can recover.
The merge-base structural detector turns that wait into an immediate fatal
when the reclaim-head task appears
COMPLETED, has released all consumerreferences, but has not released its scope reference. The same lifecycle-based
predicate is used for task-window exhaustion, heap exhaustion, and
dependency-pool exhaustion.
Those lifecycle fields do not identify which scope owns the reclaim head.
The head may belong to a closed child, sibling, or earlier serial scope whose
scope_endhas already executed. The scheduler can still transition such ahead to
CONSUMED, advancelast_task_alive, and unblock the allocation.Because the merge-base check reads the lifecycle fields while the scheduler is
progressing them, it can report a structural fatal immediately before that
valid reclaim.
The missing proof is whether this allocation wait prevents a still-pending
scope_endthat is required for this particular head to becomeCONSUMED.Lifecycle state alone cannot establish that relationship.
Versions
b7ad6e97e383868ba34326caa3ff2ada733349f2.21e82d5e54eb8dbb922557a102afca8f58a89ea0.83d01313d9bfc247c4b7c8bcf969d1019f0d106f.Fix
PTO2TaskAllocator::alloc()andPTO2DepListPool::ensure_space()with anoldest-open-task identity check.
O(1) from the orchestrator's existing
scope_beginsandscope_tasksbookkeeping.
immediate structural fatal only if that pointer equals the reclaim-head
slot. Otherwise, keep waiting for reclaim and use the existing approximately
500 ms no-progress timeout as the fallback.
tensormap_and_ringbuffer.diagnostics and troubleshooting documentation.
No shared-memory fields, task-slot fields, locks, atomics, runtime hooks, or
linear scope scans are added.
Definitions
dependency-pool allocation cannot obtain space.
scope_beginbut not its matchingscope_end.scope_tasksrecords emitted task-slot pointers in order, andscope_begins[depth]records where each open scope starts in that array.H(head) is the slot addressed by the blocked ring's currently observedlast_task_alive:slot_states[last_task_alive & window_mask]. It is theoldest slot that this ring has not reclaimed. The reclaim boundary cannot
move past
Huntil the scheduler makesHCONSUMED.O(oldest open task) is obtained frombegin = scope_begins[current_ring_id()]. Ifbegin < scope_tasks_size,then
O = scope_tasks[begin]; otherwiseO = nullptr. Because task pointersare recorded in emission order,
Ois the oldest task on the blocked ringretained by the shallowest open scope mapped to that ring, including its
still-open descendants. Its scope reference cannot be released until the
blocked orchestrator reaches the corresponding
scope_end.H == Ocompares task-slot pointers, not task IDs or independently sampledlifecycle fields.
Cases
The same
H/Odecision applies whether the unavailable resource is atask-window slot, ring-heap space, or dependency-list capacity.
Current-scope head:
H == OThe current scope has emitted
Hand then blocks in one of the affectedallocation paths on the same ring. The orchestrator cannot reach this
scope's
scope_end, soHcannot release its scope reference or becomeCONSUMED. The fix reports an immediate structural fatal.Open-parent head:
H == OA nested scope blocks while allocating on a ring also used by an open
parent, and
His the parent's oldest retained task on that ring. Theorchestrator cannot leave the child and reach the parent's
scope_end, soHcannot becomeCONSUMED. The fix also reports an immediate structuralfatal. This includes nesting deeper than the number of physical rings,
where multiple scope depths share the deepest ring.
Earlier sibling head on the same ring:
H != OTwo sibling scopes execute serially on the same ring. The earlier scope has
executed
scope_end, but its headHhas not yet transitioned toCONSUMED. The later scope is now open and blocks while allocating on thatring;
Obelongs to the later scope, soH != O. The scheduler may stillconsume
Hand unblock allocation, which makes an immediate structuralfatal unsafe. The fix keeps waiting. If
Hnever advances, the existingtimeout reports the sustained no-progress deadlock.
Why this proves a deadlock
When
H == O, the blocked allocation prevents the orchestrator from reachingthe
scope_endthat would releaseO's scope reference.Otherefore cannotbecome
CONSUMED, while ordered reclamation cannot advance past the same slotas
H. The allocation is waiting for a reclaim that the blocked allocationitself prevents, so this is a structural cycle and can be reported
immediately.
When
H != O, the blocked orchestrator is not proven to pinH. The schedulermay still consume it and advance the reclaim boundary. The allocator therefore
continues waiting; if the boundary remains stuck, the timeout reports sustained
no progress without misclassifying a transient scheduler state.
Scope depth maps one-to-one to rings until the deepest ring; deeper scopes
share that ring. For that ring,
scope_beginsselects the shallowest openscope, so
Ostill includes tasks retained by any open ancestor.Concurrency and ABA safety
scope_tasksandscope_beginshave one orchestrator writer, and the schedulerdoes not modify them. While blocked in allocation, that orchestrator cannot
close a scope or change
O. BecauseOremains scope-retained, its slot cannotbe consumed and reused. A concurrent reclaim-boundary read may therefore delay
detection when
H != O, but it cannot turnH == Ointo a false positive.The dependency-list path uses the new task's
slot_state.ring_id, while thescope helper uses
current_ring_id();prepare_task()establishes that theserefer to the same ring before live-fanin wiring can block.
Assumptions and unchanged concerns
The proof relies on existing runtime contracts: scope references are retained
until
scope_end, each scope stack has one orchestrator writer, and a live taskslot is not reused. Those contracts would need to be revisited if early scope
release or multiple orchestrator writers are introduced.
This change does not alter the existing
int32_ttask-ID lifetime limit,current_task_indexpublication ordering, or the allocator's possiblediagnostic-code overwrite by a concurrent fatal. None is used to establish
H == O.Regression coverage
Focused A2/A3 and A5 tests cover:
deepest ring;
structural fatal;
These cases distinguish oldest-open-task slot identity from the old
COMPLETED + consumers released + scope bit unsetapproximation.They use the normal scope-end and scheduler-consume lifecycle and add no
production test hooks or AICPU hot-path logging.
Validation
runs on both architectures.
git diff --check origin/main...HEADand modified-file pre-commit checkspassed.
Performance
Computing
Ois O(1): one ring lookup, onescope_beginsload, one boundscheck, and at most one
scope_tasksload. It adds no scan, lock, atomicoperation, or shared-memory layout change. The allocator compares slots once
per 1024 blocked spins; live-fanin computes
Oonce before the dependencypool's existing availability check.
An on-device ABBA benchmark compared the two validated commits:
a2a3.tests/st/a2a3/tensormap_and_ringbuffer/multi_round_paged_attention/test_multi_round_paged_attention.py,Case1,--skip-golden.per measurement.
19fca94a0ae0ccb86203173a1f03b8bfcf0bb846.The final commit only amends its message; both commits have source-tree hash
b04e4b339cc260f55414c3ac9c3e43cd657cfa4a.The paired deltas change direction between the two measurements, and the
combined device, effective, orchestrator, and scheduler deltas remain within
1%. The benchmark therefore shows no stable performance regression from this
change.