Fix: keep the host cancel ordered after the handshake clear - #2280
Open
Leaf-Salix wants to merge 4 commits into
Open
Leaf-Salix wants to merge 4 commits into
Leaf-Salix wants to merge 4 commits into
Conversation
The cancel fills TmrLaunchControl::host_cancel, which lies inside the region the handshake clear zeroes. With the clear on the context's AICPU stream and the cancel on the caller stream, nothing ordered the two: the clear could land after the cancel, reset the word to zero, and leave AICore spinning in its pre-window poll until the op-execute timeout. Issue the cancel on the AICPU stream, behind the clear, so stream FIFO orders the cancel after it. The cancel also cannot overtake a submitted AICPU task on that stream, which makes "cancel only before AICPU submission" structural rather than positional. The native binder UT pinned the old stream; it now pins both memsets to the AICPU stream, which with the existing call-order assertion covers clear-then-cancel.
…tream The compensating cancel and the handshake clear write the same control word, so the fix issues both on the AICPU stream and relies on that stream's FIFO to order them. Two tests now hold that in place. The native binder UT records the stream of every memset and requires them all to ride `handles.aicpu`. It carries the no_hardware label, so the guard is present even where the ASCEND-gated native UT is not built, and it fails on the caller-stream cancel at both compensation sites. The compensation trace assertion already pins the call order, so stream plus order together cover clear-then-cancel. The a2a3 kernel-mode capture ST gains a launch_fail_compensation scenario: it fails the launch's own AICPU enqueue, with AICore already launched and polling, and requires the ladder to finish. It now also asserts the ladder's own progress — three memsets (two clear regions plus the cancel), five event records and four event waits — so a ladder that bails out at its first step cannot pass on the caller's silent sync alone.
…ure ST The compensation ladder's cancel and the handshake clear write the same control word, and nothing orders them when they ride different streams. On hardware that interleaving has never been observed: the clear's stream has usually drained by the time the cancel is enqueued, so the hazard stays latent and the scenario cannot fail on it. Arming CAPTURE_OBSERVER_REORDER_CANCEL_CLEAR makes the interleaving deterministic instead of hoping for it: after a cancel issued on a different stream than the clear, the observer puts a zero fill back on top, on the cancel's own stream so FIFO places it after. Same-stream code is left alone, so the switch reproduces the hazard rather than the outcome of one particular build — an unfixed caller stream cancel hangs AICore, the fixed AICPU stream cancel is untouched. The emulated fill goes through the resolved symbol rather than this interception, so async_clears still counts only what the sequence itself issued. Diagnostic only: no scenario arms this, and it is dropped again before the PR.
…e ST The switch is a diagnostic, not a contract. It exists to show the hazard is real and that the fix removes it, not to run in CI, so it does not ship. Armed on a2a3, with the ST unchanged: cancel on the AICPU stream (fixed) -> 5/5 pass cancel on the caller stream (unfixed) -> 5/5 fail, caller sync times out at 10s The unfixed failure is the ladder never completing: AICore stays in its pre-window poll because the emulated late clear reset the word it waits on. This is the failure mode N2-new-2 describes, made deterministic rather than hoped for. Note what the pair also shows: unarmed, the unfixed build passes the scenario 10/10, so the scenario alone cannot catch this hazard and the deterministic guard belongs in the unit test that pins both memsets to the AICPU stream.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
4 tasks
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
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.
Motivation
This closes N2-new-2 from the unified issue list (HIGH, introduced this round):
The compensating cancel and the handshake clear write the same four bytes:
TmrLaunchControl::host_cancelsits at offset 0 — asserted bysrc/common/task_interface/tmr_kernel_control.h:72.build_tmr_kernel_clear_plansetsregions = {control, reports}andcancel = {control.address + offsetof(TmrLaunchControl, host_cancel), 4}atsrc/common/tensormap_and_ringbuffer/kernel_clear_plan.h:63-64. The cancel word is inside the region the clear zeroes.Two writers, one word — and with the clear on the AICPU stream and the cancel on the caller stream, nothing ordered them against each other. When the clear lands last,
host_cancelreads zero and AICore never leaves its pre-window poll (src/a2a3/runtime/tensormap_and_ringbuffer/aicore/aicore_executor.cpp:333; A5 atsrc/a5/runtime/tensormap_and_ringbuffer/aicore/aicore_executor.cpp:313). The caller'swait(AicpuDone)then never fires, and the round is released only when STARS reaps the op at the op-execute timeout — 45 s,PLATFORM_OP_EXECUTE_TIMEOUT_USatsrc/a2a3/platform/include/common/platform_config.h:77.The two writes, and the stream each rides
Both writes are
aclrtMemsetAsynccalls, and the handle passed in is the stream they land on —kernel_launch_owner.cpp:103-116forwards that argument straight to the driver. Three handles are in play:h.callerh.aicpuKernelStreamKind::Aicpu) — inside the runtimeh.aicore0)0xff)0d55fac7h.aicpu(:51)h.caller(:40)84c44df5h.aicpu(:55)h.aicpu(:44)The clear did not move. It has always been on the AICPU stream and this PR leaves it there; the only thing that moved is the cancel, from the caller's stream onto the AICPU stream. That direction is deliberate: the clear must complete before the AICPU task reads the control block, and the AICPU task is enqueued on that same stream, so the clear is already where it has to be. Moving the cancel in costs nothing else; moving the clear out would have needed a new cross-stream edge to keep the same guarantee.
So the whole change in one line each way:
Where the split came from
5b2a5a5cAdd: integrate kernel-mode PRs with an end-to-end launch test (#2216)h.callerh.caller29a1cd40Refresh the kernel-mode integration line ... (#2241)h.aicpuh.caller52d6f468(this PR)h.aicpuh.aicpu#2241moved the clear (and AicoreStart) to the AICPU stream and left the cancel behind, splitting two writers of one word across two streams. Before it, both rode the caller stream and FIFO ordered them, so the hazard is a regression from that refactor rather than an original design choice. This PR restores the single-stream property, on the AICPU branch where the clear now lives.The fix
One argument changes, at
src/common/platform/onboard/host/kernel_launch_sequence.h:44. The clear line shown below is unchanged context — it is quoted so the reader can see that only the cancel moved:auto compensate = [&](bool retry_core_done) { auto cleanup = [&](int rc) { ... }; + // Cancellation and the handshake clear write the same control word, so + // they must share a stream: FIFO is what orders the cancel after the + // clear. Across two streams the clear can land last, reset the cancel, + // and leave AICore spinning before its register window opens. - if (!cleanup(ops.cancel_waiting_aicore(ops.context, h.caller))) return; // :40, caller stream + if (!cleanup(ops.cancel_waiting_aicore(ops.context, h.aicpu))) return; // :44, AICPU stream ... }; // unchanged, earlier in the same function: if (!step(KernelLaunchStep::Clear, ops.memset_handshake(ops.context, h.aicpu))) return result; // :51 -> :55The handle argument is the memset's target stream, with no layer in between:
Why this shape of fix rather than another:
#2249established that a launch must not synchronize internally, so a host-side sync on the AICPU stream is not an option.kernel_launch_sequence.h:57-61. Recording an event after the clear and waiting it on the caller would break that.kernel_launch_sequence.h:65-72), so this changes nothing today but survives a future refactor.collect_reportstreats a non-zerohost_cancelas an abort signal (src/common/tensormap_and_ringbuffer/kernel_core_group.h:68,78). The issue text does not mention this reader, but the same ordering is what keeps it correct.host_cancelout of the cleared region. It still has to be reset between rounds, and that reset is itself an ordering requirement, so the change would relocate the problem rather than remove it.Two files are changed in the same commit because both pinned the old behaviour:
tests/ut/cpp/common/test_kernel_launch_native.cpp:80-82— the expectation and its comment named the caller stream for the cancel.docs/kernel-launch-binder.md:87— the published sentence justified a caller-stream cancel with "the caller's stream … carries nothing but Start at that point", which is precisely the premise that produced the race.Tests
Coverage added or extended
tests/ut/cpp/common/kernel_binder_test_support.h+test_kernel_launch_binder.cpp:162-163— the fake records the stream of every memset and the enqueue-failure test requires all of them to ridehandles.aicpu. This is the deterministic guard: it fails on a caller-stream cancel at both compensation sites (fail == 7, the AicoreDone-retry path;fail == 8, the AICPU-launch path) and it carries theno_hardwarelabel, so it runs with no device and in any CI. It closes a real gap: the only test that asserted the memset stream was the native UT, which is gated behindSIMPLER_ENABLE_HARDWARE_TESTSand therefore absent from a default build.tests/st/a2a3/tensormap_and_ringbuffer/kernel_mode_capture/test_kernel_mode_capture.py:354-362— thelaunch_fail_compensationscenario now also asserts the ladder's own progress: three memsets (two clear regions plus the cancel), five event records, four event waits. Previously "the caller's sync returned" was the only assertion, and a ladder that bailed out at its first step would still sync clean, because the caller stream only has to carry Start in order to drain.Executed results
test_kernel_launch_binder, macOS, no deviceh.callerrestored)SCOPED_TRACE 8, i.e. the AICPU-launch compensation site. 0.3 stest_kernel_launch_binder, macOS, no devicelaunch_fail_compensation, 10 runslaunch_fail_compensation, 10 runslaunch_fail_compensation, 5 runslaunch_fail_compensation, 5 runslaunch_fail_compensation, 3 runs, committed tree84c44df5Environment: Ascend
a2a3onboard runtime, CANN 9.0.0 (/usr/local/Ascend/cann-9.0.0), exclusive single-devicetask-submitjobs, checkout-local build environment. For the unfixed rows the same tree was rebuilt withh.callerrestored (runtime.sosha25612b35607…, versusb686bb14…fixed); the unfixed build reproduced the same digest across two independent builds.In the whole-file run the environment's resource scheduler dispatched the 30 cases as 30 separate child jobs, each reporting its own
[PASS …]; the outer pytest session therefore ends withno tests ran, which is expected for that dispatch and not a deselection. Per-scenario results were collected from the child job markers: 30 dispatched, 30 passed, 0 failed.The unfixed failure is the intended mechanism, taken from the child process log:
The caller's bounded sync expires because the ladder never completes: AICore is still spinning, so AicpuDone never reaches the caller.
The reproducer pair:
34202626(armed, measured) →84c44df5(reverted)The cross-stream interleaving is latent, not merely rare. Unarmed, the unfixed build passes the scenario 10/10, because by the time the host enqueues the cancel the clear's stream has usually drained. A test that cannot fail on the defect cannot demonstrate it — so commit
34202626adds a switch that makes the interleaving deterministic, measures the defect with it, and commit84c44df5removes the switch again so that nothing of it ships.34202626—Add: make the cross-stream cancel/clear race reproducible in the capture STThe switch lives in the capture observer and is armed by
CAPTURE_OBSERVER_REORDER_CANCEL_CLEAR:It therefore reproduces the hazard — "two writers of one word sit on different streams, and the clear lands last" — rather than the outcome of one particular build. Same-stream code is never touched, which is exactly why the fixed build passes with it armed while the unfixed build fails. The emulated fill goes through the resolved symbol rather than through the interception, so
async_clearsstill counts only what the sequence itself issued, and the scenario itself is unchanged by the switch (thelaunch_fail_compensationassertions are identical in both trees).What
34202626established on a2a3 — two 5-run batches, scenario unchanged:Against the unfixed tree (
h.callerrestored and the runtime rebuilt):and the failure is the mechanism the issue describes, taken verbatim from the child process log:
That is N2-new-2 reproduced on demand instead of hoped for. The emulated late clear resets
host_cancelto zero, so AICore never leaves its pre-window poll; the compensating ladder'swait(AicoreDone)therefore never fires,AicpuDoneis never recorded, and the caller — whose only path to a tail isAicpuDone— cannot drain. Its bounded 10 s sync expires, and without the switch the same round would have been released only by STARS at the 45 s op-execute timeout.Against the fixed tree the switch is inert — cancel and clear share the AICPU stream, so its second condition never fires:
Read together: the defect is real (
34202626+ unfixed, 5/5 fail, at the failure point the issue predicts) and the fix removes it (34202626+ fixed, 5/5 pass). Without the fix, the same three numbers are 0/5 and 5/5 the other way.84c44df5—Revert: drop the cross-stream cancel/clear reproducer from the capture STRemoves the switch so that the merged content carries no injector; its message records the same measurements. It is reverted rather than kept because it is a fault injector that nobody arms and that only reproduces an interleaving the fixed code cannot produce — shipping it would put a switch into CI that is red by construction. The property actually worth guarding, both memsets on one stream, is pinned deterministically by the unit test above, which needs no device. Keeping the two commits in the history is the point; shipping the mechanism is not.
The tree at
84c44df5is byte-identical to the tree ata35ad53f(git diff --stat a35ad53f 84c44df5is empty).Reproduction
Not validated:
launch_fail_compensationwas repeated (10 + 5 + 5 + 3 runs) and only it was used for the unfixed/reproducer comparisons. The other 29 scenarios are single-run evidence, not stability evidence.src/a5/platform/onboard/host/CMakeLists.txt:93compileskernel_launch_native.cppas well — so the change applies to A5, but only A2/A3 silicon was used here.compensate(false)path the cancel is now enqueued on the same stream as thelaunch_aicputhat just failed. If a real CANN enqueue failure leaves that stream in an error state, a later async operation on it might not execute, and the cancel would be lost. The scenario's fault injection returns before calling the real API (kernel_capture_observer.cpp,fail_prepare_step), so it cannot rule this out; verifying it needs an injection that makes the real call fail, which this PR deliberately does not add.Commits
52d6f468—Fix: keep the host cancel ordered after the handshake cleara35ad53f—Add: cover the compensation ladder and pin its memsets to the AICPU stream34202626—Add: make the cross-stream cancel/clear race reproducible in the capture ST(diagnostic; reverted by the next commit)84c44df5—Revert: drop the cross-stream cancel/clear reproducer from the capture STBased on
0d55fac7, the current tip offeat/kernel-mode-integration-test.中文总结
host_cancel位于TmrLaunchControloffset 0;该字落在清零区间内),而#2241把握手清零挪到 AICPU 流却把 cancel 留在 caller 流,两个写方跨流无序 —— clear 后落地就把 cancel 抹成 0,AICore 卡在 pre-window 轮询直到 op-execute 超时(45 s)。0)与"取消"(写0xff)。h.caller= 调用方传进来的那条流(域外);h.aicpu= context 自有的隐藏 AICPU 流(域内);h.aicore同类、本次不涉及。0d55fac7:清零在h.aicpu(:51)、取消在h.caller(:40)—— 两条流之间没有任何边,先后无保证;84c44df5:清零仍在h.aicpu(:55)、取消归到h.aicpu(:44)—— 同一条流,驱动按 FIFO 执行,清零严格先于取消。kernel_launch_sequence.h:44),把补偿期 cancel 也放到 AICPU 流,靠同流 FIFO 定序。同一提交内对齐了 UT 断言与那份已发布文档(文档原来用"caller 流此刻只有 Start"来论证旧行为,而这正是竞态的前提)。选它的理由:launch 内部不允许同步(#2249),事件方案会在 caller 流上加边、破坏 capture 的 caller→AICPU→AICore 传播;同流后 cancel 无法越过已提交的 AICPU 任务,把"仅在 AICPU 提交前取消"从位置约定升级为结构约束;顺带保护了 AICPU 侧collect_reports这个第二读者。handles.aicpu;旧代码在两个补偿点都必红,且该 target 带no_hardware标签 —— 补上了默认构建里(ASCEND 门控的 native UT 之外)没有任何测试锁这条流的缺口。launch_fail_compensation场景本身未改动):34202626(加复现开关)验证了旧版问题确实存在:把那一行回退成h.caller并重建 runtime 后,5/5 失败(每次约 19.5 s),失败点经子进程日志核实为assert context.lib.aclrtSynchronizeStreamWithTimeout(context.caller, 10000) == 0→AssertionError。失败语义:被人为后置的清零把
host_cancel抹回 0 → AICore 停在 pre-window 轮询 →AicoreDone不触发 →AicpuDone不记录 → caller(唯一通往 tail 的路是AicpuDone)拿不到 tail → 10 秒有界同步超时。这正是 issue 描述的"AICore 永久自旋",从"偶发"变成"按需复现"。84c44df5回退掉这个复现开关:git diff --stat a35ad53f 84c44df5为空,即最终合并内容里不含任何注入器。删而不是留的理由:它靠"人为把字写回 0"制造交错,等于强行让修复失效 —— 演示的是危险交错存在,不是产品会进入它;留着等于往 CI 里放一个必红且无人武装的故障注入器。提交 3/4 成对保留在历史里,正是为了让 reviewer 能自己复现、也能看到它被移除。launch_fail_compensation做了重复与旧版对照(其余 29 条是单次证据、不是稳定性证据);A5 真机未跑(host 序列两架构共用);compensate(false)的 cancel 现在与刚失败的 AICPU launch 同流,若真实 enqueue 失败污染该流,cancel 可能丢失 —— 现有注入在调用真实 API 之前就返回,无法排除,需要"让真实 API 失败"的注入才能验证,本 PR 有意不加。