fix(scheduler): tag sync_start drain ack/election state by generation#1461
fix(scheduler): tag sync_start drain ack/election state by generation#1461Leaf-Salix 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:
📝 WalkthroughWalkthroughThe sync-start drain protocol now uses attempt-scoped acknowledgements and elections to reject stale retry state. Runtime wiring exposes a deterministic ABA test mode, with lifecycle resets, unit coverage, and an SPMD reproducer. ChangesSync-start drain protocol
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SchedulerThreads
participant DrainState
participant Runtime
SchedulerThreads->>DrainState: enter drain and advance drain_attempt
SchedulerThreads->>DrainState: publish per-thread acknowledgement
SchedulerThreads->>DrainState: elect matching-attempt worker
DrainState-->>SchedulerThreads: accept attempt or invalidate cohort
SchedulerThreads->>Runtime: finalize or retry sync-start drain
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10436722dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/host_build_graph/runtime/scheduler/scheduler_completion.cpp`:
- Around line 472-487: In SchedulerContext::enter_drain_mode, advance and
publish drain_attempt before clearing drain_worker_elected, matching the
ordering used by handle_drain_mode and the shared parent implementation. Keep
pending_task publication and the remaining coordination resets intact, but
ensure stale-round election checks cannot observe the new task before the
attempt changes.
In
`@src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cpp`:
- Around line 503-521: In SchedulerContext::enter_drain_mode, advance and
publish drain_attempt before clearing drain_worker_elected. Preserve the
existing reset sequence and pending_task publication, but ensure the new
generation is visible before election state is reset, matching the ordering
already used by handle_drain_mode.
In
`@src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cpp`:
- Around line 416-430: Reorder the initialization in
SchedulerContext::enter_drain_mode so drain_attempt is incremented and published
before clearing drain_worker_elected, matching the ordering used by
handle_drain_mode’s insufficient-resource path and the other scheduler variants.
Preserve the existing drain-state initialization and ownership logic.
In
`@tests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start_drain_aba_hook/test_spmd_sync_start_drain_aba_hook.py`:
- Line 19: Update the test setup around SIMPLER_DRAIN_ABA_TEST to assign "1"
unconditionally, ensuring the ABA hook is exercised regardless of the inherited
environment. Scope the override to this test and restore the prior environment
value during teardown, including removing the variable when it was previously
unset.
🪄 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: 88fb1af7-bff2-4d96-a640-1199859fa07a
📒 Files selected for processing (23)
docs/dynamic-linking.mdsrc/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_cold_path.cppsrc/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_completion.cppsrc/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_context.hsrc/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_types.hsrc/a2a3/runtime/tensormap_and_ringbuffer/common/pto_runtime_status.hsrc/a2a3/runtime/tensormap_and_ringbuffer/host/runtime_maker.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/runtime.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_context.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_types.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/shared/runtime.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_context.hsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_types.htests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start_drain_aba_hook/kernels/aic/kernel_spmd_mix_pressure.cpptests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start_drain_aba_hook/kernels/aiv/kernel_spmd_mix_pressure.cpptests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start_drain_aba_hook/kernels/orchestration/spmd_sync_start_drain_aba_hook_orch.cpptests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start_drain_aba_hook/test_spmd_sync_start_drain_aba_hook.pytests/ut/cpp/a2a3/test_scheduler_state.cpptests/ut/cpp/a5/test_scheduler_state.cpp
sync_start_pending / drain_worker_elected / drain_ack_mask carried no generation tag, so a scheduler thread that already cleared an old drain attempt's ack barrier could win the election CAS for a newer attempt reusing the same sentinel values (ABA). The stale thread then published stage_go and waited on stage_done from threads still accumulating acks for the new attempt, splitting the barrier and hanging until the AICPU op-execute timeout (507018). Add a drain_attempt counter bumped on every reset (new task or insufficient-resource retry), tag each thread's ack with the attempt it belongs to, and reject/self-revert an election win that lands after the attempt has moved on. Applies to all three drain protocol variants: a2a3/tensormap_and_ringbuffer, a2a3/host_build_graph, and a5/tensormap_and_ringbuffer. enter_drain_mode() had the same class of ordering bug at the new-task boundary: it cleared drain_worker_elected before bumping drain_attempt, reopening the ABA window for a thread delayed since the previous (already-finished) round. Reordered to bump drain_attempt first in all three variants, matching the ordering the insufficient-resource retry path already used. Add a deterministic reproduction: an env-gated test hook (SIMPLER_DRAIN_ABA_TEST=1, a2a3/tensormap_and_ringbuffer only) that parks a scheduler thread past its own ack barrier but before election, resumes only once a sibling's reset has advanced drain_attempt, and waits for peers to re-ack under the new attempt before falling through — recreating the stale-owner interleaving from the issue. The hook latches a dedicated error code and fails loudly if the forced race window is never observed, so a future change to the test's resource math can't silently turn it into a no-op pass. The env var is scoped to the test via an autouse monkeypatch fixture rather than a module-level os.environ.setdefault, so it can't leak into unrelated tests or be silently disabled by a preexisting value in the environment. New scene test: tests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start_drain_aba_hook. Fixes hw-native-sys#1455.
1043672 to
51dd422
Compare
Summary
sync_startdrain coordinates scheduler threads through a set of reusableshared fields (
sync_start_pending,drain_worker_elected,drain_ack_mask,drain_stage_go,drain_stage_done_mask). None of thesefields carried a generation/attempt tag. When the elected owner found
global resources insufficient, it reset the ack/election fields back to
their sentinel values so all threads could retry — but a scheduler thread
that had already cleared the old attempt's ack barrier, and was merely
delayed before performing its own election CAS, could resume after the
reset and win the CAS for the new attempt using stale state. That stale
thread would then publish
stage_goand wait onstage_donefrom peersthat were still accumulating acks for the new attempt, splitting the
barrier. Nothing in the protocol could detect or recover from this split,
so the run hung until the AICPU op-execute timeout fired
(
RuntimeError: run failed with code 507018), matching the report in#1455.
This is a textbook ABA problem: every individual field value is valid at
any instant, but the fields can be observed as belonging to two different
drain attempts by two different threads at the same time.
Fix
drain_attemptcounter (atomic<uint64_t>) toSyncStartDrainState,bumped every time the ack/election state is reset — both when a brand
new
sync_starttask enters drain and when the elected owner resets dueto insufficient resources.
drain_ack_maskbitmask with a per-threaddrain_ack_attempts_[]array: each thread stamps its ack with theattempt it belongs to. The "all acked" check
(
sync_start_drain_ack_mask_for_attempt) only counts acks whose stampedattempt matches the attempt currently in progress, so a late ack from an
old attempt can never satisfy a newer attempt's barrier.
sync_start_drain_try_elect: after winningthe CAS on
drain_worker_elected, it re-checksdrain_attempt. If theattempt has moved on since the caller captured it, the function
self-reverts the CAS (releases
drain_worker_electedback to 0) andreports "not elected", so a stale winner never gets to act as owner for
the wrong generation.
handle_drain_modenow also bails out as soon as it observes
drain_attempthas changed,instead of only relying on
is_completed()/drain_worker_elected == 0.Applied identically to all three drain-protocol implementations that share
this pattern:
a2a3/tensormap_and_ringbuffer,a2a3/host_build_graph, anda5/tensormap_and_ringbuffer.Deterministic reproduction
Ported the reproduction described in #1455 into an env-gated test hook
(
SIMPLER_DRAIN_ABA_TEST=1,a2a3/tensormap_and_ringbufferonly): once perdrain run, scheduler thread 1 parks right after its own ack barrier but
before its election attempt, resumes only once a sibling's
insufficient-resource reset has advanced
drain_attempt, and waits for theother threads to have re-acked under the new attempt before falling
through — recreating the exact stale-owner interleaving from the issue.
Against the pre-fix protocol this reliably reproduces the split-barrier
hang; against the fix,
sync_start_drain_try_electrejects the staleelection attempt and the run completes normally.
New, self-contained scene test:
tests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start_drain_aba_hook(one holder MIX task occupying a cluster, one
sync_startMIX task needingall of them — forces at least one insufficient-resource retry on the first
attempt, matching the issue's repro steps).
The hook also guards against silently proving nothing: if a full spin
budget passes without a sibling's reset ever advancing
drain_attempt,the race window was never exercised. Rather than falling through and
letting the scene test pass on the unraced happy path, the hook latches a
dedicated
PTO2_ERROR_DRAIN_ABA_TEST_NOT_EXERCISEDcode and fails the run— so a future change to the test's resource math that breaks the repro
fails loudly instead of quietly stopping to prove anything.
Documentation
Updated
docs/dynamic-linking.md's description ofSchedulerContext::deinit()'s reset responsibilities, which still namedthe now-removed
drain_ack_maskfield.Tests
Unit tests (host-side, no hardware)
New
SyncStartDrainAttemptTestsuite (tests/ut/cpp/{a2a3,a5}/test_scheduler_state.cpp)directly exercises the two new helpers:
LateAckCannotSatisfyNextAttemptBarrier— an ack stamped with an oldattempt is excluded from the current attempt's mask even after the
current attempt's other threads have acked.
StaleParticipantCannotOwnNextAttempt— a thread whose captured attemptno longer matches the live
drain_attemptfails to elect, and does notleave
drain_worker_electedin a won state.Verified with a from-scratch clone of the pushed branch (independent of the
working checkout that produced it) and a fresh CMake configure/build:
60/60 no-hardware C++ unit tests pass (
ctest -LE requires_hardware),including the full pre-existing
test_scheduler_state/test_a5_scheduler_state/
test_trb_runtime_temp_buffersuites — no regressions.tests/ut/cpp/CMakeLists.txt.Hardware A/B test (real a2a3 silicon, Ascend 910-series chip)
Ran the new
spmd_sync_start_drain_aba_hookscene test withSIMPLER_DRAIN_ABA_TEST=1against two builds, each installed from a cleaneditable install with the onboard toolchain on
PATH, with the onboardbuild cache cleared before each install and verified by
strings-checkingthe compiled
libaicpu_kernel.sofor the expected symbols before running(to rule out a stale/cached binary from an unrelated install):
upstream/mainatd0bc661a, unmodified) + the same ABA-forcing hookRuntimeError: run failed with code 507018after ~62s (most recent run: 61.90s), matching #1455Re-confirmed both sides freshly (clean onboard rebuild + rerun) after the
final round of changes to this branch, with
upstream/mainre-checked viagit ls-remoteimmediately beforehand to confirm the baseline commit(
d0bc661a) had not moved since this branch was created — the failingand passing results above are current, not carried over from an earlier,
possibly stale build.
Environment:
--platform=a2a3on real device (not thea2a3simsimulator),single-device execution, CANN toolkit
cann-9.0.0, Python 3.10.9,pytest 9.0.3 (plugins:
pytest-forked1.6.0,anyio4.12.1,pytest-xdist3.8.0), run with
-s -v --forked.Base commit for this branch:
upstream/mainatd0bc661a(hw-native-sys/simpler).Related Issue
Fixes hw-native-sys/simpler#1455