Add: kernel-mode C ABI skeleton and wire headers (K1) - #2064
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: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (29)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds kernel-mode runtime contracts, lifecycle state management, validating host-runtime stubs, dynamic symbol loading, and tests. It also adds scalar-count metadata to callable artifacts and exposes it through C++ and Python APIs. ChangesKernel runtime lifecycle
Callable scalar metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The kernel-mode foundation preserves existing program-mode behavior, validates unsupported runtime paths, and maintains callable ABI compatibility. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 126 functions across 22 files. (7 skipped: 7 unsupported.) 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. A rabbit reads each line, Comment |
e52b52a to
a2e1222
Compare
sunkaixuan2018
changed the title
8f30a55 to
21fb15c
Compare
ChaoWao
left a comment
ChaoWao
left a comment
There was a problem hiding this comment.
Review: K1 kernel-mode ABI gate
Reviewed at a67dfc4 against merge-base d79c88c. CI is 20/20 green including both
self-hosted pools and both OSes, and the PR body's own numbers check out — I verified
the test counts (20 / 20 / 3), the scalar_count_ padding arithmetic (config_name_len_
ends at 9364, round_up(9368, 16) is still 9376, so sizeof(ChipCallable) is unchanged),
and the kernel-cache ABI-token invariance (_chip_callable_abi_token() in
scene_test_cache.py:52 SHA-256s a callable built with the default scalar_count=0, so
the bytes are identical). Nothing below is a "this is broken" finding. They are design
comments, and A1/A2 are the two I would want settled before the surface is frozen, since
freezing is the whole point of the PR.
Naming checks out against .claude/rules/codestyle.md: the simpler_kernel_mode_* prefix
matches the simpler_init / simpler_run family, kernel is used in its established
repo sense (an entity submitted to a stream through the rtKernelLaunch family —
launch_aicpu_kernel → rtsLaunchCpuKernel, launch_aicore_kernel →
rtKernelLaunchWithHandleV2), the new PTO_RUNTIME_ERR_* enumerators correctly reuse the
existing enum's prefix, no new PTO2 spelling, #pragma once, enum class, wire-POD
guards, and the gm_heap_bytes / gm_sm_bytes / runtime_arena_bytes field names line up
with setup_static_arena's parameters. Leaving the new classes out of a namespace matches
the local style of that directory (MemoryAllocator, RunStreamPair).
The state machine in kernel_execution_state.{h,cpp} is the part I liked most: the
sticky-retriable Closing with per-handle nulling so a retry redoes only the remainder, the
two separate error slots so a controlled poison cannot mask a teardown failure, and a
destructor that makes no runtime calls at all because an ACLGraph may still reference the
handles. That reads like it was designed from the failure cases backwards.
A1 — simpler_kernel_mode_ctx_control may not need to exist at all
This is a level above the earlier review round on FREEZE's ordering (which the body records
as resolved by requiring a kernel-mode CONFIGURE). The question here is whether the
capacity-freeze mechanism is needed, not whether its preconditions are ordered correctly.
The PR already guarantees what FREEZE protects. From simpler_kernel_mode_init's own
doc comment:
configis context-static; launches never mutate it.
Arena capacity is derived entirely from config.runtime_env — resolve_arena_sizing()
(runtime_maker.cpp:461) → ArenaStaticSizes{total_heap, sm_size} + layout.offsets.arena_size
→ setup_static_arena(...). So a context-static config means a constant
requested_size, which means commit_region()'s
if (arena.is_committed() && requested_size <= cached_size) return 0;is always the branch taken, and the grow / release branches are unreachable by
construction. FREEZE guards against something the declared contract already rules out.
And a gate is weaker here than code. FREEZE puts the guarantee on the caller
remembering to call it; forget it and the launch path silently reverts to "usually doesn't
allocate, but might" — which surfaces inside an ACLGraph capture as a mysterious failure on
whichever launch first changes sizing. It also models the violation as caller misuse,
which is why it needs a new outward-facing code. If instead kernel mode never allocates
after init, a violation is an internal invariant break and the existing
PTO_RUNTIME_ERR_INTERNAL covers it — no new ABI surface at all. This is the case
.claude/rules/env-macro-gating.md §1 asks us to prefer: "do the thing unconditionally
when it is always correct."
CONFIGURE doesn't survive the same question. Its payload is mode plus capacity
intent, and both already have a single source:
-
mode— whether the caller invokedsimpler_initorsimpler_kernel_mode_initalready
decides it, which is exactly whatExecutionModeClaimStateexists to record. Right now
the same fact is stored twice with two different enums and no synchronisation between
them:KernelCtxControlState::tuple_.mode(SimplerExecutionMode) and
ExecutionModeClaimState::mode_(ClaimedExecutionMode). -
capacity intent —
gm_heap_bytes/gm_sm_bytes/runtime_arena_bytesmap one-to-one
ontosetup_static_arena(uint32_t, size_t gm_heap_size, size_t gm_sm_size, size_t runtime_arena_size),
which is already fed fromCallConfig.runtime_env. Andsimpler_kernel_mode_init
already takes aconst CallConfig *.Worth flagging that this is the exact link where
PTO2_RING_*was retired:codestyle.md
§10 records that removal as the model to copy, becauseCallConfig.runtime_env"already
carried the same sizing per task and was strictly more expressive"
(warn_on_retired_ring_env()in eachruntime_maker.cppis what that left behind). A
third channel for the same sizing, frozen into a public ABI, is harder to retire than the
env var was. What canruntime_envnot express that these three fields can? If the
answer is "kernel mode wants final byte counts rather than per-ring parameters", adding
that path toRuntimeEnvseems preferable to a parallel ABI entry.
Suggested landing: enforce it in commit_region() by reading the existing single
source, ExecutionModeClaimState::mode() == Kernel, rather than a new frozen_ bit.
Reaching the grow or release branch under kernel mode is then a bug, reported as
PTO_RUNTIME_ERR_INTERNAL. No new state, no new entry point, no new error code.
What that removes: the simpler_kernel_mode_ctx_control entry plus both stubs (dlsym
surface 5 → 4) · SimplerKernelCtxControl + SimplerKernelCtxAction +
SimplerExecutionMode + 9 static_asserts · KernelCtxControlState (88 + 81 lines) ·
Environment / Capabilities and the device_bound() / has_committed_arena_region()
accessors added to both runner bases · PTO_RUNTIME_ERR_CAPACITY_EXCEEDED ·
test_kernel_ctx_control.cpp (225 lines). Core churn drops by roughly 40%.
Two side notes that fall out of this:
PTO_RUNTIME_ERR_CAPACITY_EXCEEDEDcurrently has zero uses — only the definition at
runtime_c_api.h:114. It is also one letter away from the existing
SIMPLER_ERROR_FANIN_CAPACITY_EXCEEDED(device band, code 4) while meaning something
completely different ("capacity is frozen, don't touch it" vs "the fanin pool is
genuinely full").- It also dissolves a contradiction in the current preconditions. FREEZE requires
env.init_done, which bothc_api_shared.cppmap todevice_bound()=device_id_ >= 0,
anddevice_id_has exactly one writer — the program-mode path at
device_runner_base.cpp:475-479, right afterrtSetDevice, commented "simpler_init
performs the only lifetime write". SinceExecutionModeClaimStatemakes the two modes
mutually exclusive, a kernel-mode context can never satisfyinit_doneas mapped. Today
that is masked becausecaps.kernel_modeis false everywhere;
FreezeSucceedsOnceThenFailsClosedpasses only by hand-supplyingkInitWithCapacitywith
kFull, a combination no real component can produce. If A1 is not taken, this needs
resolving on its own — a frozen precondition whose only possible satisfier is the
mutually-exclusive mode can't stay frozen.
A2 — drop the version machinery from SimplerKernelInvocationHeader
This header goes host → AICPU, and both ends are produced by build_runtimes.py in the
same pip install, landing in the same build/lib/{arch}/{variant}/{runtime}/. They cannot
be built separately, so abi_version can never actually disagree.
Suggest removing: SIMPLER_KERNEL_INVOCATION_ABI_VERSION + the abi_version field + its
consumer-side check · header_bytes (in one build the answer is sizeof) · reserved0 and
reserved[2], whose only purpose is "add a field later without moving the layout", i.e.
version evolution spelled differently — in one self-consistent tree you add the field and
change both sides · the 18 offset static_asserts and the 12 lines of test that re-assert
them. 64 → 40 bytes.
Keep is_trivially_copyable_v && is_standard_layout_v: that guard is
codestyle.md §8's requirement and it catches a real mistake (a pointer or std::string
sneaking into a wire struct), which is a different concern from versioning.
Same question applies to SimplerKernelCtxControl if it survives A1 — though struct_size
should go regardless. In one build sizeof is constant so the check is vacuous; across
builds abi_version already covers it. Two fields guarding one invariant, and because the
match is exact, a v2 struct is hard-rejected rather than negotiated — so it doesn't even
provide the evolution it appears to.
Explicit exception: the scalar_count_ compatibility work is the one place in this PR
with genuine cross-build skew, because ChipCallable has an on-disk kernel cache. That
reasoning and its four tests are correct as they stand — please don't remove them. The
distinction is whether the byte stream is written to disk, sent to another machine, or
compiled against by another repo.
B1 — scalar_count duplicates state already derivable from the signature
ChipCallable::signature_[0..sig_count_) already carries the scalars, stated independently
in two places: prepare_callable_common.h:62 ("Scalars are also present
(ArgDirection::SCALAR) and follow the tensor entries") and runtime_maker.cpp:562
("scalars follow the tensor entries"). The repo also already has the derive precedent —
count_callable_tensor_args() (args_dump_aicpu.cpp:282) computes the split by filtering
SCALAR out of sig_count() rather than storing it.
Three concrete consequences:
- Two different caps for overlapping facts.
make_callablevalidates
scalar_count ∈ [0, CHIP_MAX_SCALAR_ARGS=128]whilesignature_holds up to
CHIP_MAX_TENSOR_ARGS=256entries. - No consistency check, and the new test pins the inconsistency as accepted behaviour:
test_task_interface.py:1258buildssignature=[IN, OUT], scalar_count=5and asserts it
round-trips — two args, zeroSCALARentries, declaring five scalars. - An ambiguous consumer contract.
kernel_invocation_header.hsays the counts are
checked "against the callable's declared signature (ChipCallablesig_count/
scalar_count)", butsig_countincludes scalars, so the correct comparandum for
tensor_countissig_count - scalar_count. As written this is a trap for whoever
implements the AICPU-side check.
Either have make_callable verify the field equals the trailing SCALAR run and document
it as a cached derivation, or drop the field for a derived accessor (no wire change needed).
Either way please make the header comment say sig_count - scalar_count explicitly.
B2 — argument validation for the other entries is duplicated, untested
init / prepare_callable / launch validate inline in both
onboard/host/c_api_shared.cpp:1218-1262 and sim/host/c_api_shared.cpp:1020-1064. I
diffed the added sections: 55 of 58 lines are byte-identical, the only differences being
the comment block, one static_cast<DeviceRunnerBase*> vs <SimDeviceRunnerBase*>, and one
log string. The callable_id range check, the callable_size < sizeof(ChipCallable) check,
and the three (ptr == NULL && size != 0) triples each exist twice with no test on either
copy.
The PR justifies the shared placement of KernelCtxControlState on the grounds that "stub
parity is a correctness requirement, not a convenience" — I agree with that, which is why
it's worth noting the argument currently holds for 1 of 5 entries (and 0 of 4 if A1 lands).
A small shared validation helper would put all of them behind the same reasoning.
Minor, same area: the null/size checks are one-directional —
(binary == NULL && size != 0) is rejected but (binary != NULL && size == 0) passes
silently.
B3 — initialize()'s cleanup-failure branch is untested
kernel_execution_state.cpp:87-97 has the most distinctive semantics in the file: when a
create fails and the rollback cleanup also fails, the context lands in Closing rather
than back in New, latches the cleanup error in unexpected_teardown_error_, keeps ops_
for a retry, and returns the create error rather than the cleanup one. None of that is
covered — PartialInitFailureRollsBackCleanly only exercises the clean-rollback path.
Also uncovered: the get_current_device failure return. And
FakeContextOps::create_stream_rc_after is defined but never set by any case (only
create_event_rc_after is used). Setting destroy_failures_remaining alongside
create_event_rc_after, plus one case on the stream knob, would close it.
Smaller points
KernelLaunchOpsis declared and consumed by nothing. The "forbidden operations are
unrepresentable" guarantee only binds a future launch implementation if that
implementation is required to route through the table. Nothing enforces that today — K2
could callaclrtSynchronizeStreamdirectly and no test would notice. Worth stating the
routing requirement in the header as an obligation on the consumer.KernelContextPhase::Initializingis unobservable.phase_only holds it inside
initialize()'s critical section and it is always overwritten before the lock is
released, soclose()'scase Initializingis unreachable. Harmless as defensive code,
but the header's phase-machine diagram also omitsInitializingwhile the enum lists it —
worth making the two agree.- The resource set is hard-wired.
initialize()unconditionally creates all
KernelStreamKind::Countstreams and allKernelEventKind::Countevents. If hbg and tmr
end up needing different event sets, this class changes rather than its caller. If the
four events are genuinely runtime-independent, one sentence saying so would settle it. ExecutionModeClaimState::mark_closed()returns anintthat is always 0 — either
voidor give it a failure case.simpler_kernel_mode_init's first 9 parameters are byte-identical tosimpler_init's
(the tails differ: 3 sdma parameters vs 1context_generation). I am not suggesting
merging the entries — the borrowed-device init semantics genuinely differ. But two
8-parameter binary-loading lists will drift together; a shared
struct SimplerExecutorBinaries { ... }would prevent that.- Reverse improvement worth taking:
simpler_kernel_mode_prepare_callabletakes
callable_size, while the existingsimpler_register_callable(ctx, callable_id, const void *callable)
takes only a pointer and therefore cannot validate the image at all. The new entry is
right; consider backporting the parameter. kernel_execution_state.cpp(206 lines) is compiled into all four host runtimes with
zero production callers, referenced only by its UT. Negligible in size and clearly
intentional for a K1 skeleton, but worth tracking if K2 slips.
ℹ️ pto_isa.pin is a8040450238f162985d8b596fbebeb54bfba2bf5 and this PR changes no
pto-isa header references (verified: zero +/- pto includes in the diff), so no pin bump
is implied. Advisory only.
Net of A1 + A2 this PR gets smaller — one fewer public ABI entry, one fewer wire struct,
one fewer state machine, one fewer error code — while the core kernel-mode guarantee gets
harder, because it stops depending on caller discipline. That seems like the right trade for
a PR whose entire value is that the surface it freezes will not move.
a51b878 to
9b83280
Compare
|
@ChaoWao All review items are addressed; the branch is re-squashed to one commit, rebased onto current A1 — taken in full. A2 — taken. B1 — taken as "cached derivation + verify". B2 — taken. The entry validation is one copy in B3 — taken. New cases cover the failed-rollback path (create fails, cleanup also fails → Smaller points: The one remaining debt is stated in the PR body: the four guard sites cannot be exercised through a real |
ChaoWao
left a comment
ChaoWao
left a comment
There was a problem hiding this comment.
Re-review at 9b83280: verification, then an architectural read
Thanks — A1/A2/B1/B2/B3 all landed, and A1 landed more thoroughly than I asked for
(the three ACL-lifecycle guards were yours, not mine).
What I verified rather than took on trust
| Item | Verified |
|---|---|
| A1 | ctx_control / CtxControl / PTO_RUNTIME_ERR_CAPACITY_EXCEEDED have zero residue repo-wide (the one grep hit is the unrelated pre-existing SubmitDispatchResult::CAPACITY_EXCEEDED); dlsym surface 5 → 4 |
| A2 | fields reordered by alignment, sizeof is 40; version/header_bytes/reserved/offset asserts gone, POD guards kept; the "both sides come from one build" criterion is now stated in the header |
| B1 | validation body is correct — with sig == nullptr && sig_count == 0 the loop doesn't run, so no null deref |
| B2 | one copy, (binary == nullptr) == (size == 0) fixes the one-directional check, 5 dedicated UTs |
| B3 | all three cases present, including the previously idle stream knob |
| CI | 19 pass + 1 skipping (deploy) |
The commit_region() guard is right on the boundaries: kernel_mode short-circuits first
so the program path is untouched, arena.is_committed() lets the first commit through
(otherwise kernel mode could never establish capacity), and the ternary maps exactly onto
the grow/release branches.
I also checked your exhaustiveness claim on ensure_acl_ready class by class, since that
kind of claim needs a positive control: AclInitGuard's only instantiation is below the
guard in force_reset_device (a2a3 877 > 869, a5 833 > 826); acl_ready_'s only
write-to-true is below the guard in ensure_acl_ready; the fatal path reaches reset only
through attempt_fatal_reset → force_reset_device, and its if (acl_ready_) block has no
else reset. Within the four APIs it names, the claim holds.
The architectural read
Below is the part I owe you that I didn't give last round. One judgement, then the
structure behind it.
This PR uses two opposite techniques for one constraint — "kernel mode must not touch the
caller's device state" — and applies them to the wrong halves.
| Technique | Where | Strength | Status |
|---|---|---|---|
| Typed allowlist — forbidden ops don't exist in the type | KernelContextOps / KernelLaunchOps |
Structural: a future author cannot write the call | Zero consumers, not wired |
Scattered denylist — if (mode()==Kernel) refuse |
8 sites across 4 files | Exhaustive: true today, by discipline tomorrow | This is the one actually in force |
The technique that survives contact with future edits is the one that isn't connected yet.
The three problems below are consequences of that inversion, not separate defects.
1. The denylist's perimeter is drawn around the wrong set, and the main path is outside it
ensure_acl_ready is not on the program path at all. It's exposed as its own C entry
ensure_acl_ready_ctx, and its only caller is ChipWorker::create_comm_stream_checked
(chip_worker.cpp:977) — the comm path. The ordinary program path is:
simpler_init → attach_current_thread(device_id)
├─ rtSetDevice(device_id) ← no guard
├─ configure_aicore_op_timeout() ← no guard
│ └─ aclrtSetOpExecuteTimeOutV2()
└─ device_id_ = device_id
aclrtSetOpExecuteTimeOutV2 is device-global configuration. It isn't one of the four APIs
the claim enumerates, so the perimeter misses it — and it is precisely the
borrower-pollutes-host case: kernel mode borrows torch_npu's device and silently changes
the op-execute timeout for every torch_npu operator on that device. Worse than a stray
rtDeviceReset, because nothing fails; the host's behaviour just quietly changes.
This isn't an oversight so much as the denylist's defining property: you have to already
know what to forbid in order to forbid it. An allowlist inverts that — a call absent from
the table cannot be reached, no enumeration required.
2. attach_current_thread fuses three concerns, and K2 has no seam
rtSetDevice(device_id); // (1) bind thread to device
if (device_id_ == -1) {
configure_aicore_op_timeout(); // (2) mutate device-global config
device_id_ = device_id; // (3) record identity
}Kernel mode needs (3), probably wants (1), and must never have (2) — and there is no seam to
separate them. K2's simpler_kernel_mode_init will have to either reuse this (polluting the
host's timeout) or write a second path that only does (1)+(3), at which point device_id_'s
"simpler_init performs the only lifetime write" comment stops being true.
This is also the root of the init_done contradiction from the last round. A1 removed
FREEZE, but the underlying coupling — device_id_'s write being welded into a program-only
method — is untouched and will resurface in K2 unchanged.
3. Identity is a construction-time property modelled as a runtime state machine — and nothing writes it
ExecutionModeClaimState has Unclaimed → Program|Kernel → Closed. But a context's identity
is fixed at its first init and never changes: that's a constructor parameter, not a state
machine. The costs of modelling it as one have all come due:
Unclaimedmust exist, and since every guard reads== Kernel,Unclaimedsilently
means "program".- Neither init entry claims.
simpler_initgoes straight toattach_current_thread
with no claim; kernel init is a stub.claim_program/claim_kernelhave zero call
sites insrc/— only 8 reads ofmode(). - Therefore
mode()is permanentlyUnclaimed, all 8 new guards are permanently
unreachable, andclaim_*/abort_kernel_initialization/mark_closedare dead code
outside their UT. - The program/kernel mutual exclusion the class exists to provide is currently enforced by
nothing.
If identity were fixed when the context is created, Unclaimed wouldn't exist and "the
defence silently does nothing because someone forgot to claim" would be structurally
impossible. As it stands this is the same anti-pattern FREEZE was removed for — a guarantee
resting on someone remembering a step — relocated rather than eliminated.
4. One handle carries two contracts, indistinguishable at the C ABI
simpler_run(ctx, ...) on a kernel-mode context is type-legal. It doesn't break today only
because kernel mode can't be established; after K2 the thing stopping it will be yet another
runtime guard.
Meanwhile the core methods are growing identity branches: finalize() already has three
paths (acl_ready_ / kernel / else), setup_static_arena() has two capacity semantics,
ensure_acl_ready() has a permanently-refusing path. Persistent state, launch blobs and
invocation snapshots are all still to come, and each will add its own branch. The seam
belongs at the handle: if the two identities produced distinct types (or the context carried
an immutable mode), "program entry on a kernel context" would be a type error instead of the
next guard's customer.
5. The concept has no owner
ClaimedExecutionMode (state machine) lives in platform/include/host/;
SimplerExecutionMode (wire value) lives in task_interface/. Two representations of one
concept in two architectural layers, with no conversion function and no consistency
guarantee — K2 will have to invent the mapping when it writes the state machine's Kernel
into the header's mode. Execution identity is neither a platform detail nor a
task_interface detail.
Suggested direction
Not all of it belongs in this PR — but it's worth settling before the surface is frozen:
- Move identity to context creation, demoting
ExecutionModeClaimStatefrom a mutable
state machine to an immutable field.Unclaimeddisappears; guards stop depending on who
remembered to claim. - Split
attach_current_threadinto composable steps so kernel init can take
"record device_id_" and "bind thread" without "mutate global timeout". K2 needs this seam;
it is cheaper to cut now. - Move the device lifecycle onto a capability table, isomorphic with
KernelContextOps/KernelLaunchOps. The PR already argues that technique is right for
launch; it is equally right for the ACL lifecycle, which is the half actually executing
today. - If 2 and 3 are too large for this PR, it should at minimum wire
claim_program()into
simpler_init. That changes no program behaviour (first call succeeds, idempotent) but
gives the state machine's program half full CI coverage and makes the mutual exclusion
real. Without it the 8 guards are declarations until K2. Worth deciding whatfinalize()
does — amark_closed()there would reject aninit → finalize → initreuse.
Two concrete items independent of the above
scalar_count == 0 is ambiguous in a way the new formula doesn't survive. The field
comment keeps "0 means either an artifact built before this field existed or an orchestration
that takes no scalars; the two are indistinguishable", while the invocation header now states
unconditionally that a consumer checks tensor_count against sig_count - scalar_count. A
legacy callable whose signature holds 5 SCALAR entries with scalar_count = 0 makes that
formula yield tensor_count = sig_count, counting the scalars as tensors. B1 fixed the
formula but not its interaction with the sentinel.
Two options; I'd prefer the second. Either document the fallback (scalar_count == 0 ⇒
derive by counting SCALAR entries, i.e. what count_callable_tensor_args() already does),
or have make_callable also reject scalar_count == 0 when the signature does contain
SCALAR, so 0 unambiguously means "no scalars". I checked examples/, tests/st/ and
simpler_setup/: no production ChipCallable signature contains ArgDirection.SCALAR
today, so the stricter version breaks no existing caller.
Fatal teardown under kernel mode retries three times. attempt_fatal_reset(force_reset_device, kFatalResetAttempts=3) will hit the new guard three times, emitting three
"force_reset_device: refused" errors plus a "did not confirm clean" — which reads like a
failed reset when it is in fact a by-design refusal. Returning UNSUPPORTED is semantically
right (kernel mode genuinely must not reset the caller's card), but the branch belongs
before attempt_fatal_reset. Unreachable today; K2's problem.
None of this is a "it's broken" finding — CI is green and the program path is genuinely
untouched. The architectural point is narrower: A1 removed a guarantee that rested on the
caller remembering to call FREEZE, and replaced it with guarantees that rest on developers
remembering to add guards and on init remembering to claim. The abstraction level dropped;
the pattern didn't change. Item 4 above is the cheapest step that turns the current
declaration into something CI actually exercises.
1ecb27c to
dc1268c
Compare
|
@ChaoWao This replaces an earlier comment of mine on the same subject — if you have two notifications from this PR within the hour, this is the one to read. My first reply argued the identity work belonged in a follow-up PR. That was wrong, and it is now in this one at Why the larger diff is still safe to read as zero program-path change
The one added call on the program path is Your itemsItems 2, 3 and 5 — identity is a write-once property, not a state machine. There is no unlatch and no rollback. That is an init-side decision with a consequence worth writing down rather than leaving to be derived: deleting
Items 1 and 2 — the attach split. Of the seventeen
Item 3 was right about the goal and wrong about the mechanism — and I would not have found that without tracing itYou said wiring the claim would make the guards fire. It would not have, and neither would a latch on its own. Two of the three ACL guards sit behind Making One defect neither of us has reported, found while checking the aboveThe kernel no-reset branch in It is reachable because Today this is inert along with everything else — nothing latches KERNEL — so it is a latent defect that arms the moment K2 makes kernel init real. I would rather hand it to you named than have you find it. The cheaper of the two fixes looks like refusing a kernel init on a context whose Also in, from the previous roundEvery guard reads On Not in this PR — four items, and none of them is "the next PR"Item 4, one handle carrying two contracts — yours to rule. This wants distinct types or an entry-level split at the C ABI, which is a surface change, and it does not block K2. Today the eleven program-only entries ( The fatal-teardown policy — now a card with no owner. This stopped being a branch move once
Merge requestThe request stands on a better basis than last time: the guards are no longer declarations, the seam K2 needs exists and is labelled unexercised, the parts that remain inert are shown to be inert by grep rather than asserted, and the three things I am leaving out are named with an owner each instead of pooled into a vague follow-up. Item 4 is the one I need from you before it can move. |
poursoul
left a comment
poursoul
left a comment
There was a problem hiding this comment.
Review: K1 kernel-mode C ABI skeleton (#2064)
merge-base：405b5bbd ｜ head：86dd62b4 ｜ CI 全绿（含 onboard a2a3/a5、双 arch UT、packaging）
先说结论：方向和分层是对的，身份 write-once、guard 只读一处、stub 与真实现共享校验、受限词汇表让禁止操作不可表达——这几个决定都比"加个 bool 判断"稳得多。但作为一个**唯一产出就是"一份别人可依赖的接口说明"**的 PR，有四条建议在合入前处理。
0. 变更规模
| Bucket | 文件 | 增 | 删 | churn |
|---|---|---|---|---|
| Core | 19 | 1165 | 38 | 1203 |
| Test/Ex | 9 | 836 | 3 | 839 |
| Docs | 2 | 9 | 2 | 11 |
| Build | 4 | 4 | 0 | 4 |
| TOTAL | 34 | 2014 | 43 | 2057 |
Core 1203 行超过 1000 行阈值。不过构成特殊：其中约 480 行是新头文件里的说明性注释，实际可执行逻辑不到 400 行——真正的审阅负担不在代码量，而在下面第 1 条。
1. 机制理解（供核对，若有误读请指正）
- 身份：
ExecutionModeLatch（src/common/platform/include/host/execution_mode_latch.h）挂在两个 runner base 上，write-once。simpler_init在触碰任何 runner / process 状态之前先 latch PROGRAM，所以互斥在每次 program init 上都成立，不依赖谁记得调一个单独的 claim 步骤。无 unlatch、无 rollback。 - C ABI：4 个新入口（
simpler_kernel_mode_supported/init/prepare_callable/launch），第五个生命周期入口复用已有的finalize_device；新增 host band 错误码PTO_RUNTIME_ERR_INVALID_STATE = -1003。 - wire 信封：
SimplerKernelInvocationHeader（40 字节）+ 后随 runtime-specific payload；payload 格式归各 runtime，两侧同一次build_runtimes.py产出，故不带版本协商。 - 共享参数校验：
kernel_entry_validation.h三个 inline 函数编进全部 8 个 host-runtime 组件，保证 stub 与真实现接受/拒绝完全相同的参数。 - 状态机：
KernelExecutionState，New → Collecting ⇄ ReadyEnqueued，部分入队失败 →Poisoned（只接受 close），close → sticky 可重试Closing→Closed；两个独立错误槽（首个 poison 原因 vs 首个真实 teardown 失败）。 - 受限词汇表：
KernelContextOps(5 op) /KernelLaunchOps(6 op)，synchronize、分配、stream 创建、capture 查询、model attach 在表里无法表达。 - 设备绑定拆分：
attach_current_thread→bind_current_thread（纯 rtSetDevice）+attach_current_thread（program：bind + 写 op-execute 看门狗 + 记device_id_）+adopt_borrowed_device（kernel：只记device_id_）。 - guard 落点（8 处，当前全部不可达）：
ensure_acl_ready、force_reset_device、finalize()的 rt 层 reset、attach_current_thread、setup_static_arena的commit_region（onboard + sim），a2a3 / a5 各一份。
2. 做对的地方
- a2a3 / a5 双树同步落地，无遗漏，符合 codestyle 规则 10「同一 commit 完成所有 sibling」。
scalar_count_的落位很稳妥：塞进config_name_len_与storage_之间的历史尾部 padding，sizeof(ChipCallable)保持 9376，所有字段偏移不变，legacy blob 读作 0；并且顺手给ChipCallable/CoreCallable补了 20 条 offset/size ABI 断言，这是净收益。validate_kernel_prepare_callable_args用alignof(ChipCallable)而非CALLABLE_CHILD_ALIGN——因为storage_声明为alignas(CALLABLE_CHILD_ALIGN) char storage_[]，两者等价，检查是对的。- Python 侧
scalar_count是尾部带默认值的 kwarg，不破坏任何 Python 调用方。 commit_region把is_kernel()提到 lambda 外只取一次，没在循环里反复加锁。- 新增的
KernelExecutionState/ExecutionModeLatch都没带项目名前缀 ✅（对比下面 Should-fix 5）。 #pragma once✅｜enum class✅｜无新增PTO2✅
3. Must fix / 必须讨论
3.1 PR 描述描述的是一个已被推翻的版本
这是本 PR 最实质的问题，因为它唯一的价值就是给后续并行开发一份可依赖的说明——说明本身错了，冻结就失去意义。
- body 里的
ExecutionModeClaimState、accepts_kernel_calls()、claim_kernel()、ClaimedExecutionMode，树里一个都不存在。 - body 写 claim 是 "abortable on init failure"，而
execution_mode_latch.h明确写 "There is no unlatch and no rollback"——直接矛盾。 - body 的 "Known debts" 里「
ClaimedExecutionMode与SimplerExecutionMode是两套零值相反的表示且无 converter」这条已不成立（现在只有SimplerExecutionMode一套）。 - body 的 "Deliberately left to the follow-up PR" 一节用三段论证说明拆
attach_current_thread（thread bind / 设备看门狗 / 身份三合一）不在本 PR 处理，但代码确实拆成了bind_current_thread/attach_current_thread/adopt_borrowed_device。 - 测试数 body 写 23，实际 24。
旁证：把 #2171 携带的 K1 快照放进来对比，这个机制至少改过三版——
| 来源 | 机制 |
|---|---|
| 本 PR body 描述 | ExecutionModeClaimState / claim_kernel() / abortable |
| #2171 携带的 K1 快照 | kernel_ctx_control / SimplerKernelCtxControl wire struct / CONFIGURE→FREEZE |
| 本 PR 当前 head | ExecutionModeLatch / latch() / 明确无 rollback |
body 停在第一版，#2171 停在第二版，代码已在第三版。请按当前代码重写 body，特别是 "What is frozen vs. what is enforced" 那张表和 follow-up 一节。
3.2 全新的 wire struct 没有 layout 断言
src/common/task_interface/kernel_invocation_header.h 里只有 is_trivially_copyable / is_standard_layout 一条断言，没有 sizeof == 40，没有任何 offsetof——而同一个 PR 给已存在多年的 ChipCallable 补了 20 条。test_kernel_invocation_header.cpp 只做同进程 memcpy roundtrip，那个测试对布局变化永远不会失败。
同时：36 字节有效字段 + 4 字节尾部 padding，既没有显式 reserved 字段，也没有零值校验——一个没写 {} 的实例会把未初始化字节 memcpy 到 AICPU。host_copy_tensor_count（注释写 "consumers reject nonzero meanwhile"）本质上就是个没有 validator 的 reserved 字段。
建议：补 sizeof + 全字段 offsetof 断言；把尾部 padding 显式化为 uint32_t reserved_ 并在未来的 AICPU dispatch 入口校验为零。
3.3 mode guard 的返回码没有约定
同一类「模式不对」分了三种码：
| 位置 | 返回 |
|---|---|
ensure_acl_ready / force_reset_device / attach_current_thread |
PTO_RUNTIME_ERR_UNSUPPORTED |
adopt_borrowed_device |
PTO_RUNTIME_ERR_INVALID_STATE |
commit_region |
PTO_RUNTIME_ERR_INTERNAL |
本 PR 的卖点正是冻结接口，返回码约定却没冻结。
另外 kernel_entry_validation.h 把纯粹的调用方参数错误（空指针、callable_id 越界、callable_size 不足、binary/size 不成对）一律报成 PTO_RUNTIME_ERR_INTERNAL——而 INTERNAL 在这个 band 的定义是「内部不变式被破坏，诊断看上一行日志」。这里缺的是一个 PTO_RUNTIME_ERR_INVALID_ARGUMENT。
4. Should fix
4.1 scalar_count_ 这个字段的存在价值可疑
它自己的文档规定：消费者必须先推导有效值——字段非零就用字段，为零就去数 signature 的 SCALAR 项。既然 fallback 永远可用而且廉价（signature 就在同一个 header 里，最多 256 项），这个缓存字段从来不是 load-bearing 的：正确答案永远能只靠 signature 得到。代价却是一条 wire ABI 面、make_callable 一个必需的第三位置形参（仓外 C++ 调用方直接编译不过）、6 个 UT，以及一条要求每个消费者各自实现的推导规则。
建议二选一：用 -1 表示"未记录"让字段无歧义（0 就真的是 0 个 scalar）；或者不加字段，提供一个 count_scalar_args(sig, n) helper。
附带一处误导交叉引用：callable.h 注释说 "the split count_callable_tensor_args already computes"，但那个函数签名是 count_callable_tensor_args(const CoreCallable &)，对 ChipCallable 的 signature 并不适用。
4.2 死代码进了全部 8 个 .so
kernel_execution_state.cpp被加进 4 个 CMakeLists（覆盖 8 个 variant），但KernelExecutionState/KernelContextOps/KernelLaunchOps零生产调用点，只有 UT 引用。adopt_borrowed_device零调用点；bind_current_thread唯一调用者是attach_current_thread，所以这次拆分目前收益为零。- sim 侧的 arena kernel guard 是永久死代码，不是"暂时未接线"：sim 自己声明 kernel mode 永不支持（"it has no real streams for a caller to lend"），这条分支按构造永不可能触发。
4.3 4 个 dlsym 无条件 eager 加载，让旧 .so 硬失败
ChipWorker::init 里 4 个符号无条件 load_symbol，缺符号即抛异常。后果：任何旧的预编译 host_runtime.so 会让 ChipWorker::init 直接抛，即使用户完全不碰 kernel mode——而本 PR 里所有 backend 的 supported() 都返回 0，且 ChipWorker 拿到的 4 个函数指针没有任何方法调用它们。为一个当前谁都用不到的能力，把整个 worker 初始化变成硬失败点，代价与收益不成比例。
现成的能力协商机制 simpler_kernel_mode_supported() 已经在这里了，合理分层应是：
| 符号 | 定位 | 理由 |
|---|---|---|
simpler_kernel_mode_supported |
必需 | 能力探测入口，必须永远可调 |
init / prepare_callable / launch |
应当可选 | 仅在 supported() 非 0 后才需要，lazy 解析即可 |
否则等于说「能力探测函数与被探测的能力本身绑在同一必需性等级」，探测就失去了意义。真正钉住导出的是 test_host_runtime_abi.py，那部分保留即可。
4.4 头文件注释里的状态叙述会立刻腐化
.claude/rules/comments.md 要求注释是"present-tense fact about the code as it currently stands"，而这几处是「目前还没实现 / 还没人用」的状态叙述：
kernel_invocation_header.h：Nothing mints this value and no device-side comparand exists/no consumer implements itruntime_c_api.h：so today only program contexts exist and those guards never firedevice_runner_base.cpp：The refusal is not self-contained ... so a fired guard drops the very base addresses it names
这些句子在下一个 PR 落地的那一刻就变成谎话，而且没人会想起来去改。"frozen vs enforced" 那张表属于 commit message / docs/，不属于头文件。
4.5 Simpler 前缀漏进 C++ 类内私有别名
src/common/worker/chip_worker.h：
using SimplerKernelSupportedFn = decltype(&simpler_kernel_mode_supported);
using SimplerKernelInitFn = decltype(&simpler_kernel_mode_init);
using SimplerKernelPrepareCallableFn = decltype(&simpler_kernel_mode_prepare_callable);
using SimplerKernelLaunchFn = decltype(&simpler_kernel_mode_launch);先界定该带前缀的部分：extern "C" 导出符号（simpler_kernel_mode_*）和跨 ABI 的 wire struct / enum（SimplerKernelInvocationHeader、SIMPLER_MODE_*）带前缀是正确的，codestyle 规则 9 对此有明确豁免。
但这 4 个是 ChipWorker 的私有类型别名，作用域已被 ChipWorker:: 限死，永不出现在符号表、不可能冲突，Simpler 这 7 个字符信息量为零。该文件里确实已有 6 个 Simpler*Fn（所以新增的是在跟随既有约定），但同一个 private: 块里也活着另一套——8 个 Comm*Fn（CommBarrierFn 等），对应的同样是 extern "C" 导出符号（comm_barrier 等），却都没带前缀。即：文件里并存两套约定，新代码选了较差的那一套。
按规则 9「replacing the disambiguation it provided with clear names or a namespace」，应为 KernelSupportedFn / KernelInitFn / KernelPrepareCallableFn / KernelLaunchFn。纯 C++ 类内改名，不碰任何 ABI，decltype(&simpler_kernel_mode_*) 右边保持不变；只改新增的 4 个，既有 6 个不动。
佐证：本 PR 自己新增的 KernelExecutionState、ExecutionModeLatch 都没带前缀，做对了。
5. Consider
ExecutionModeLatch的形状：用std::mutex保护一个 bool + enum，而且通过execution_mode_latch()返回非 const 引用把latch()完全公开——write-once 不变式只靠纪律维持。更稳的形状：runner 上提供latch_program()/latch_kernel()，读者拿 const 引用；存储换std::atomic免锁，顺带避免 mutex 成员让 runner 不可移动。hidden_stream()/event()的锁是假安全：锁内取裸指针、锁外使用，并发close()销毁即 UAF。这正是 launch 实现要建在上面的接口，现在定型比以后改便宜。KernelLaunchOps的函数指针带noexcept，KernelContextOps不带，非对称且无说明；enum class KernelStreamKind : size_t底层类型偏重，uint8_t足够。finalize()里出现只含注释的空else if分支；写成else if (!execution_mode_latch().is_kernel())更直白。- 路径 / 命名：
execution_mode.h放在src/common/task_interface/，但它是src/common/worker/runtime_c_api.h那套 C ABI 的枚举；kernel_entry_validation.h的三个函数与ExecutionModeLatch都在全局命名空间且被编进 8 个组件。另外ChipCallable的MaxSig常量叫CHIP_MAX_TENSOR_ARGS(256) 却同时装 tensor 和 scalar——本 PR 引入scalar_count后这个旧名字更容易误导（scalar 另有上限CHIP_MAX_SCALAR_ARGS= 128）。 - 性能：本 PR 无热路径影响，所有 guard 都落在 init / finalize / arena commit，不在 dispatch 路径上。唯一成本是 8 个
.so各多一个未使用的 TU。 - 新错误码沿用遗留
PTO_前缀（PTO_RUNTIME_ERR_INVALID_STATE）与 codestyle 规则 10 有张力，但同一 enum band 内保持一致的优先级更高，跟随PTO_RUNTIME_ERR_BASE是合理的，不要求本 PR 处理。
6. 其他检查
- pto-isa pin ℹ️ advisory：当前 pin 在
5a4f74cbf627d4aac2e0ce10d5e0d8b118343265，本 PR 无任何 pto-isa include 变化（无路径变更、无新增 include），pto_isa.pin未改动——确认该 pin 仍然够用即可。若需 bump，记得用SIMPLER_PTO_ISA_BUILD_COMMIT重建 onboard a2a3 的host_runtime.so。 - stub 覆盖：四个 platform variant 的 CMakeLists 均已加入
kernel_execution_state.cpp✅ docs/investigations/：无与 kernel mode / borrowed device / ACLGraph 相关的已否决提案，本 PR 不与任何既往结论冲突。
7. 结论
needs discussion。合入前建议至少处理：
| 优先级 | 项 | 理由 |
|---|---|---|
| 🔴 高 | 3.1 body 与代码不一致 | 本 PR 的产出就是这份说明，说明错了冻结就没有意义 |
| 🔴 高 | 3.2 wire header 缺 layout 断言 + 尾部 padding | 全新 wire struct 的布局无任何保护，UT 检不出来 |
| 🔴 高 | 3.3 guard 返回码 / 参数错误码 | 冻结接口的 PR，返回码约定没冻结 |
| 🟡 中 | 4.1 scalar_count_ 保留 / 改 -1 / 换 helper |
本 PR 唯一真正改了 wire ABI 和一个必需形参的部分，需作者答复 |
| 🟡 中 | 4.3 eager dlsym | 影响面覆盖所有持旧 .so 的用户，当前收益为零 |
| 🟡 中 | 4.5 类内别名前缀 | 零 ABI 风险，只改新增的 4 个，现在改成本最低 |
| 🟢 低 | 4.2 / 4.4 / §5 | 死代码与注释腐化，可随后续 milestone 收敛 |
86dd62b to
b4ff4e3
Compare
sunkaixuan2018
changed the title
b4ff4e3 to
2ab04b1
Compare
|
@poursoul 已按这份总评修正，并重写了 PR 描述：
§5.1 / §5.5 暂保留：latch 自身在锁内拒绝换模式，公开引用不会绕过 write-once；本次没有热路径成本证据要求替换 mutex，atomic 本身也不会恢复隐式可移动性。文件移动、全局 namespace 和既有 MaxSig 命名不在本次调整中，避免给其他并行 PR 增加机械迁移。K2 接线仍需封住借用设备与已有 ACL 状态的混用，并完成真实 context 的 guard 拒绝测试；当前没有生产 KERNEL latch。 验证：旧的已安装运行库先复现了缺生命周期符号的失败；重建后 CTest 144/144，通过全部 Python UT（2265 passed / 15 skipped），包含上述 7 个真实动态库用例。完整 pre-commit 通过（对齐 CI 的 clang-tidy 18，并使用串行执行避免缓存修复争用）。最终提交仅相对该验证树纠正了状态图注释，提交 hooks 也通过；GitHub CI 尚未全绿，详情见下。 CI 补充：最近检查时 17 项检查通过、1 项失败、1 项仍在运行（另 1 项部署跳过）。失败的是 A5 onboard / PMU TestPmu::test_run：首个错误为 该 PMU 文件及相关 quiesce / buffer pool / 测试代码与本次 base 相同；静态检查未发现本次修改改变该路径，但分片为何合并失败尚未确定，不能据此认定为 flaky。最有价值的补充证据是失败目录保留分片的大小、内容及读写状态。尝试重跑被 GitHub 拒绝（HTTP 403：需要仓库管理员权限），需维护者协助检查失败产物并重跑。A2/A3 onboard 已通过，DeepSeek A2/A3 onboard 检查仍在运行。 |
2ab04b1 to
0b457bb
Compare
nalinaly
left a comment
nalinaly
left a comment
There was a problem hiding this comment.
仅针对 kernel arena 拒绝路径补充一条行内意见：拒绝新请求不能释放 captured graph 可能仍引用的已提交资源。
| "setup_static_arena: kernel mode forbids %s a committed region (cached %zu, requested %zu)", | ||
| requested_size == 0 ? "releasing" : "growing", cached_size, requested_size | ||
| ); | ||
| return PTO_RUNTIME_ERR_INTERNAL; |
There was a problem hiding this comment.
建议修复：拒绝 kernel arena 变更时保留此前已提交的 region。
这里的 PTO_RUNTIME_ERR_INTERNAL 会被外层转换成 ok = false，继而进入 if (!ok)，对 gm_heap、gm_sm、runtime_pool 全部调用 release() 并清零 cached size。DeviceArena::release() 会实际调用底层 free，因此“禁止增长/释放，保护 captured graph 的旧基址”最终反而释放了这些基址。即使把请求视为内部不变量违例，也不能在尚未结束 graph 引用和在途设备使用时销毁旧资源。
建议 kernel 模式先对三个 region 做完整、无副作用的预检查：任一已提交 region 的增长或非零改零请求被拒时直接返回，不进入统一回滚，保持旧基址、cached size 和 committed 状态不变。如果后续允许补建 region，分配失败只回收本次新建资源；不要顺带改变 program 模式原有的全回滚语义。
建议补拒绝路径测试：分别让第一、第二、第三个已提交 region 触发增长/释放拒绝，确认返回错误但旧资源未释放、元数据未清空，底层 free 次数不增加。
当前 kernel init 仍是 stub，所以这是能力启用后的潜伏缺陷，不是已复现的设备故障。建议在本 PR 修复；若明确留给后续接线 PR，也应作为启用 kernel 能力前的硬条件。
There was a problem hiding this comment.
已按建议修复。
改法：kernel 模式的容量判定整体提到提交序列之前，对 gm_heap / gm_sm / runtime_pool 三个 region 做一遍只读预扫，任一违例直接 return PTO_RUNTIME_ERR_INTERNAL，不进入 commit_region，因而也到不了 if (!ok) 的统一回滚。program 模式的全回滚语义一行未动。
判定抽成 kernel_entry_validation.h 里的 kernel_arena_change_is_forbidden(committed, cached, requested)，生产代码与测试共用同一份定义，免得两边各写一遍再漂移。语义：未提交一律放行；已提交时，增长（requested > cached）与非零改零的释放都拒绝，等大与缩小放行——后两者既不换 base 也不改 committed size。
测试：tests/ut/cpp/common/test_kernel_entry_validation.cpp 新增 4 个 KernelArenaGuard.*。其中 RefusalScanOverCommittedRegionsReleasesNothing 用计数 allocator 背书的三个真实 DeviceArena：三区提交后由第三个 region 触发增长拒绝——此时前两个已提交的 peer 正是回滚会误伤的对象——断言底层 free 次数不增、三个 base() 与 is_committed() 全部不变。
有一条没做到，说清楚。 您建议"分别让第一、第二、第三个已提交 region 触发拒绝"，我只覆盖了第三个。它是这条性质的最坏情形（前面有两个已提交 peer 待保护），但确实不是逐位置覆盖。
更要紧的是这些用例钉的是谓词加 arena 不变量，而不是真正执行一次 setup_static_arena：K1 里没有任何途径能把 context latch 成 KERNEL——simpler_kernel_mode_init 在 latch 之前就返回 UNSUPPORTED，而按您在另一条里的意见，这个 stub 也不该提前认领。所以"回滚没有被进入"目前是靠结构保证的：refusal 的 return 位于 commit_region 这个 lambda 被定义之前，中间没有任何可变异的语句。真正跑通这条路径要等 K2 接线，我把它记成了启用 kernel 能力前的硬条件，不是可选项。
head 31b03d92d，CI 19 pass / 1 skipping。
0b457bb to
51dcf38
Compare
nalinaly
left a comment
nalinaly
left a comment
There was a problem hiding this comment.
补充一条 ACL 所有权准入意见：以不支持同进程 PROGRAM/KERNEL 混用为前提，建议先封住 ensure_acl_ready 的未认领 context 旁路，并保留现有独立通信初始化入口。
| } | ||
| // A kernel-mode context borrows the caller's device and ACL context; | ||
| // ACL initialization requires program-mode ownership. | ||
| if (execution_mode_latch().is_kernel()) { |
There was a problem hiding this comment.
建议在 ACL 副作用之前认领 PROGRAM，封住未认领 context 的旁路。
按 workspace 等公共资源不支持同进程 PROGRAM/KERNEL 混用的前提，这里的第一层修复应完善模式准入，不需要引入共享 ACL 生命周期方案。目前 is_kernel() 只拒绝已经认领 KERNEL 的 context；未认领 context 仍可通过 ensure_acl_ready_ctx 调用 aclInit/aclrtSetDevice 并设置 acl_ready_=true，却没有认领 PROGRAM。后续 kernel init 接线后，若只做 KERNEL latch 与 borrowed-device adopt，就可能产生 KERNEL && acl_ready_；healthy finalize 会优先进入 ACL 分支，调用 aclrtResetDevice/aclFinalize，后面的 kernel no-reset 判断保护不到这一组合。当前 K1 kernel init 仍返回 UNSUPPORTED，因此这是启用前需要关闭的潜伏路径，不是已实测的当前 kernel 执行故障。
建议在基础参数校验之后、任何 ACL 调用之前，将此 guard 替换为：
const int mode_rc = execution_mode_latch().latch(SIMPLER_MODE_PROGRAM);
if (mode_rc != 0) return mode_rc;这样未认领 → PROGRAM，已是 PROGRAM → 同模式认领成功，已是 KERNEL → INVALID_STATE 且没有 ACL 副作用。ACL 初始化失败也不撤销身份，与现有 write-once/no-rollback 合同一致。模式选择的竞争由已有 latch mutex 处理，无须仅为模式选择新增覆盖 ACL 调用的大锁；这不代表同模式 init/close 的生命周期并发已经安全。
不能简单改成“必须已经是 PROGRAM”：现有 tests/ut/cpp/hardware/test_comm_lifecycle.cpp:154 起直接使用 create_device_context → ensure_acl_ready_ctx，并不先调用 simpler_init。应保留该独立通信入口，并将合同明确为“首次初始化所有权的入口决定模式”，其中包含 ensure_acl_ready_ctx。A5 的同名函数需同步修改。后续真实 kernel init 应在资源副作用之前认领 KERNEL，但不要让当前 UNSUPPORTED stub 提前认领。
回归用例建议覆盖：helper-first 认领 PROGRAM 后拒绝同 context 的 KERNEL；KERNEL-first 调用 helper 被拒且 ACL 调用次数为零；PROGRAM 重复调用保持同一身份；注入 ACL 初始化失败后身份仍是 PROGRAM；原独立通信初始化用例继续通过。K1 可用 fake ACL/runner 验证入口副作用边界，不能只测 latch 类或把 UNSUPPORTED 当作互斥验证。finalize 还可防御性拒绝 KERNEL && acl_ready_，在任何破坏性清理前报错并保留状态，不能静默清标志或继续 reset/finalize。
范围说明：此修改只封住同一个 context 的旁路。不同 context 的同进程两模式混用若要主动拒绝，仍需统一的进程级准入，不能把各 runtime 动态库各自的 static 当作进程级保护；这是禁止混用的另一层，不是要求支持两模式共存。
There was a problem hiding this comment.
已按您给的形状修复，a2a3 与 a5 同 commit 落地。
在 device_id 基础校验之后、任何 ACL 调用之前：
const int mode_rc = execution_mode_latch().latch(SIMPLER_MODE_PROGRAM);
if (mode_rc != 0) { LOG_ERROR(...); return mode_rc; }未认领→PROGRAM，已是 PROGRAM→同模式返回 0，已是 KERNEL→INVALID_STATE 且身后没有任何 ACL 副作用。ACL 初始化失败不撤销身份，与 write-once / no-rollback 一致。
您指出的那条链现在不可达了，这点我做了正向核对而不是想当然：acl_ready_ = true 在两个 arch 上各只有一个写入点，就是 ensure_acl_ready 自己（a2a3:187 / a5:146）。它现在先 latch PROGRAM，所以 KERNEL && acl_ready_ 无法成立，healthy finalize 的 ACL 分支也就不会在借来的卡上跑 aclrtResetDevice / aclFinalize。
独立通信入口保住了：test_comm_lifecycle.cpp:154 的 create_device_context → ensure_acl_ready_ctx（不经 simpler_init）落在 fresh context 上，latch PROGRAM 成功，行为不变。合同按您的说法写进了头注释与 PR 描述——"首次取得初始化所有权的入口决定模式"，其中包含 ensure_acl_ready_ctx。UNSUPPORTED 的 kernel stub 没有提前认领。
两件没做的，明说：
-
您建议的入口级回归用例——KERNEL-first 调 helper 被拒且 ACL 调用次数为零、注入 ACL init 失败后身份仍是 PROGRAM——K1 里构造不出来，因为没有任何途径能 latch KERNEL（理由同上）。这正对应您那句"不能只测 latch 类或把 UNSUPPORTED 当作互斥验证"，我认同，所以没有拿 latch 类测试来充数，也没有把 UNSUPPORTED 包装成互斥证据。这块随 K2 接线补，已记为启用前硬条件。
-
finalize 里对
KERNEL && acl_ready_的防御性拒绝没有加。理由是上面那条可达性核对：该组合现在无法产生。加一个恒假分支，我更倾向于等 K2 真正有 KERNEL latch、teardown 策略也定下来时一起做，否则它现在既无法被测试覆盖，也容易在 K2 改动时被当成已有保护而误信。如果您认为即便不可达也应当先立着，我可以补——这条听您的。
head 31b03d92d，CI 19 pass / 1 skipping。
51dcf38 to
eeb2614
Compare
poursoul
left a comment
poursoul
left a comment
There was a problem hiding this comment.
Re-review（第二轮）
新 head 51dcf380，rebase 至当前 main c540572d。CI 19 pass / 1 skipping（deploy）。
先说结论：approve。上一轮四条高优先级全部解决，而且几处的解决方式比我建议的更彻底——scalar_count 不是补 sentinel 而是证明字段不必要后整个删除，泄漏的 accessor 不是加保护而是直接移除，sim 侧那条永久死的 guard 直接删掉。剩下两条都是一两行的收尾。
1. 上一轮意见的处理情况
| 上轮条目 | 状态 | 怎么改的 |
|---|---|---|
| 🔴 3.1 body 与代码不一致 | ✅ | body 完全重写，ExecutionModeClaimState / abortable / 过期的 "Deliberately left to the follow-up PR" 段全部消失，现在与代码一致 |
| 🔴 3.2 wire header 缺 layout 断言 | ✅ | sizeof == 40 + 8 条 offsetof 全补齐；尾部 padding 显式化为 uint32_t reserved_；头注释写明"生产者必须完整零初始化"与"消费者必须拒绝非零 host_copy_tensor_count 和 reserved_" |
| 🔴 3.3 返回码不统一 | ✅ | 新增 PTO_RUNTIME_ERR_INVALID_ARGUMENT (-1004)；结构性参数错误统一用它，四处 mode guard 统一 INVALID_STATE，commit_region 保留 INTERNAL 并在注释里说明这是容量不变式而非调用错误。四档语义在 body 和头文件里都写清楚了 |
🟡 4.1 scalar_count_ 字段冗余 |
✅ | 字段整个删除，改为 count_scalar_args(sig, n) 派生 + scalar_count() 访问器。make_callable 签名回退（仓外 C++ 调用方不再被破坏），两个既有 UT 文件也不必改动，ChipCallable wire ABI 一个字节没动，20 条 offset/size 断言作为纯加固保留 |
🟡 4.2 死代码进 8 个 .so |
✅ | kernel_execution_state.cpp 从 4 个平台 CMakeLists 移除，只编进 UT；sim 侧那条按构造永不可能触发的 arena guard 删除 |
| 🟡 4.3 eager dlsym | ✅ | 只有 probe 必需，其余三个在 supported() != 0 后才解析；新增 tests/ut/py/test_chip_worker.py 四个用例（unsupported 无符号、supported 全符号、缺必需符号可重试、program init 失败后可重试）。test_host_runtime_abi.py 仍钉住全部四个导出 ✅ |
| 🟢 4.4 状态叙述式注释 | ✅ | today only program contexts exist and those guards never fire、Nothing mints this value and no device-side comparand exists、The refusal is not self-contained … 等全部删除或改写为 present-tense。全量 grep 无残留 |
🟢 4.5 Simpler*Fn 前缀 |
✅ | 改为 KernelSupportedFn / KernelInitFn / KernelPrepareCallableFn / KernelLaunchFn |
| Consider 2 锁的假安全 | ✅ | 直接删除 hidden_stream() / event() 两个会泄漏裸指针的 accessor，TOCTOU 面消失 |
Consider 3 noexcept 非对称 + size_t 底层类型 |
✅ | KernelContextOps 也加 noexcept；两个 enum 改 uint8_t |
Consider 4 空 else if 分支 |
✅ | 改为 } else if (!execution_mode_latch().is_kernel()) |
Consider 1 latch 形状（mutex / 非 const 引用暴露 latch()） |
⬜ 未动 | 属 Consider，可接受 |
另外确认几处回归风险为零：
ChipWorker::init里新增的三个load_symbolthrow 点落在已有的try内，其catch开头就是destroy_device_context_fn_(device_ctx_)，回滚完整。kernel_supported_fn(device_ctx_)调用前device_ctx_已创建并做过 null 检查。KernelExecutionState状态机实现除INVALID_ARGUMENT拆分外未变，语义无回归。
2. 本轮新发现
2.1 应该修：改码时漏改文档
src/common/platform/onboard/host/device_runner_base.h:246 仍写：
Refuses with
PTO_RUNTIME_ERR_UNSUPPORTEDon a context latched to kernel mode
而 device_runner_base.cpp 的 attach_current_thread 现在返回 PTO_RUNTIME_ERR_INVALID_STATE。修 3.3 时漏掉了这一处，属 doc-consistency.md §1。
2.2 应该修：supported() 的调用时机没写进契约
runtime_c_api.h:590 只有一行：
/** Return nonzero when this runtime/context can execute kernel-mode launches. */
int simpler_kernel_mode_supported(DeviceContextHandle ctx);但 ChipWorker::init 已经把它固定在一个很具体的位置：create_device_context() 之后、simpler_init() 之前。也就是说这个入口必须在一个尚未初始化的 context 上给出答案。
今天 stub 忽略 ctx 直接返回 0，所以无所谓；但这已经成了每个真实后端必须遵守的事实契约——一个想按设备给出不同判断的实现，得知道自己是在未初始化的 context 上被问的。既然本 PR 的产出就是冻结接口，这条应该写进 doc block（"may be called on a created but not yet initialized context; must not require init state"之类）。
2.3 遗留（可接受，请确认是有意的）
bind_current_thread/adopt_borrowed_device仍是零生产调用点。这次拆分已被 body 明确列为本 PR 内容（不再声称留给 follow-up），所以不存在前后矛盾，只是这两个函数要等 K2 才有第一个调用者。- body 末尾 "CI is not yet green: A5 onboard PMU test failed…" 已过时（现在全绿），建议删掉这段。
KernelExecutionState的状态机图从Collecting ⇄ ReadyEnqueued改成→（更准确），但mark_ready_enqueued()实现未变，仍接受ReadyEnqueued → ReadyEnqueued自环，图里没体现。极小。
2.4 Consider
scalar_count()从 O(1) 字段读变成 O(sig_count) 遍历（最多 256），且会throw std::invalid_argument。callable.h被 AICPU TU 包含（aicpu_executor.cpp、aicpu_legacy_executor.cpp、scheduler_dispatch.cpp），目前安全——它是模板成员函数，没有 device 侧调用者就不会实例化，而count_scalar_args本身无 throw 无 STL。但如果将来 AICPU dispatch 路径要消费它，两点都要注意：既不能抛异常，也不该每次 launch 重算。count_scalar_args是全局命名空间的inline自由函数，名字相当通用，而callable.h被约 20 个 TU 直接或间接包含。放进 namespace 或改个更具体的名字更稳。
3. 结论
approve。2.1 和 2.2 建议在合入前顺手修掉，都是一两行；2.3 / 2.4 可留待 K2。
eeb2614 to
31b03d9
Compare
|
@poursoul 谢谢 approve。2.1 / 2.2 已修；顺着您 2.3 里那条过期 body 文字，又清掉了两处同类的。
2.4.1 我给一个不同的定级。 您把 2.4.2 2.3 三条确认都是有意的： 本轮还合入了 @nalinaly 的两条行内意见——kernel arena 拒绝路径的副作用、 head |
ChaoWao
left a comment
ChaoWao
left a comment
There was a problem hiding this comment.
Item 4 ruling, one correction of mine, and confirmations
Read at 31b03d92d. CI 19 pass / 1 skipping. poursoul's approve and nalinaly's two
inline items are in; I'm not re-covering ground they closed.
First: my item 3 was wrong on the mechanism, and your trace is right
I said wiring the claim would arm the guards. It would not have, and your reason is the
one that matters: force_reset_device opens on device_id_ < 0 and finalize() opened on
device_id_ == -1, and device_id_ had exactly two writers — attach_current_thread
(which must rtSetDevice first) and ensure_acl_ready (which guard ① refuses). A kernel
context could reach neither, so those two guards were unreachable through a mechanism
entirely independent of the missing claim. A PROGRAM latch can never make is_kernel()
true, so nothing about wiring it would have changed that.
I reasoned from "the state is never written" to "writing it is what's missing" without
tracing what else stood between a kernel context and each guard. Re-pointing device_id_
at "which device" rather than "owns the device" is what actually arms ② and ③, and I would
not have gotten there from where I was looking.
Item 4 — my ruling: do not split the handle type. Fix the refusal depth instead.
Why not the split
I checked the thing I should have checked before proposing it:
typedef void *RuntimeHandle;
typedef void *DeviceContextHandle;Two typedef void * names are the same type to a C compiler. A
SimplerKernelContextHandle alongside DeviceContextHandle would buy zero compile-time
separation — passing one where the other is expected diagnoses nothing. My last-round
framing ("it would be a type error instead of the next guard's customer") was simply false
on this ABI, and I asserted it without reading that line. Withdrawn.
Getting real separation means opaque struct pointers (struct SimplerKernelContext *),
which changes the shape of the whole C ABI and drags RuntimeHandle with it. That is a
large, purely-structural change to buy a property the runtime check already provides. Not
worth it, and definitely not worth it while K2 is being built against this surface.
What the real problem is
Not the handle's type — the depth at which the refusal happens.
You made simpler_init latch as its first act, explicitly above the first side effect
rather than merely above the attach. That is the right shape. But the other program-only
entries inherit their refusal from attach_current_thread, which is not their first act.
simpler_prepare_run (a2a3 c_api_shared.cpp:800-826) does all of this first:
state = new (runtime) OnboardNativeRunContext(runner, *config, ...); // placement new
...
if (!runner->try_reserve_native_run(state, ...)) { ... } // takes the admission slot
state->runner_reserved = true;
STRACE_CONTEXT(state->trace_inv, state->trace_hid, 1); // emits a trace
int rc = runner->attach_current_thread(runner->device_id()); // ← refusal lands here
if (rc != 0) return cleanup_failed_prepare(state, rc, true);So a kernel-context prepare_run constructs a run context, takes and releases the
native-run admission slot, and emits a trace before being refused. cleanup_failed_prepare
does roll it back, so this is not a leak and not a live bug — the entry is correct today.
My objection is that it is correct by coincidence of ordering plus a complete rollback
path, not by construction, and that property is invisible: nothing marks
try_reserve_native_run as "must not run under a kernel latch", so the next person to add
a step above the attach has no signal. Compare simpler_register_callable, where the only
things above the attach are two read-only checks — that one is clean, but by accident of
how it happens to be written, not by rule.
That is also why "eleven entries are refused because attach_current_thread refuses" is
weaker than it sounds: it is true of the return code, not of the side effects.
What I'd do instead
Give the program-only entries the same shape you gave simpler_init — refusal as the first
act:
// one line at the top of each program-only entry
if (int rc = reject_on_kernel_latch(runner, "simpler_prepare_run"); rc != 0) return rc;Three properties this buys that the current shape doesn't:
- Uniform depth — refusal precedes every side effect at every entry, not just where
the attach happens to sit early. - Uniform code — all of them report
INVALID_STATE, rather than inheriting whatever
the first internal callee returns. - Testable as one invariant — a table-driven test can assert "every program-only entry
on a kernel-latched context returnsINVALID_STATEand leaves runner state unchanged",
which also catches the next entry someone adds and forgets. That is the piece the
current design cannot express at all.
This is a per-entry line plus one test, no ABI change, and it composes with everything you
have already landed. I'd treat (3) as the actual deliverable — the guard list has now been
re-derived three times in this review (mine, yours, poursoul's), which is a sign it wants a
test rather than another round of enumeration.
Timing: this doesn't block K2 — the entries do refuse today, and the codes are the only
thing K2 would observe. If you'd rather land it as its own change, that's fine by me; it
needs an owner more than it needs to be in this PR. What I'd avoid is leaving it implicit,
because the cost grows with every entry added above an attach.
On the two holes you found yourself
ensure_acl_ready latching PROGRAM is the right call of the two you offered, and I
agree with the reasoning now in the comment. The alternative — refusing a kernel init when
acl_ready_ is already true — closes one direction and states the invariant as a special
case; latching makes the latch the single source of truth for both orderings, which is the
property the rest of the design rests on. That it also makes the refusal side-effect-free
(latch before the first ACL call) is the part I'd have missed.
Finding the else if unreachability yourself — a kernel finalize running aclrtResetDevice
aclFinalizeon the borrowed card wheneveracl_ready_was true — is the most valuable
single item in this round, from any of the three of us. It was live in the code I reviewed
and I read right past it.
Confirmations
scalar_count: deleting the field outright is better than either option I gave you.
The sentinel ambiguity I raised disappears rather than getting documented,make_callable's
signature stops breaking out-of-repo C++ callers, and the derived value is correct for
legacy blobs for free. poursoul's read ("the cached field was never load-bearing") was
the right one.reserved_: worth noting this doesn't conflict with my A2. I asked you to drop
reservedfields kept for version evolution; poursoul asked you to make the tail
padding explicit so an incompletely-initialized instance can't memcpy uninitialized bytes
to the AICPU. Different reasons, and his is right — I had written the implicit tail
padding off as harmless, which was the shallower read.- I verified the layout asserts, the
INVALID_ARGUMENTsplit, and that
kernel_execution_state.cppis out of all four platform CMakeLists.
Nothing here blocks merge from my side. Item 4 as I've framed it is a follow-up with an
owner, not a gate on this PR.
ChaoWao
left a comment
ChaoWao
left a comment
There was a problem hiding this comment.
Full review at 31b03d92d: API design, clean code, architecture
A pass over the three dimensions rather than a continuation of the item-by-item thread.
Merge-base c540572d. Where something was already covered in the earlier rounds I reference
it rather than restate it.
Summary up front. The documentation and invariant-statement quality here is above this
repo's average — execution_mode_latch.h spends 40 lines of comment on 30 lines of code and
earns them, covering why write-once, what no-rollback costs, and why the re-init permission
is asymmetric. Of the three dimensions, architecture is strongest, clean code is second, and
API design is the weakest — which matters disproportionately, because the API is this
PR's only deliverable. One item (1.1) I'd want resolved before merge; everything else is a
card.
1. API design
1.1 simpler_kernel_mode_supported(ctx) contradicts its own contract — worth fixing before freeze
The contract (runtime_c_api.h:591-597) says:
Callable on a context that
create_device_context()has returned but that no init has
touched … an implementation must answer from the runtime build and the device the
context names, never from init state.
But create_device_context(void) takes no parameters (runtime_c_api.h:235) and
device_id_ starts at -1. An un-inited context names no device. The contract requires
implementations to use information that the specified call timing guarantees is absent.
Both stubs confirm it — neither even names the parameter:
int simpler_kernel_mode_supported(DeviceContextHandle) { return 0; }So ctx is an argument no conforming implementation can legitimately use: the only
information available is which .so is loaded, and that is fixed at link time. This isn't a
typo — it only became visible once poursoul's 2.2 made you write the call timing down, which
is exactly what writing contracts down is for.
Three ways out; I'd take the first:
int simpler_kernel_mode_supported(void)— honest about the answer being a property of
the runtime build.ChipWorker's call site barely changes.- Keep the parameter and specify that implementations must ignore it — then it is a
placeholder for future use, which is the thing this repo's convention says not to add. - Move the probe after init so
ctxcarries a device — but that breaks the layering
ChipWorkernow has (probe first, then decide whether to resolve the rest).
A self-contradicting parameter on a frozen surface is much more expensive than no parameter.
1.2 finalize_device carries two contracts
Kernel mode is "four new entries plus the existing finalize_device", so that entry now
forks on the latch: program resets the device and aclFinalizes, kernel releases only
context-owned resources.
It is documented, but this is the other face of Item 4 — and subtler than the eleven
program-only entries. Those are "calling on the wrong context should be refused"; this one is
legal on both and does different things. I'm not arguing for a separate
simpler_kernel_mode_finalize (ABI surface +1, and the teardown logic genuinely overlaps) —
only that the doc should state the dual semantics as deliberate, rather than leaving readers
to infer it from the guards.
1.3 context_generation is caller-minted without a reason, and collides by name
simpler_kernel_mode_init takes uint64_t context_generation, contracted as a "nonzero
host-process-unique identity minted by the caller". The contract doesn't say why the
caller mints it. If it only needs process uniqueness, an atomic counter inside simpler
supplies it without pushing the obligation outward — and simpler can't validate what it
receives, so a caller that repeats a value goes undetected.
The name collision is the bigger risk: SimplerKernelInvocationHeader::generation is
documented as the residency slot's occupancy generation — "a property of the slot, not of
the callable in it". Two different generation concepts inside one API family. When K2 fills
in header.generation, these are the likeliest pair to cross. Suggest renaming the init
parameter context_id or context_token.
2. Clean code
2.1 ChipWorker's four function-pointer members are written and never read
kernel_supported_fn_, kernel_init_fn_, kernel_prepare_callable_fn_,
kernel_launch_fn_: grepping src/, every occurrence outside their declarations is either
an assignment or a null-out. Zero reads. They cost 4 using aliases + 4 members + 4
assignments + 4 sites × 4 lines of null-out, and buy nothing — they're private with no
accessor, so K2 can't reach them from outside either.
The thing that does work is the local kernel_supported_fn in init(), which gates the
lazy resolution. The members are pure future-proofing, and the K2 PR will add them naturally
when it has a caller.
This is the same reasoning you already accepted for moving kernel_execution_state.cpp out
of the four platform CMakeLists — just in a different place.
2.2 Function-pointer rollback is hand-repeated at four sites — and it already bit you once
init()'s three rollback paths plus finalize() each enumerate every function pointer by
hand. You reported the consequence yourself last round: "ChipWorker was leaving the four
new function pointers dangling into the .so that DlHandleGuard dlcloses, on all three
teardown paths." That bug's cause is this structure — adding a symbol means remembering
four places.
Collecting the pointers into a struct HostRuntimeFns { … }; turns all four sites into
fns_ = {}; and removes the bug class. Pre-existing, not yours to fix here — but worth a
card, since this PR is at least the second change to trip over it.
2.3 Half of ExecutionModeLatch's public surface is test-only, and one piece is a trap
Production uses latch() and is_kernel(). is_latched() and latched_mode() have no
callers outside the UT (test_kernel_execution_state.cpp:104/112/113/136).
And latched_mode() has an easy-to-misuse shape: before any latch it returns
SIMPLER_MODE_PROGRAM (the member default), with only a comment — "meaningful only once
is_latched()" — holding the line. A caller who forgets the guard reads "unlatched" as
"program", which is precisely the ambiguity write-once was introduced to remove.
Since production has no caller: delete them, or fold into
std::optional<SimplerExecutionMode> mode() const so "unlatched" can't be misread at the
type level.
2.4 create_thread doesn't use the seam you just extracted
std::thread DeviceRunnerBase::create_thread(std::function<void()> fn) {
int dev_id = device_id_;
return std::thread([dev_id, fn = std::move(fn)]() {
rtSetDevice(dev_id); // bare call, return value discarded
fn();
});
}bind_current_thread is defined 12 lines below and does the same job with a device_id < 0
check, a consistency check, and logging. Here rtSetDevice is called bare and its result
dropped; if device_id_ is still -1, rtSetDevice(-1) fails silently.
You added a comment explaining why this particular rtSetDevice is legitimate for a borrowed
context, so you clearly considered the site — but the new abstraction sits next door unused.
The lambda genuinely can't propagate an error outward, but it could still call
bind_current_thread and log the failure. Pre-existing; Consider.
3. Architecture
3.1 The allowlist/denylist mismatch is still there, and this round sharpened it
Same point as my second round, but the contrast is now starker:
| Technique | Status | |
|---|---|---|
KernelContextOps / KernelLaunchOps |
Allowlist — forbidden ops absent from the type | Zero production consumers; kernel_execution_state.cpp is now out of all four platform CMakeLists, UT-only |
8 is_kernel() guards |
Denylist — refuse point by point | Compiled into all 8 .sos; the defence actually in force |
Moving the state machine out of the shipped artifacts is right on its own terms — dead code
shouldn't ship. But it also means the half that provides structural guarantees is no longer
compiled into the product at all, while the half resting on enumeration is the entire real
defence.
The C ABI doc now encodes this as a contract: launch's doc says routing every runtime call
through KernelLaunchOps is "the implementation's obligation". Stating an invariant as an
implementer's duty rather than a structural constraint is the one place this design falls
back on discipline — everywhere else you structuralized it (write-once latch, shared
validation, the attach split). Worth being the thing K2's wiring is watched hardest on.
3.2 Refusal depth (Item 4 — detailed in my previous comment)
For the record: simpler_init latches as its first act, while simpler_prepare_run is
refused by attach_current_thread only after placement-new, try_reserve_native_run taking
the admission slot, and STRACE_CONTEXT. Fully rolled back, so not a live bug — correct by
ordering-plus-rollback rather than by construction. Suggested fix remains a uniform
first-act refusal per entry plus one table-driven test.
3.3 Dependency direction: an internal platform class pulls in the C ABI header for one constant
execution_mode_latch.h (platform/include/host/) includes runtime_c_api.h (worker/)
solely for PTO_RUNTIME_ERR_INVALID_STATE, which is why latch() returns int rather than
bool. An internal state class thereby absorbs the C ABI's error vocabulary into its own
interface.
This follows existing convention (KernelExecutionState does the same), so per
discipline.md §2 I am not asking you to change it here. Noting it because as this class
of internal type multiplies, the platform → worker back-edge calcifies; the eventual fix is
internal types returning their own result type, translated at the c_api_shared.cpp boundary.
3.4 Three judgements worth calling out as right
- Re-pointing
device_id_from "owns the device" to "which device this is on" is what
makes guards ② and ③ reachable at all, and you swept every read across both arches and both
backends before changing it. That was the substantive move this round. ensure_acl_readylatching PROGRAM rather than special-casing makes the latch the
single truth for both orderings, instead of encoding the invariant as "refuse a kernel init
whenacl_ready_is set".- The
bind/attach/adoptsplit separates thread binding, device-global watchdog
configuration, and identity recording, so the kernel path can take only what it's entitled
to. This is the seam K2 needs.
Priority
| Level | Item | Why |
|---|---|---|
| Before merge | 1.1 supported(ctx) contract contradiction |
Frozen surface; free to fix now, costs callers after K2 wires it |
| Worth doing here | 2.1 delete the four write-only members | Same reasoning you accepted for the CMakeLists removal |
| Worth doing here | 2.3 drop or optional-ize latched_mode() |
Trap shape, no production caller |
| Card | 1.3 rename context_generation |
Likeliest pair to cross when K2 fills the header |
| Card | 3.2 uniform entry-level refusal + table test | Ownership already under discussion |
| Note only | 1.2 / 2.2 / 2.4 / 3.1 / 3.3 | Partly pre-existing, partly K2 watch-points |
1.1 is the only one I'd hold merge for.
Review：四个探针的实测结果，以及一个必须在 K1 冻结前定掉的口子基线：head 先交底一句：我核过 1. 🔴 FREEZE 移除之后，E.4 的 DFX 分界没有载体了v3 采纳了我上一轮的 A1，把 context-control 整个拿掉。容量那一半有接盘的——改成在 arena 分配器内按 mode 无条件强制，#2193 顺着这个口径修拒绝路径。DFX 那一半没有。 实测： E.4 的实质是一条时间线：
没有 freeze，这条时间线塌成一个点，kernel 模式的 DFX 只能是恒开或恒关，没有中间态。这不是措辞问题，需要一个裁定。连带两处现在是悬空的：计划里 D1 的验收写的是"freeze 后只读导出"，⑤K3 的验收用例叫"DFX×freeze"。 为什么这条非要在 K1 里解决：K1 是冻接口的 PR。如果裁定结果是"kernel 模式需要一个 DFX 状态迁移点"，那它就是一个 ABI 入口；接口冻了再加，代价差一个量级。如果裁定是"恒关"或"恒开"，K1 不用加东西，但头文件里要写死这条契约，否则 K2/K3 会各按各的理解实现。 我没有立场替 DFX 定恒开还是恒关——只是这个选择现在没人做，而 K1 一合就贵了。 2. 🟠
|
| 臂（队列深 4000，每臂 40 次 × 4 轮） | 违例 |
|---|---|
每次现取 current_stream() |
0 / ~160 |
| ⑨binder 的 fork-join 形状，现取 | 0 / ~160 |
| 缓存 stream 指针 | 2 次 |
| 缓存 stream ＋ data_ptr | 2 次（读到 3997、3907，即 4000 次自增里有几次还没提交） |
（probe_f2_task_queue_ordering.py。注意 0/160 不是证明，机理也无文档依据——裸调用耗时 0.03–0.08 ms、取 stream ~0.1 ms，都远小于排空队列要的 ~30 ms，所以并不是"取 stream 会 flush"，更像是那点开销恰好够后台线程追上。不能拿它当契约。）
建议：在 runtime_c_api.h 的 launch 注释里写明「caller_stream 必须是本次调用当下的 current stream；实现方不得缓存，调用方也不得跨调用复用」。这是 K1 该做的事——它是唯一能让下游四个 PR 看到同一条规则的地方。
顺带一条给 ⑪K8 的（不在本 PR 范围，但结论是限制性的）：adapter 不应在 op body 里直接调 launch，应走 torch 自己的入队路径。上表是 K8"定序假设"的直接反证。
3. 🟡 "launch 期零分配"在运行期毫无执法
Probe C 用 raw aclmdlRICapture* 直驱、每个操作一个独立 capture、用 CaptureGetInfo 区分"当场拒绝／被捕获／静默污染"。结果里与 K1 契约相关的几条：
| 操作 | op rc | CaptureEnd | 判定 |
|---|---|---|---|
aclrtMalloc / aclrtFree |
0 | 0 | 不拒绝，图照常生成（GLOBAL 与 RELAXED 都一样） |
aclrtSynchronizeStream（捕获流）／SynchronizeDevice／StreamQuery（捕获流） |
107027 | 0 | 拒绝 |
aclrtMemcpy / aclrtMemset（同步） |
107030 | 0 | 拒绝 |
aclrtSynchronizeEvent / QueryEventStatus |
107028 | 0 | 拒绝 |
aclrtSynchronizeStream（别的流） |
0 | 0 | 放行（捕获期返回 0，但 replay 时不会发生） |
| wait 一个在捕获开始前 record 的 event | 107024 | 0 | 拒绝 |
| 侧流工作、完全不挂 event | 0 | 0 | 不进图、不报错，eager 跑掉一次 |
| fork 到侧流但不 join 回来 | 0 | 107025 | End 抓住，不给图 |
三条结论：
- 零分配不变式靠 CANN 是抓不到的，只能靠 ⑫ST 的禁止-API shim。K1 的头注释若要声明这条契约，应当同时写明它无运行期执法。
107024正是"等一个捕获外 record 的 event"。§0 第 6 行三分支里两个存活分支都明写"不 enqueue 旧 tail wait"——这条设计现在有硬证据了。而 kernel 模式的 event 是 context 常驻的，跨捕获边界等待是结构性会撞上的形状。- 危险的只有"漏挂 event"一种；"漏 join"反而安全（
CaptureEnd返 107025）。⑨binder 的失败闭包要防的是前者。
4. 附：两个被计划点名为排期闸的探针，已经跑完且是绿的
计划里 ⓿probe 至今无 owner，并被标为"全表唯一不缺技术前置、只缺人"、最迟 W-B 末必须出结论。其中两个我已经做掉：
Probe B′（计划语："最要紧""不通则整段返工，930 前窗口极窄"）：E.1 的双流形状——AICPU 专用流 ＋ AICore hidden 流，两条都只经 event 与调用方的流相连——能同时进同一次捕获，3/3 replay，零 ACL 错误。Probe C 里用 raw capture 又独立验了一遍（"两条侧流都 join 回来"那行）。目标二的单点风险以正面结果关闭。
Probe D（H7 分桶切换的前提）：多张图共享同一批 context 常驻 event/stream——交替 replay A B A B A A B 7/7、背靠背不同步 replay AB/BA/AA/BB 4/4、replay 之后在同一批 event 上跑 eager，全过。H7 的前提成立。
两条都限 a2a3；按非目标 #12，不得由 a2a3 推断 a5。a5 全部未跑。
剩下的 Probe E（handshake 最小失效区）和 Probe A2（rtFuncHandle 能否当 aclrtFuncHandle）我这边做不了——它们问的是 TMR 运行时自身的行为，得先构建 simpler，不是 ACL 层面能回答的。需要的话我可以在本机构建后补上。
小结：第 1 条是我唯一想在合入前看到结论的（因为它可能是个 ABI 入口）；第 2 条建议落成头文件里的一句话；第 3、4 条是给下游 PR 和 probe 卡的材料，不挡本 PR。
ChaoWao
left a comment
ChaoWao
left a comment
There was a problem hiding this comment.
Scope: KernelExecutionState belongs in ②K2, and there is now hard evidence
A scope test worth stating explicitly, because it settles several open threads at once:
Something belongs in K1 iff the four downstream PRs would each invent their own version
and disagree without it.
By that test most of this PR is core — the four entries, the shared validation helper, the 8
stubs, the 40-byte envelope, ExecutionModeLatch + the mode enum + the new error codes, the
dlsym surface, the ACL-lifecycle invariants, and the bind/attach/adopt split are all things a
downstream author has to agree with everyone else about.
KernelExecutionState and the two restricted vocabularies are not, and I can now show it
rather than argue it.
1. It is in no library
kernel_execution_state.cpp lives under src/, but grepping every CMakeLists and .cmake
in the tree, its only reference is:
tests/ut/cpp/CMakeLists.txt:524
${CMAKE_SOURCE_DIR}/../../../src/common/platform/shared/host/kernel_execution_state.cpp— an add_executable, not a library target. The file is in no .so. Its 24 UTs exercise
code that does not ship.
2. It is K2's subject, and K2 has already rewritten it
#2176 is "Add: persistent kernel-context execution resources (K2)", owner @YunjiQin, and its
base is main — so it carries its own copy rather than stacking on this branch. I diffed the
two:
src/common/platform/include/host/kernel_execution_state.h | 40 ++++++++++-----
1 file changed, 32 insertions(+), 8 deletions(-)
This is not drift, it is a redesign of the exact thing K1 freezes:
K1 (this PR): KernelEventKind { PrepareTail, Start, AicoreDone, SerialTail } // 4
K2 (#2176): KernelEventKind { Start, AicoreStart, AicoreDone, AicpuDone, SerialTail } // 5
PrepareTail is gone; AicoreStart and AicpuDone are new; KernelStreamKind's semantics
are corrected ("only Aicore is hidden in the capture sense; Aicpu is a dedicated private
stream"); create_event gains a creation-flag contract. And K2 carries a constraint K1 had
no way to know:
AicoreStartis recorded on the aicpu stream before the AICPU launch: the AICPU
orchestrator spins on AICore's handshake report, so anAicoreStartrecorded after the
launch could only fire once the AICPU task completed, which is a deadlock.
So the resource set K1 pins is not an under-determined contract that downstream needs handed
to it — it is a guess, and the implementation has already found it wrong. K1 cannot
validate it, because K1 has no path that latches KERNEL. Freezing it costs #2176 a conflict
resolution on code that has zero consumers here.
This also resolves the allowlist/denylist mismatch I raised two rounds ago in the better
direction. I had concluded "watch it during K2 wiring"; the right answer is that the
allowlist half simply isn't K1's, and moving it out leaves K1 with a coherent story — it
carries contract text and the guards that make program mode safe today, nothing that only
becomes reachable later.
Suggested action: drop kernel_execution_state.{h,cpp}, KernelContextOps,
KernelLaunchOps and test_kernel_execution_state.cpp from this PR; #2176 already has the
version informed by the real launch sequence. ExecutionModeLatch stays — it is consumed
here, by simpler_init and eight guards.
A scope rule worth writing into the PR description
The trajectory of this PR — five entries + ctx_control POD → four entries with
ctx_control removed → nalinaly's two items landed same-day — shows it growing by absorbing
review items. nalinaly explicitly offered the choice on both:
当前 kernel init 仍是 stub,所以这是能力启用后的潜伏缺陷,不是已复现的设备故障。建议在本
PR 修复;若明确留给后续接线 PR,也应作为启用 kernel 能力前的硬条件。
The choice was offered and never made; the default won. And your own reply gives the reason
it should have gone the other way: those tests pin the predicate plus the arena invariant
rather than actually running setup_static_arena, because K1 has no way to latch a context
KERNEL.
Proposed rule, worth stating in the description so it holds for the rest of the series:
K1 carries contract text, plus the guards required to make program mode safe today (the
ACL-lifecycle ones). Any latent defect that only becomes reachable once kernel mode is
enabled is registered as a hard precondition on ②K2 / ⑩a — not speculatively fixed in K1,
because K1 cannot construct a KERNEL context and therefore cannot verify the fix.
Applied to what has already landed: the ensure_acl_ready PROGRAM latch stays — it
governs what an unclaimed context does today, which is program-mode behaviour. The arena
refusal pre-scan would have been a precondition by this rule, but it is landed, small, and
carries a real-allocator counting test; I am not arguing to back it out, only that the rule
should stop the next one.
Self-check against the same rule
My own item 3.2 (uniform first-act refusal on the program-only entries, plus a table-driven
test) fails it. A test asserting "every program-only entry on a kernel-latched context
returns INVALID_STATE and leaves runner state unchanged" cannot be written in K1 — there
is no way to produce a kernel-latched context. I said it "doesn't block K2 and could land as
its own change", which was the right call for the wrong reason. Correctly filed: a hard
precondition on the PR that makes kernel init real, not a follow-up card of its own.
My items 1.1 (supported(ctx) contradicting its contract), 2.1 (the four write-only
ChipWorker members) and 2.3 (latched_mode()) all pass the rule — they are contract text
and deletions, verifiable today.
One coordination note: K1 vs ⑤K3 on the capacity invariant
#2193 is "kernel-mode capacity refusals no longer free what they protect (⑤K3)", base main.
Its own material is static_arena_bank.h + both arches' runtime_maker.cpp + the sim side —
a different layer from the device_runner_base.cpp this PR touches, so they do not collide.
But both describe the same invariant, and after K1 merges #2193 must rebase. Worth one line
somewhere: K1 owns the predicate and the onboard refusal ordering; K3 owns the arena bank
and the two runtimes. Nobody has written that down yet.
Net: I'd hold merge for 1.1 (the supported(ctx) contradiction) and the
KernelExecutionState removal. Everything else in my previous comment stays a card.
更正：撤回上一条的 §1（FREEZE／DFX 载体）上一条评论我把 §1 标成 🔴 并说"必须在 K1 冻结前定掉"。那条撤回，K1 在这件事上不需要做任何事。 我错在哪我问的是"kernel 模式的 DFX 是恒开还是恒关"。这是个假二分——我漏了第三种，而且它才是对的： DFX 做成常驻服务，device 侧使能由每次调用的
代码上这已经成立：五个通道的使能在 所以 E.4 不是"缺载体"，是前提写反了E.4 说「FREEZE 事务内拆 collector ＋ 释放 DFX workspace」——它假设 capture 之前必须把 DFX 拆掉。 常驻模型的要求正相反：collector 和它的 device 侧缓冲必须活着、地址稳定，capture 才能烧地址、replay 才能复用。capture 前拆掉才是会坏的那个做法。 于是结论是减法：
我原来那条如果被采纳，会往一个冻接口的 PR 里塞一个本来就不该存在的生命周期入口。请忽略它。 真正待做的在别处今天 kernel 模式下 DFX 其实根本不转——不是会出错，是不启动也不落盘： 修法是把括号从 run 作用域挪到 context 作用域（ 上一条的其余部分不变§2（ 完整模型、代码依据与落点见 #2202。 |
关于范围：一条建议移出，一条规则建议写明poursoul 已经 approve，我这条不是要挡合入。但 PR 在几轮 review 里从"五入口 ＋ context-control POD"长成了 28 文件 / +2157，其中一部分是靠吸收 review 意见长的，我想把范围这件事摆到台面上说一次，因为它影响的不只是这个 PR。 尺子计划卡对 K1 的定义是："一切的公共闸——K4/H2/K2 挂接段都按它的头文件草案开发"，且**"只声明、不创建任何资源"**。据此只需要一把尺子：
按这把尺子，绝大部分内容是过的——四入口、K9 头、 1. 建议把
|
31b03d9 to
a9cfea2
Compare
ChaoWao
changed the title
Kernel mode borrows the caller's device and stream for a bounded asynchronous operator. Define its C ABI, invocation envelope, and lifecycle contracts without enabling kernel execution in any backend. - Add the four simpler_kernel_mode entries, validating unsupported stubs, and distinct invalid-argument and invalid-state errors. Every host_runtime.so exports all four, so a consumer resolves them unconditionally like the rest of the uniform ABI; shipping stubs is already this repo's answer for an unsupported capability. The probe answers the capability question at call time and does not decide which symbols get resolved. - Latch the execution identity once. Program init latches PROGRAM before side effects; errors and finalization cannot change the mode. Separate thread binding, program attachment, and borrowed-device adoption, and guard the onboard device lifecycle by that identity. - Latch PROGRAM in ensure_acl_ready before its first ACL call. The entry is reachable without simpler_init, so taking ACL ownership is itself what makes a context a program context; leaving it unlatched would let a later kernel latch coexist with acl_ready_, whose finalize resets a device the context does not own. - Decide a kernel-mode arena capacity refusal across every region before touching any of them. The commit sequence rolls all regions back on failure, so a refusal raised from inside it would release the committed bases a captured graph still names. - Pin the 40-byte invocation header and every field offset. Make its trailing reserved bytes explicit and require both reserved fields to be zero. Payload validation belongs to the AICPU consumer. - Derive scalar counts from the callable signature. Preserve the existing factory signatures and serialized layout, with fixed callable offsets and tests that ignore historical padding bytes. - Scope simpler_kernel_mode_supported to what a caller can answer: the probe runs before any init, and create_device_context() takes no device, so an implementation answers from the runtime build alone. - Cover wire bytes, scalar counts, error contracts, the write-once identity latch, arena refusal over real committed regions, and dlsym failure recovery. Keep uniform-export checks for all eight components. Kernel init remains unsupported, so no production call latches KERNEL. The persistent execution resources -- hidden stream pair, event set and phase machine -- belong to the change that implements them rather than to this gate. They have no consumer here and ship in no library, and the branch already stacked on this one revises the event set from four entries to five, carrying an ordering constraint this change has no way to see. Production integration must also supply generation checks, safe enqueue/close serialization, and graph-resource lifetime enforcement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a9cfea2 to
df8fe43
Compare
4 participants
Summary
Add the shared kernel-mode C ABI and contracts for contexts that borrow the caller's device and stream. The program entry latches PROGRAM before changing runner or process state.
ExecutionModeLatchis write-once and idempotent for the same mode; failures and finalization never roll it back or permit a switch to the other mode.All current backends report kernel mode unsupported. This PR defines and tests the shared contracts; it does not enable kernel launches or allocate kernel execution resources in production.
Scope. This gate carries the surface the downstream PRs must agree on, plus the guards that make program mode safe today. The persistent execution resources — hidden stream pair, event set, and phase machine — are not here: they have no consumer in this change, ship in no library, and the K2 branch already stacked on this one revises the event set from four entries to five with an ordering constraint this change has no way to see. They belong to the change that implements them.
Contracts and implementation
simpler_kernel_mode_supported,simpler_kernel_mode_init,simpler_kernel_mode_prepare_callable, andsimpler_kernel_mode_launch. Kernel close reusesfinalize_device. Every built component exports all four, andChipWorkerresolves them unconditionally like the rest of the uniform ABI — shipping not-supported stubs is already this repo's answer for an unsupported capability, so the probe answers the capability question at call time rather than deciding which symbols get resolved.simpler_kernel_mode_supportedruns before any init, andcreate_device_context()takes no device, so an implementation answers from the runtime build alone;ctxis accepted for signature symmetry and must not be dereferenced for a device.host_copy_tensor_countandreserved_fields. Runtime-specific payloads follow it. Both wire endpoints are built together, without version negotiation.INVALID_ARGUMENT(-1004), explicit mode/lifecycle guards returnINVALID_STATE(-1003), and an unavailable capability returnsUNSUPPORTED(-1001).INTERNAL(-1000) remains reserved for internal invariants, including forbidden changes to committed kernel arena capacity.ChipCallable::scalar_count()and the Python read-only property derive the count from the signature throughcount_scalar_args. No scalar-count field, sentinel, or extra factory argument is added. The original C++/Python construction interfaces, callable sizes, and field offsets remain intact and are protected by layout assertions.bind_current_thread(thread binding),attach_current_thread(program ownership and watchdog setup), andadopt_borrowed_device(record a borrowed device). Mode guards read the single latch.ensure_acl_readylatches PROGRAM before its first ACL call, because that entry is reachable withoutsimpler_initand taking ACL ownership is itself what makes a context a program context. The sim arena has no kernel-only branch.A kernel-mode capacity refusal is decided over all three arena regions before the commit sequence begins, so it returns without reaching the rollback that would release the committed bases a captured graph still names. Program-mode rollback semantics are unchanged. No production entry latches KERNEL in this PR, so the onboard kernel branches remain unreachable until integration enables support.
context_generationis checked for nonzero by the stub but is not persisted by it.Testing
Verified on Linux against a worktree-local venv built with
build.targets=build_package_sim:test_execution_mode_latch.-m "not requires_hardware"), including the reworkedChipWorkerdlsym cases — an unsupported runtime still exports the whole family, and any missing entry fails init whatever the probe would answer.clang-format,ruff check,ruff format, andmarkdownlint-cli2clean on every changed file.CI has been re-triggered by the latest push; see the checks below for the authoritative result.