Fix: avoid remote teardown after fatal AICore errors - #1664
Fix: avoid remote teardown after fatal AICore errors#1664sunkaixuan2018 wants to merge 1 commit into
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:
📝 WalkthroughWalkthroughChangesThe PR adds stop-on-failure stream creation and centralizes stream setup. It adds non-blocking AICore exit signaling and guards repeated emergency shutdown. Fatal device teardown now retries force reset, abandons host-side resource handles, and skips unsafe per-resource runtime destruction. Fatal device recovery and stream lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DeviceRunner
participant Scheduler
participant AICore
participant DeviceResources
DeviceRunner->>Scheduler: begin emergency shutdown
Scheduler->>AICore: write exit signal without acknowledgement wait
DeviceRunner->>DeviceRunner: wait for device-down handoff
DeviceRunner->>DeviceResources: force reset and confirm recovery
DeviceRunner->>DeviceResources: abandon invalidated handles
DeviceRunner->>DeviceRunner: clear unusable device state
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/common/platform/onboard/host/device_runner_base.cpp (2)
116-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated stop-on-failure stream creation logic.
Both functions perform the same three steps:
rtStreamCreate,aclrtSetStreamFailureMode(..., ACL_STOP_ON_FAILURE), and destroy-and-clear the handle on configuration failure. Keeping two copies risks the two diverging on a future fix (for example, an added retry or a changed failure mode).
src/common/platform/onboard/host/device_runner_base.cpp#L116-L133: keepcreate_stop_on_failure_stream, but change its signature to takevoid **stream(or a small template) soDeviceRunner::create_run_streamcan call it directly instead of reimplementing the same three steps.src/a2a3/platform/onboard/host/device_runner.cpp#L531-L552: replace the body ofcreate_run_streamwith a call to the shared helper fromdevice_runner_base.cpp, passing a"run"name for logging.🤖 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/common/platform/onboard/host/device_runner_base.cpp` around lines 116 - 133, Consolidate the duplicated stream setup by updating create_stop_on_failure_stream in src/common/platform/onboard/host/device_runner_base.cpp:116-133 to accept void **stream (or an equivalent small template) while preserving creation, ACL_STOP_ON_FAILURE configuration, and cleanup behavior. Replace DeviceRunner::create_run_stream in src/a2a3/platform/onboard/host/device_runner.cpp:531-552 with a direct call to the shared helper using the "run" log name.
1174-1252: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFatal-teardown correctness relies on an unenforced call order.
In the abandon path,
mem_alloc_.free()(Line 1200),free_tensor()(Line 1247), andmem_alloc_.finalize()(Line 1252) run unconditionally. They only avoid touching device memory becausemem_alloc_.abandon_after_device_failure()(Line 1183) already clearedptr_size_map_earlier in the same function. Nothing in this loop or inMemoryAllocatorprevents a later refactor from reordering these calls, which would silently reintroduce realrtFree()calls against a poisoned/reset device — the exact hang this PR removes.Guard these calls explicitly with
abandon_device_resources, the same way the stream and DMA-workspace releases above are already guarded, so the abandon behavior does not depend on call order.🛡️ Proposed fix to make the abandon path order-independent
for (auto &kv : chip_callable_buffers_) { - mem_alloc_.free(reinterpret_cast<void *>(kv.second.chip_dev)); + if (!abandon_device_resources) { + mem_alloc_.free(reinterpret_cast<void *>(kv.second.chip_dev)); + } if (!abandon_device_resources) { LOG_DEBUG( "Freed chip callable buffer: chip_dev=0x%lx, size=%zu, hash=0x%lx", kv.second.chip_dev, kv.second.total_size, kv.first ); } } @@ if (device_wall_dev_ptr_ != nullptr) { - free_tensor(device_wall_dev_ptr_); + if (!abandon_device_resources) { + free_tensor(device_wall_dev_ptr_); + } device_wall_dev_ptr_ = nullptr; } - - // Free all remaining allocations (including handshake buffer and binGmAddr) - mem_alloc_.finalize(); + // Free all remaining allocations (including handshake buffer and binGmAddr). + // Skipped in the abandon path: ptr_size_map_ was already cleared above. + if (!abandon_device_resources) { + mem_alloc_.finalize(); + }🤖 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/common/platform/onboard/host/device_runner_base.cpp` around lines 1174 - 1252, Guard the device-memory cleanup calls in the teardown flow with abandon_device_resources: skip chip buffer mem_alloc_.free(), device_wall free_tensor(), and mem_alloc_.finalize() when abandoning device resources, while retaining them for normal teardown. Update the related cleanup loops around chip_callable_buffers_ and device_wall_dev_ptr_ so fatal teardown cannot issue device frees regardless of call order.src/a2a3/platform/onboard/host/device_runner.cpp (1)
1086-1119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead code: this poisoned-device retry block is unreachable.
The new fatal branch at Lines 959-1029 always returns whenever
device_unusable_is true at function entry, since nothing in this function setsdevice_unusable_back totruebetween the two checks (it is only mutated byrecover_device_or_mark_unusable(), called fromrun()). So theif (device_unusable_) { ... }block at Lines 1087-1119 — which retriesforce_reset_device()up tokMaxResetAttemptstimes — can never execute; it duplicates the retry loop already added at Lines 984-999.Remove this block (and its now-always-true
reset_rc == 0follow-up at Lines 1126-1128 can be simplified accordingly), so a future reader does not assume this retry path still runs.🤖 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/platform/onboard/host/device_runner.cpp` around lines 1086 - 1119, Remove the unreachable device_unusable_ force-reset retry block from finalize, including its kMaxResetAttempts loop and related logging. Simplify the subsequent reset_rc == 0 follow-up because the retry is already handled by the fatal branch earlier in finalize; preserve the existing recovery behavior from that branch.
🤖 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/platform/onboard/host/device_runner.cpp`:
- Around line 1086-1119: Remove the unreachable device_unusable_ force-reset
retry block from finalize, including its kMaxResetAttempts loop and related
logging. Simplify the subsequent reset_rc == 0 follow-up because the retry is
already handled by the fatal branch earlier in finalize; preserve the existing
recovery behavior from that branch.
In `@src/common/platform/onboard/host/device_runner_base.cpp`:
- Around line 116-133: Consolidate the duplicated stream setup by updating
create_stop_on_failure_stream in
src/common/platform/onboard/host/device_runner_base.cpp:116-133 to accept void
**stream (or an equivalent small template) while preserving creation,
ACL_STOP_ON_FAILURE configuration, and cleanup behavior. Replace
DeviceRunner::create_run_stream in
src/a2a3/platform/onboard/host/device_runner.cpp:531-552 with a direct call to
the shared helper using the "run" log name.
- Around line 1174-1252: Guard the device-memory cleanup calls in the teardown
flow with abandon_device_resources: skip chip buffer mem_alloc_.free(),
device_wall free_tensor(), and mem_alloc_.finalize() when abandoning device
resources, while retaining them for normal teardown. Update the related cleanup
loops around chip_callable_buffers_ and device_wall_dev_ptr_ so fatal teardown
cannot issue device frees regardless of call order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1779fe14-fdfc-43ca-9328-dda944db23ee
📒 Files selected for processing (14)
src/a2a3/platform/include/aicpu/platform_regs.hsrc/a2a3/platform/onboard/host/device_runner.cppsrc/a2a3/platform/onboard/host/device_runner.hsrc/a2a3/platform/shared/aicpu/platform_regs.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_context.hsrc/common/aicpu_loader/host/load_aicpu_op.cppsrc/common/aicpu_loader/host/load_aicpu_op.hsrc/common/platform/include/host/memory_allocator.hsrc/common/platform/include/host/run_stream_slots.hsrc/common/platform/onboard/host/device_runner_base.cppsrc/common/platform/onboard/host/device_runner_base.hsrc/common/platform/onboard/host/device_runner_helpers.htests/ut/cpp/hierarchical/test_run_stream_slots.cpp
aab86b1 to
ac6781f
Compare
审查意见 —— 合并前请处理整体方向、机制与测试覆盖都很扎实,投资记录(investigation)写得尤其好。合并前有四点需要澄清: 1. 2. 普通(非 SDMA)中毒路径的强制复位从最多 3 次降为 1 次——请确认对普通分支是刻意的。 记录在案的单次复位理由(重试会叠加 ~300s remote-event 超时)是 SDMA 专属的;普通故障下 3. 针对 #1425 的核心回归测试 4. a2a3/a5 双树不对称。 a2a3 把 次要(可考虑): |
ac6781f to
cfd17c4
Compare
|
@ChaoZheng109 感谢细致的审查 —— 第 2 点直接命中了本 PR 的一个真实回归,CI 也独立印证了它。逐条回复: 1. 2. 普通路径复位 3→1 是回归,已修复 —— 并且它正是本次 CI 失败的原因。 不是刻意权衡,是我漏掉的。你指出的"a2a3 就地恢复变弱"在这次
即:单次复位未能确认干净的卡,把毒化状态留给了下一个进程。 修复按你的建议执行:非 SDMA 分支恢复 3 次有界重试(a2a3 与 a5 同步),仅 SDMA 分支保留单次。理由现在写在代码与 investigation 里: 3. 按 marker 选择且覆盖 4. a5 原子化已补齐;
次要项: |
|
@coderabbitai Thanks — all three nitpicks were reviewed against
Separately, the reset budget did change in the latest push, in the opposite direction from that dead-code removal: ordinary (non-SDMA) poison now keeps a bounded three-attempt budget via |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== stop-on-failure configuration sites =='
rg -n -C 4 'aclrtSetStreamFailureMode|ACL_STOP_ON_FAILURE|create_stop_on_failure_stream' \
src/a2a3/platform/onboard/host/device_runner.cpp \
src/common/platform/onboard/host/device_runner_base.cpp \
src/common/platform/onboard/host/device_runner_base.h || true
printf '%s\n' '== abandon-device-resource guards =='
rg -n -C 8 'finalize_common_impl|abandon_device_resources|chip_callable_buffers_|device_wall_dev_ptr_|mem_alloc_\.finalize' \
src/common/platform/onboard/host/device_runner_base.cpp
printf '%s\n' '== reset attempt symbols and call sites =='
rg -n -C 6 'kMaxResetAttempts|attempt_fatal_reset|force_reset_device|sdma' \
src/a2a3/platform/onboard/host/device_runner.cpp \
src/a2a3/platform/onboard/host/device_runner.h \
src/a5/platform/onboard/host/device_runner.cpp \
src/a5/platform/onboard/host/device_runner.h 2>/dev/null || trueLength of output: 58058
These three findings are resolved. 🐇 ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
164ad18 to
cdfba86
Compare
|
Follow-up: the first fix was necessary but not sufficient. Second root cause found and fixed. Where it stood. Restoring the three-attempt reset budget took What the remaining 3 were. The surviving failures were on devices whose preceding test was a pool/allocator-deadlock fault injection (
The actual cause. This PR made emergency shutdown fire-and-forget. The control that isolates it: The fix. Emergency shutdown now signals every handshake'd core first, then joins them — rather than signal-and-wait one core at a time, which is what the original serial Worth being explicit that this does not reintroduce the #1425 hang: the acknowledgement is an on-device poll of the core's The |
447fac5 to
07c8032
Compare
A fatal AICore error left teardown walking device resources that the poisoned card could no longer retire, so Worker.close() blocked in the driver's remote-event timeout. Fatal teardown now stops host collectors, force-resets the card, and then forgets the failed generation's handles without per-resource destroy, free, or unregister calls. cleanup_active_run() takes the same branch, so an enqueue rollback or drain on a poisoned card drops host-side ownership instead of issuing frees and stream destroys the driver cannot retire. Healthy teardown keeps the full release path. The reset budget follows the stream population. force_reset_device() drains before it resets and returns 0 only when its post-reset probe confirms the card, so a second pass runs against a settled card and can recover a poison the first could not; ordinary poison therefore keeps a bounded three-attempt budget on both a2a3 and a5. A Worker holding the 48 CP-process SDMA streams gets a single attempt, because there a non-confirming reset has already blocked on the driver's 150/300-second remote-event timeout and a retry only multiplies that wait. Collapsing both populations onto one attempt leaves a fault-injected card poisoned for the next process that lands on it. Scheduler threads publish fatal state before completion, so no thread enters the healthy per-thread shutdown path for a fatal run and races the emergency broadcast for the same cores, and one thread owns that broadcast. Emergency shutdown signals every handshake'd core before joining any of them, instead of signalling and waiting one core at a time, and the join takes one deadline for the whole group rather than one timeout per core. The join is load-bearing: returning while cores still run leaves the card poisoned past the host's device reset, so the next process on that device fails at launch. The shared deadline is what keeps it affordable — the onboard deinit timeout is 1 second, so a per-core deadline would cost a second per unresponsive core on a fatal run where every core is dead. It is an on-device register poll, so it adds no host or remote operation. ACL_STOP_ON_FAILURE applies only to an a2a3 run stream whose Worker actually provisioned the SDMA workspace. Ordinary a2a3 and all a5 streams keep their existing error and diagnostic contract, and take no added handoff delay. Mitigates hw-native-sys#1425. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
07c8032 to
90eafb5
Compare
|
CI is fully green — all 18 checks pass, including both hardware jobs (
The last run before this one finished 12-green / 3-red, where all three red jobs failed on infrastructure rather than code — The branch has also been rebased onto current
Still open for your call, unchanged from the earlier comment: whether to fold the emergency-shutdown fix into the three sibling scheduler trees ( |
2 participants
Summary
ACL_STOP_ON_FAILUREonly to the a2a3 per-run stream when a real SDMA workspace is provisioned.DEV_RUNNING_DOWN.The a2a3 SDMA fatal path waits
max(10 seconds, configured op-execute timeout + 5 seconds)before reset. This is an empirical workaround for CANN 9.0.0 and driver 26.0.rc1, not a portable runtime completion fence; it is deliberately limited to workers that actually provision the SDMA workspace.Reset budget follows the stream population
force_reset_device()drains the card before resetting it and returns 0 only when its post-reset probe confirms a usable generation, so a second pass runs against a settled card and can recover a poison the first could not. Ordinary poison therefore keeps a bounded three-attempt budget on both a2a3 and a5.A Worker holding the 48 CP-process SDMA streams is the exception and gets a single attempt: there a non-confirming reset has already blocked on the driver's 150/300-second remote-event timeout, so a retry multiplies that wait without adding a completion condition.
Testing
host_build_graphandtensormap_and_ringbufferruntimes with CANN 9.0.0.507015error contract;Worker.close().sub_class=S1) and tensor wait timeout (orch_error_code=8).st-onboard-a5CI job; local myserver hardware is a2a3, while the a5 runtime build and a5 C++ unit suite passed there.Scope
Mitigates #1425 — this bounds fatal teardown to roughly 10–30 s and keeps ordinary Workers on the fast path. It does not restore SDMA-Worker fault recovery to ~0.3 s: the root cause (CANN exposes no retirement fence for CP-process SDMA streams) is unresolved and remains deferred pending a CANN runtime-and-driver fix, so #1425 should stay open against that dependency.