Add: kernel-mode C ABI skeleton, state machine, and wire headers (K1) - #2064
Add: kernel-mode C ABI skeleton, state machine, and wire headers (K1)#2064sunkaixuan2018 wants to merge 1 commit into
Conversation
|
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.
Kernel mode is simpler's second execution identity: instead of owning
the device, a context borrows the caller's already-current device and
stream to enqueue one bounded asynchronous operator per launch, so a
PyPTO program is capturable by ACLGraph as an ordinary node. This
change freezes the public surface that identity hangs off; it creates
no resources and changes no program-path behavior.
- runtime_c_api.h declares the lifecycle entries
simpler_kernel_mode_{supported,init,prepare_callable,launch} and adds
the host-band code PTO_RUNTIME_ERR_INVALID_STATE. The existing
finalize_device stays the fifth lifecycle entry. The execution mode
is claimed by whichever init entry runs first (ExecutionModeClaimState
on the platform runner) — the single mode source every kernel-mode
guard keys on, so the guards arm the moment kernel init is accepted,
with no separate declaration call to forget. Kernel-mode capacity is
a mode invariant rather than a gated state: config is context-static,
so each pooled arena region is committed at most once, and
setup_static_arena reports a grow or release request on a committed
region under kernel mode as an internal invariant break; capacity
intent travels in CallConfig.runtime_env like everywhere else.
- ensure_acl_ready(), force_reset_device(), and finalize()'s rt-layer
device reset refuse on a kernel-mode context (a2a3 + a5): the ACL
lifecycle belongs to the caller, and every call site of the five ACL
lifecycle APIs falls into three enumerable classes (below the
ensure_acl_ready guard, inside force_reset_device behind its own
guard, or gated on acl_ready_ which only the guarded path sets), with
finalize's rt-layer reset intercepted by its own kernel-mode branch —
so poison recovery can never reset the device out from under the
host process.
- kernel_invocation_header.h pins the envelope every kernel launch
ships to the AICPU (mode / callable / generation / payload length /
int32_t arg counts). Both sides of the wire come from one
build_runtimes.py build, so the struct carries no version or size
negotiation; the POD/standard-layout guards remain. ChipCallable's
sig_count includes the scalar entries, so the consumer comparanda are
sig_count - scalar_count for tensors and scalar_count for scalars.
- Kernel-entry argument validation is shared by all eight host-runtime
components through kernel_entry_validation.h (one copy of the
null/range/image-size checks; a binary pointer and its size must be
present or absent together), so a stub and a real implementation
accept and reject exactly the same arguments.
- KernelExecutionState and ExecutionModeClaimState carry the kernel
context phase machine (New/Collecting/ReadyEnqueued/Poisoned/
Closing/Closed with sticky, retriable Closing and separate
runtime-error and teardown-error slots) and the two restricted
operation vocabularies; synchronize, allocation, capture queries,
and model attachment stay unrepresentable in those tables, and a
launch implementation is obligated to route through them.
- ChipWorker dlsyms the four new symbols from every runtime, so a
component missing one fails at load. test_host_runtime_abi.py
asserts the export across all eight components, and table-driven UTs
cover the phase machine (including failed-rollback landing in
Closing with the create error reported and the cleanup error
latched), the shared argument validation, and the wire layout.
- ChipCallable additionally records scalar_count as a cached
derivation of the signature's SCALAR entries: make_callable rejects
a nonzero count that disagrees with the signature, while 0 also
means "not recorded" (legacy blobs read 0). The field occupies four
bytes of historical header tail padding, so every historical offset,
sizeof, and the kernel-cache ABI token are unchanged;
ChipCallable.build gains a trailing scalar_count=0 keyword and a
read-only property.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
2 participants
Motivation
simpler today has exactly one execution identity: program mode, which takes exclusive ownership of the device. Kernel mode is the second identity — a context that borrows the caller's already-current device and caller-owned stream to enqueue one bounded asynchronous operator per launch: no device reset, no internal stream/device synchronize on the prepare/launch/close paths, zero allocation at launch, and no capture/model-state queries, so a launch is capturable by ACLGraph as an ordinary node.
This PR is K1, the single public gate the rest of the kernel-mode pipeline hangs off. It freezes the surface — entry points, the invocation wire envelope, the context state machine, and the restricted operation vocabularies — so the runtime-specific work (invocation snapshots, HBG launch blobs, persistent state) can be developed against it in parallel. It deliberately creates no resources and changes no program-path behavior: every guard keys on a kernel-mode claim that nothing in this PR can yet establish.
Surface frozen by this PR
Four lifecycle entries (
src/common/worker/runtime_c_api.h), familysimpler_kernel_mode_*:simpler_kernel_mode_supported/simpler_kernel_mode_init/simpler_kernel_mode_prepare_callable/simpler_kernel_mode_launch; the fifth lifecycle entry is the existingfinalize_device(in kernel mode it releases only context-owned resources — neverrtDeviceReset/aclFinalize).ExecutionModeClaimStateon the platform runner (mutually exclusive, idempotent, abortable on init failure). Every kernel-mode guard keys on that claim, so the guards arm the moment kernel init is accepted — there is no separate declaration call a caller could forget.configis context-static, so each pooled arena region is committed at most once and never grown or released afterwards.setup_static_arena'scommit_region(onboard and sim) reports a grow or release request on a committed region under kernel mode as an internal invariant break (PTO_RUNTIME_ERR_INTERNAL); capacity intent travels inCallConfig.runtime_envlike everywhere else.ensure_acl_ready(),force_reset_device(), andfinalize()'s rt-layer device reset refuse on a kernel-mode context (a2a3 + a5) — the caller owns the ACL lifecycle, so poison recovery can never reset the device out from under the host process. Coverage is enumerable: every call site of the five ACL lifecycle APIs is below theensure_acl_readyguard, insideforce_reset_device()behind its own guard, or gated onacl_ready_(which only the guarded path sets), with finalize's reset intercepted by its own kernel-mode branch.PTO_RUNTIME_ERR_INVALID_STATE(-1003, out-of-order lifecycle call).Unified invocation envelope (
src/common/task_interface/kernel_invocation_header.h):SimplerKernelInvocationHeader, the 40-byte header every kernel-mode launch ships to the AICPU — mode / callable_id / generation / payload length /int32_targ counts — plus theSimplerExecutionModeenum (its one wire consumer). Both sides of this wire come from the samebuild_runtimes.pybuild, so the struct carries no version or size negotiation; the POD/standard-layout guards remain.ChipCallable::sig_countincludes the scalar entries, so the consumer comparanda aresig_count - scalar_countfor tensors andscalar_countfor scalars. Runtime payload formats are defined under the header by each runtime, and its fail-closed validation belongs to the AICPU-side consumers.Shared entry validation (
src/common/platform/include/host/kernel_entry_validation.h): one copy of the null/range/image-size checks for init/prepare/launch, compiled into all eight host-runtime components — a binary pointer and its size must be present or absent together — so a stub and a real implementation accept and reject exactly the same arguments.State machine and restricted vocabularies (
src/common/platform/{include,shared}/host/kernel_execution_state.*):KernelExecutionState:New → Collecting ⇄ ReadyEnqueued, partial-enqueue failure →Poisoned(only close accepted), close → sticky retriableClosing→Closed. Two separate error slots (first poison cause vs. first real teardown failure) so a controlled error can never mask a teardown failure. An init that fails and whose rollback also fails lands inClosingwith the create error reported and the cleanup error latched, retriable by explicit close.ExecutionModeClaimState: the mode single-source above; wired into both runner bases and consumed by the guards. The kernel init implementation'sclaim_kernel()call lands with the persistent-state PR.dlsym + stubs: the four symbols join
ChipWorker's mandatory dlsym table, so all 8 host-runtime components must export them; a component missing one fails at load. In this PR every component is a conservative skeleton:supportedreturns 0,initreportsUNSUPPORTEDafter the shared structural validation,prepare/launchreportINVALID_STATE("no live kernel context").Also included:
scalar_countin the ChipCallable headerThe first part of this commit (originally this PR's sole content) makes the compiled artifact record how many scalar arguments its orchestration expects — the callable-side comparandum for the invocation header's
scalar_count. The field is a cached derivation of the signature:make_callablerejects a nonzero count that disagrees with the signature'sSCALARentry count, while 0 also means "not recorded" (legacy blobs read 0; legacy and no-scalar artifacts stay indistinguishable by design).int32_t scalar_count_occupies four bytes of historical tail padding, so every field offset,sizeof(ChipCallable)(9376), the kernel-cache ABI token, and legacy blobs are unchanged;ChipCallable.buildgains a trailingscalar_count=0keyword plus a read-only property, and static_asserts pin both ChipCallable and CoreCallable layouts byte-for-byte.Review-driven design decisions (2026-09-08)
simpler_kernel_mode_ctx_controlentry with a CONFIGURE/FREEZE state machine. Review showed both payloads already have single sources — mode is decided by which init ran, capacity byCallConfig.runtime_env— and that a freeze gate is weaker than enforcing the invariant unconditionally in the arena code (a gate depends on the caller remembering to call it). The entry, its wire struct, its state machine, andPTO_RUNTIME_ERR_CAPACITY_EXCEEDEDwere removed; the guarantee moved intocommit_regionas described above.abi_version/header_bytes/ reserved words guarded nothing; evolving the struct means changing both sides in one tree. Thescalar_countcompatibility work is the deliberate exception —ChipCallablehas an on-disk kernel cache, so its cross-build reasoning and tests stay.SimplerExecutorBinariesstruct and backportingcallable_sizetosimpler_register_callableboth modify existing program-path ABI, which this PR's hard rule forbids; they belong to their own changes.Known debts (accepted, tracked)
The kernel-mode guards (
ensure_acl_ready/force_reset_device/ finalize's reset skip / thecommit_regioninvariant) are exercised by no test that goes through a real.so, because nothing in this PR can put a context into kernel mode. The persistent-state PR that flips the capability must add the rejection tests for all four guard sites, plus an ABI-level case that a kernel claim arms them.Tests
tests/ut/py/test_host_runtime_abi.py: the four symbols asserted exported on all 8 components.tests/ut/cpp/common/test_kernel_execution_state.cpp(23 cases): full phase × entry table, init/close balance with fake ops, partial-init rollback (clean and failed-rollback-to-Closing), poison first-cause latching, stickyClosingretry, separate teardown-error slot, claim mutual exclusion/abort/closed.tests/ut/cpp/common/test_kernel_entry_validation.cpp(5 cases): every structural rejection for init/prepare/launch, including both directions of the binary-span consistency check.tests/ut/cpp/types/test_kernel_invocation_header.cpp(3 cases): pinned mode values, memcpy round-trip, zero-blob semantics.tests/ut/cpp/types/test_callable_scalar_count.cpp(5 cases): factory round-trip, range rejection, signature-disagreement rejection, legacy blob reads 0, only-the-field-bytes-vary.tests/ut/cppand fulltests/ut/pygreen on the validation host; this PR's CI runs the full matrix.Commits
Single squashed commit on top of
main.