Add: enable direct-chip depth-two admission - #1750
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughL2 direct submission now supports one active run and one compatible prepared successor when the lane allows it. Native admission selects available runtime slots, retries incompatible successors after draining, and preserves depth-one fallback. Documentation and tests cover live completion fences and these execution paths. ChangesChip run admission and completion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Worker.submit
participant ChipRunLane
participant Runtime
Caller->>Worker.submit: submit direct run
Worker.submit->>ChipRunLane: enqueue run
ChipRunLane->>ChipRunLane: check compatibility and FIFO position
ChipRunLane->>Runtime: select slot
ChipRunLane->>Runtime: launch or prepare run
Worker.submit-->>Caller: return live completion fence
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ChaoWao
left a comment
ChaoWao
left a comment
There was a problem hiding this comment.
Review — W1c direct depth-two
Approve. I verified the claims independently rather than reading the PR body, and also ran the two suites the body does not mention.
Stated vs real goal
The PR body tracks implementation-plan.md §W1b-W1c almost verbatim — "one active plus one prepared successor", "third submit backpressures before preparation or frame mutation", "A5 and incompatible TMR remain depth one" — and the code does that. No goal downgrade, which is the failure mode I check for first. The plan's negative constraint also holds: git diff touches no Python FIFO, phase table, progress-owner election or poison state; the Python delta is two docstrings.
Mechanism, as I read it independently
Admission changes from always drain, then admit to drain only if the front cannot host a successor. The while loop is the backpressure: it spins the drain until either the FIFO is empty or the single front run is successor-compatible, and only then mints a generation and picks a slot. That ordering is what makes the "before preparation or frame mutation" clause literally true — on the third submit, g_prepare_count is asserted unchanged while the submitter thread is parked, which is a real concurrency assertion, not a linear-case one.
The second change is progress(): a non-front target now advances the front first. That is required, because with two runs admitted a caller can hold a handle to the successor and done()/wait() on it while the predecessor still owns the device. Recursion is bounded — FIFO is capped at 2, and the recursive call targets the front, which takes the non-recursive path.
What I verified, not assumed
| Claim | How checked | Result |
|---|---|---|
| 91/91 C++ UT | built + ctest -LE requires_hardware at PR head |
91/91 |
| depth-two engages on a2a3 TMR | concurrent_native_prepare_supported_impl returns 1 in a2a3/.../tensormap_and_ringbuffer, 0 in sim |
real, not a silent fallback |
block_on_front()'s "only the front can be LAUNCHED" invariant survives |
launch_front() only ever launches fifo.front() |
holds |
| recursion terminates | ./test_chip_run_lane under timeout 120 |
17/17, 0 ms |
Two suites the PR body does not claim, which I ran because this PR changes admission underneath them:
- Python UT — 1286 passed / 13 skipped, matching the W1b baseline. Worth stating explicitly:
_submit_l2_lockedis unchanged, so every Python-side W1b test now exercises the depth-two lane and still passes. - a2a3 onboard sweep under
task-submit— 56 passed + 24 passed / 2 skipped, 0 failures, with 122 TMR cases on real silicon. Same numbers as the W1b baseline.
One thing I checked and cleared, recorded so nobody re-derives it
The direct path picks a slot but never writes state_->generations[slot_id], unlike the leased path (:385). That looks like a missing update. It is not: direct submits pass pipeline_leased=false → admit_pipeline_generation=false, so prepare_native_run_on_slot deliberately skips pipeline_generations_.is_admissible. The direct generation is a private monotonic counter, not a lease the filter arbitrates. Pre-existing W1b behavior, correctly left alone.
Issues
Consider (non-blocking, no change requested):
The while loop calls drain_front() while holding state_->mu, and drain_front blocks on wait_native_run. So a third submit parks the lane mutex for the length of a device run — which is exactly what backpressure should do, but it also blocks done()/wait()/close() on other handles for that period. This is pre-existing (W1b's submit drained under the same lock), so it is not a regression, and the new test proves the intended blocking works. Flagging only because depth-two makes the window twice as likely to be hit: with two runs in flight there is now a handle that a caller may plausibly be polling while a third submit holds the lock.
Not raised as issues, deliberately: no missing-test finding (all three acceptance clauses are covered, including the fallback and the no-concurrent-prepare cases); no doc drift (both docstrings and the binding docstring were updated in the same commit, per doc-consistency.md §4).
Verdict
Approve. The scope matches the plan exactly, the negative constraints hold, and the acceptance row — active-plus-prepared, third-submit pre-prepare backpressure, runtime fallback — is covered by tests that assert the mechanism rather than the outcome.
Disclosure: I authored W1b (#1748), which this stacks on, so I reviewed the parts that touch my code against the invariants I wrote rather than against my memory of them.
My own review comment on hw-native-sys#1751 flagged a real gap: release_buffer()'s in-flight check only ever looks at self._accepted_run_handles, and _submit_l2_locked (worker.py, the direct-chip dispatch path submit() uses at L2) never adds its RunHandle there. create_buffer() only requires level >= 2, so an L2 Worker can hold a real, registered Buffer and dispatch chip runs against it with zero protection from release_buffer(). Checked how exploitable this is today: zero production callers do Worker(level=2).create_buffer() then race release_buffer() against a concurrent submit() -- this is a latent trap for future usage, not a firing bug. It doesn't explode today only because ImportRegistry.materialize() maps its own separate mmap for the identity, and POSIX unlink() only removes the shm's name -- an already-open mapping keeps working. That's an undocumented, untested coincidence, not a guarantee. Also corrected the assumption hw-native-sys#1751 shipped -- "L2 never needs this, a run completes synchronously inside submit()" -- which is false: the direct-chip lane permits one active plus one prepared compatible run, so up to two L2 runs can be in flight at once (per the W1b/W1c async-pipeline work, hw-native-sys#1748/hw-native-sys#1750). The real reason release_buffer()'s existing check can't see L2 runs is narrower: L2 uses a separate run-id namespace (self._chip_run_seq, tracked in self._chip_runs) and never touches _accepted_run_handles/_submit_mu -- _submit_locked returns from the L2 branch before reaching the with self._submit_mu: block. Rather than fold L2 into _accepted_run_handles (whose other readers -- _cleanup_bearing_predecessor, the live-handle scan for direct-control ordering, whole-run FIFO teardown draining -- carry L3-specific assumptions built around _orch-issued run ids and orchestration callbacks), this mirrors _chip_runs' own lifecycle with a parallel dict, _chip_run_touched_identities, added/removed at the same two points (_submit_l2_locked, _finalize_run_handle's L2 branch, and Worker.close()'s teardown) under the existing _registry_lock. release_buffer() now runs a second, independent check against it after the existing L3+ one. Extracted _identities_in_args as a shared static helper so _record_touched_identities (L3+) and the new L2 code walk tensor args the same way instead of duplicating the loop. New tests (tests/ut/py/test_worker/test_release_buffer.py): _submit_l2_locked records the touched identity (and an empty set for args=None); _finalize_run_handle clears it; release_buffer rejects while an L2 run's identity is present (registry entry survives the rejection) and succeeds once it's gone. Fixed two existing bare-Worker test helpers (test_create_buffer.py, test_remote_l3_lifecycle.py) that construct a Worker via __new__ and manually set internals -- they now also set the new dict. Verified: pytest tests/ut 1302 passed / 13 skipped / 0 failed; ruff check/format clean; a real a2a3 onboard run (pipeline_slots/test_pipeline_slots.py, an L2 direct-chip scene test) exercising the touched-identity walk on the real L2 dispatch path.
My own review comment on hw-native-sys#1751 flagged a real gap: release_buffer()'s in-flight check only ever looks at self._accepted_run_handles, and _submit_l2_locked (worker.py, the direct-chip dispatch path submit() uses at L2) never adds its RunHandle there. create_buffer() only requires level >= 2, so an L2 Worker can hold a real, registered Buffer and dispatch chip runs against it with zero protection from release_buffer(). Checked how exploitable this is today: zero production callers do Worker(level=2).create_buffer() then race release_buffer() against a concurrent submit() -- this is a latent trap for future usage, not a firing bug. It doesn't explode today only because ImportRegistry.materialize() maps its own separate mmap for the identity, and POSIX unlink() only removes the shm's name -- an already-open mapping keeps working. That's an undocumented, untested coincidence, not a guarantee. Also corrected the assumption hw-native-sys#1751 shipped -- "L2 never needs this, a run completes synchronously inside submit()" -- which is false: the direct-chip lane permits one active plus one prepared compatible run, so up to two L2 runs can be in flight at once (per the W1b/W1c async-pipeline work, hw-native-sys#1748/hw-native-sys#1750). The real reason release_buffer()'s existing check can't see L2 runs is narrower: L2 uses a separate run-id namespace (self._chip_run_seq, tracked in self._chip_runs) and never touches _accepted_run_handles/_submit_mu -- _submit_locked returns from the L2 branch before reaching the with self._submit_mu: block. Rather than fold L2 into _accepted_run_handles (whose other readers -- _cleanup_bearing_predecessor, the live-handle scan for direct-control ordering, whole-run FIFO teardown draining -- carry L3-specific assumptions built around _orch-issued run ids and orchestration callbacks), this mirrors _chip_runs' own lifecycle with a parallel dict, _chip_run_touched_identities, added/removed at the same two points (_submit_l2_locked, _finalize_run_handle's L2 branch, and Worker.close()'s teardown) under the existing _registry_lock. release_buffer() now runs a second, independent check against it after the existing L3+ one. Extracted _identities_in_args as a shared static helper so _record_touched_identities (L3+) and the new L2 code walk tensor args the same way instead of duplicating the loop. _submit_l2_locked publishes the touched-identities entry BEFORE calling _submit_chip_run_direct, not after: writing it post-dispatch left a window where a concurrent release_buffer() could see no entry at all for a run already running on the chip, since the entry that would have blocked it didn't exist yet. On a dispatch failure the entry is popped back out. _finalize_run_handle's L2 branch and Worker.close()'s teardown now take _registry_lock around every read/clear of _chip_runs and _chip_run_touched_identities (previously the membership check in _finalize_run_handle and the two clears in close() ran unlocked), using pop(..., None) so a concurrent close() can never make either raise. New tests (tests/ut/py/test_worker/test_release_buffer.py): _submit_l2_locked records the touched identity (and an empty set for args=None); _finalize_run_handle clears it; release_buffer rejects while an L2 run's identity is present (registry entry survives the rejection) and succeeds once it's gone; touched identities are visible to release_buffer() while dispatch is still blocked mid-call, proving the publish-before-dispatch ordering. Fixed two existing bare-Worker test helpers (test_create_buffer.py, test_remote_l3_lifecycle.py) that construct a Worker via __new__ and manually set internals -- they now also set the new dict. Verified: pytest tests/ut full suite passed; ruff check/format clean; pyright clean; a real a2a3 onboard run (pipeline_slots/test_pipeline_slots.py, an L2 direct-chip scene test) exercising the touched-identity walk on the real L2 dispatch path.
…#1757) My own review comment on #1751 flagged a real gap: release_buffer()'s in-flight check only ever looks at self._accepted_run_handles, and _submit_l2_locked (worker.py, the direct-chip dispatch path submit() uses at L2) never adds its RunHandle there. create_buffer() only requires level >= 2, so an L2 Worker can hold a real, registered Buffer and dispatch chip runs against it with zero protection from release_buffer(). Checked how exploitable this is today: zero production callers do Worker(level=2).create_buffer() then race release_buffer() against a concurrent submit() -- this is a latent trap for future usage, not a firing bug. It doesn't explode today only because ImportRegistry.materialize() maps its own separate mmap for the identity, and POSIX unlink() only removes the shm's name -- an already-open mapping keeps working. That's an undocumented, untested coincidence, not a guarantee. Also corrected the assumption #1751 shipped -- "L2 never needs this, a run completes synchronously inside submit()" -- which is false: the direct-chip lane permits one active plus one prepared compatible run, so up to two L2 runs can be in flight at once (per the W1b/W1c async-pipeline work, #1748/#1750). The real reason release_buffer()'s existing check can't see L2 runs is narrower: L2 uses a separate run-id namespace (self._chip_run_seq, tracked in self._chip_runs) and never touches _accepted_run_handles/_submit_mu -- _submit_locked returns from the L2 branch before reaching the with self._submit_mu: block. Rather than fold L2 into _accepted_run_handles (whose other readers -- _cleanup_bearing_predecessor, the live-handle scan for direct-control ordering, whole-run FIFO teardown draining -- carry L3-specific assumptions built around _orch-issued run ids and orchestration callbacks), this mirrors _chip_runs' own lifecycle with a parallel dict, _chip_run_touched_identities, added/removed at the same two points (_submit_l2_locked, _finalize_run_handle's L2 branch, and Worker.close()'s teardown) under the existing _registry_lock. release_buffer() now runs a second, independent check against it after the existing L3+ one. Extracted _identities_in_args as a shared static helper so _record_touched_identities (L3+) and the new L2 code walk tensor args the same way instead of duplicating the loop. _submit_l2_locked publishes the touched-identities entry BEFORE calling _submit_chip_run_direct, not after: writing it post-dispatch left a window where a concurrent release_buffer() could see no entry at all for a run already running on the chip, since the entry that would have blocked it didn't exist yet. On a dispatch failure the entry is popped back out. _finalize_run_handle's L2 branch and Worker.close()'s teardown now take _registry_lock around every read/clear of _chip_runs and _chip_run_touched_identities (previously the membership check in _finalize_run_handle and the two clears in close() ran unlocked), using pop(..., None) so a concurrent close() can never make either raise. New tests (tests/ut/py/test_worker/test_release_buffer.py): _submit_l2_locked records the touched identity (and an empty set for args=None); _finalize_run_handle clears it; release_buffer rejects while an L2 run's identity is present (registry entry survives the rejection) and succeeds once it's gone; touched identities are visible to release_buffer() while dispatch is still blocked mid-call, proving the publish-before-dispatch ordering. Fixed two existing bare-Worker test helpers (test_create_buffer.py, test_remote_l3_lifecycle.py) that construct a Worker via __new__ and manually set internals -- they now also set the new dict. Verified: pytest tests/ut full suite passed; ruff check/format clean; pyright clean; a real a2a3 onboard run (pipeline_slots/test_pipeline_slots.py, an L2 direct-chip scene test) exercising the touched-identity walk on the real L2 dispatch path.
2 participants
Summary
ChipRunLanefollow the runtimePipelineContract: compatible runs admit one active run plus one prepared successordone()and boundedwait()callsThis is W1c on top of #1748. It does not add a Python-side FIFO or change endpoint, RequestSession, profiling, or cross-repository behavior.
Testing
ctest --test-dir tests/ut/cpp/build-w1c -LE requires_hardware --output-on-failure(91/91 passed)ctest --test-dir tests/ut/cpp/build-w1c -R "^test_chip_run_lane$" --output-on-failureclang-format --dry-run --Werror src/common/worker/chip_run_lane.cpp tests/ut/cpp/hierarchical/test_chip_run_lane.cpp.venv/bin/python -m pyright python/simpler/worker.py