Repository navigation
fix(cpu-ep): stop nested_dispatch_slot_pressure aliasing the whole row - #1385
justinchuby wants to merge 2 commits into
Conversation
ea372cc to
1b027ec
Compare
|
Confirmed — this is my defect from #1377, and the diagnosis is exactly right. The comment I wrote ( One adjacent observation to hand over rather than leave buried. Re-running this test on latest main from my phase-21 harness work, Thanks for catching it. |
1b027ec to
fc7e144
Compare
|
Held as Draft deliberately, and stacked on #1382 rather than
#1382 has auto-merge armed, so it will land as soon as required CI clears the queue; GitHub will then retarget this PR to Local verification on the stacked head ( |
68b0c9e to
5c9a6a5
Compare
fc7e144 to
016dfed
Compare
|
Heads-up, and an apology for the overlap: I have opened #1407 carrying your fix, because this PR is still a draft with auto-merge off and the UB is on The other half is worth flagging to you specifically, because it explains why your defect survived review and CI in the first place. CI's Miri lane runs That was not enough, and the failure is the interesting bit: my new test passed under Miri with the bad retag still in it. Probing the pool under Miri: Miri reports I had written a canary that passes for a reason unrelated to its claim, which is exactly the class you named here. Fixed two ways: the test now asserts it actually fanned out ( Verified both directions:
The middle row is the one that matters: without the flag, the falsifier does not falsify. Your standalone Miri repro caught it because you drove the threads directly rather than going through the pool. Thanks again for finding this. |
5c9a6a5 to
8399cf5
Compare
016dfed to
9998660
Compare
8399cf5 to
4ef749e
Compare
9998660 to
c273cb5
Compare
`prefill_fan_out` and `WIDE_PREFILL_MACS` carried `#[cfg_attr(not(feature = "mlas"), allow(dead_code))]` because their only non-test caller, `run_mlas_shards`, is `#[cfg(feature = "mlas")]`. #1363 added a second caller inside `borrowed_affine_int4_matmul_prefill` and removed both guards. That function is `#[cfg(target_arch = "x86_64")]`, so the guards were only redundant on x86_64. On aarch64 without MLAS both callers disappear again and the items are dead, which is an error under the `-D warnings` the ARM64 lanes build with. `prefill_column_grain` shipped new in the same PR with no guard and only that one x86-gated caller. Restore the guards over the union of the two callers' cfgs. No behaviour change on any target: `allow(dead_code)` only applies where the item already has no caller. Caught by `scripts/check_cross_compile.sh`, which is a blocking `Rust quality` step but had not run on main — every CI run since #1363 merged is still queued. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4ef749e to
32834e7
Compare
The nesting probe added in #1377 reconstructs its row inside each task as std::slice::from_raw_parts_mut(base as *mut u64, NEST_INNER) so every concurrent task materialises a `&mut [u64]` over the *entire* row and then indexes `[start..end]`. The writes are disjoint but the references are not, which is a data race under Stacked Borrows. The comment above it already claimed "the reconstructed slice never overlaps another task's" — that was the intent, not what the code did. Reconstruct only `start..end`, which is the shape production already uses in `parallel_output_rows_repeated`. Miri, minimal reproduction of both shapes: whole-row (before): error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `unnamed-1` and (2) retag write of type `[u64]` on thread `unnamed-2` own-range (after): clean Test-only and `#[ignore]`d, so nothing shipped was affected, and the Miri lane covers `--lib task_runtime::` rather than this integration binary, which is why neither caught it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c273cb5 to
a93c635
Compare
|
Status as of 2026-08-19T07:31Z, still held as Draft on purpose. Why it is not ready yet: it is stacked on #1382, whose required checks ( Exact steps once #1382 merges (GitHub will auto-retarget this PR to
Verified on the current head |
32834e7 to
1799bdc
Compare
|
Superseded by #1407, which fixes the same I independently validated #1407 end-to-end and posted the evidence there (#1407 (comment)): the fix is correct, its falsifier reproduces exactly, and its integration step passes in 6.78s. It has one real flaw — the new lib step is missing No reason to carry two PRs for one defect. Closing this in favour of the stronger one. My original evidence stays on the record here: whole-row retag → Stacked-Borrows UB on 8/8 seeds and a Tree-Borrows data race; narrowed → clean under both. Not merged, not bypassed — closed unmerged. |
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
…e it (#1407) ## The UB `nested_dispatch_slot_pressure` (which I added in #1377) reconstructs a `&mut [u64]` over the **entire** row in every task and only then narrows: ```rust let row = unsafe { std::slice::from_raw_parts_mut(base as *mut u64, NEST_INNER) }; for slot in &mut row[start..end] { *slot += 1; } ``` The *stores* are disjoint. The *retags* are not. Under Stacked Borrows the violation is the retag: each task pushes a `Unique` covering all of `NEST_INNER`, popping the previous task's tag. Narrowing before the retag fixes it, and is the shape `parallel_output_rows_repeated` already uses in production. **Diagnosis and fix are Pris's, from #1385.** This PR carries them because that one is still a draft with auto-merge off and the UB is on `main` today. If #1385 goes ready first, close this and I will rebase the Miri half onto it — the two halves are independent. ## Why it survived, which is the part worth keeping CI's Miri lane runs `-p onnx-runtime-ep-cpu --lib task_runtime::`. **Integration tests under `tests/` are never Miri-checked at all.** Fixing this one instance would have left that gap open for the next one, so this also puts the shape under the lane as a lib test. That was not sufficient either, and the first attempt is the useful part: **the new test passed under Miri with the bad retag still in it.** Miri reports `available_parallelism() == 1`, so `resolve_width` builds a one-lane pool, every fan-out returns `Backend::Serial`, and no two tasks ever run against each other. Probe under Miri: ``` pool_width=1 backend=Serial tasks=1 ``` The lane has been type-checking the unsafe blocks in this module without exercising the concurrency they exist for. The workflow comment claims it "runs real threads under Stacked Borrows" — it runs one. So I would have shipped a canary that passes for a reason unrelated to its claim, which is precisely the class Pris named in #1385. Two fixes: 1. **The test asserts it actually fanned out** (`Backend::Native`), so it fails loudly if it ever degenerates to one task instead of passing silently. 2. **A dedicated lane step with `-Zmiri-num-cpus=4`**, scoped to that single test rather than all of `task_runtime::` — multi-CPU Miri multiplies the runtime of what is already the slowest step in the lane. The targeted step costs **18s**. ## Verified in both directions | retag shape | `-Zmiri-num-cpus=4` | result | | --- | --- | --- | | whole row (**#1377 as merged**) | yes | `error: Undefined Behavior: Data race detected between (1) retag write on thread task_runtime::t and (2) retag write of type [u64] on thread nxrt-task-0` | | whole row | **no** (lane default) | **passes** | | own range (**this PR**) | yes | passes | The middle row is the finding: without the flag the falsifier does not falsify. ## Scope Test-only plus one workflow step. No production change — `for_each_chunk_mut` was always correct. 30/30 `task_runtime::` lib tests pass natively and under Miri; the repaired benchmark still runs (`1 of 30 dispatches declined`); fmt and clippy clean. Includes the one-line rustfmt repair of `onnx-runtime-ep-cuda/src/runtime.rs` that `main` is currently failing on (same as #1393/#1395/#1398 — whichever lands first makes the rest a no-op). 🤖 Generated with [GitHub Copilot CLI](https://github.com/features/copilot/cli) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
> **Process note, stated up front.** This defect entered `main` via #1363, which I merged with an admin bypass while every required check was still `queued`. That was wrong, I am not repeating it, and this PR goes through the normal gates. Full disclosure of what I bypassed is in the comment below. `parallel_output_rows_dispatches_to_the_task_runtime` **fails on every stock CI runner** and is live on `main` today. ## The defect The test asserts the flat fan-out reaches the task runtime. But routing reads `rayon::current_num_threads()`, and `flat_fan_out`'s *first* gate is deliberately "stay on Rayon below `MIN_ROUTED_FAN_OUT_WIDTH` (16)". Below that width the test asserts something policy never promised. Measured on unrepaired `main`: | `RAYON_NUM_THREADS` | 4 | 8 | 15 | 16 | 32 | |---|---|---|---|---|---| | result | **FAILED** | **FAILED** | **FAILED** | ok | ok | `ubuntu-latest` is 4 vCPU. The whole `onnx-runtime-ep-cpu` lib suite on unrepaired main at that width: ``` test result: FAILED. 1447 passed; 1 failed; 17 ignored kernels::matmul_nbits::tests::parallel_output_rows_dispatches_to_the_task_runtime ``` It passed for me only because this development host is 16C/32T — the defect needs a *narrower* machine to appear, which is exactly the kind of thing the CI I bypassed exists to find. The existing `task_runtime::width() <= 1` guard does not cover it: task-runtime width and Rayon width are different numbers, and on a 4-vCPU box the first is `> 1` while the second is `< 16`. ## The fix, and the trap in it Install a Rayon pool of exactly the routing width so the decision under test is host-independent. My first attempt only wrapped the fan-out — and **still failed at `rayon=1`**, because `output_chunk_len` reads the same Rayon width and the test's *precondition* carried the identical defect. Moving the precondition inside the pool too is what actually removes the host dependency rather than relocating it. Skipping below the threshold would have been the weaker fix: the test would silently no-op on every real runner and guard nothing. - passes at rayon = **1, 2, 4, 8, 15, 16, 32** - **still falsifies** — forcing `PrefillFanOut::Wide` makes it fail, so it is not vacuous - adds the coverage assertion it should always have had (every output row written exactly once) ## Scope Test-only. No production behaviour changes. Deliberately **not** included: - the **aarch64 dead-code break** #1363 also shipped (`WIDE_PREFILL_MACS`, `prefill_fan_out`, `prefill_column_grain` are dead on a non-mlas ARM64 build, failing `-D warnings`) → **#1382** by @pris was open first and is already armed. I had written the same three `allow(dead_code)` restorations, verified they clear `cargo clippy --target aarch64-unknown-linux-gnu -- -D warnings`, then dropped them from this branch rather than ship a conflicting duplicate. - the **Stacked-Borrows UB** in #1377's test → **#1385** by @pris, and **#1407** (mine, which additionally closes the Miri lane gap that let it through: `miri.yml` runs `--lib task_runtime::` only, so integration tests under `tests/` are never Miri-checked). ## Validation | check | result | |---|---| | `cargo fmt --all -- --check` | clean | | `cargo test -p onnx-runtime-ep-cpu --lib` (host width) | 1447 passed, 0 failed | | `cargo test -p onnx-runtime-ep-cpu --lib` at `RAYON_NUM_THREADS=4` | **1447 passed, 0 failed** (main: 1 failed) | | target test at rayon 1/2/4/8/15/16/32 | all pass | | falsification probe (force `Wide`) | fails as required | Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## squad/pris-1363-aarch64-dead-code #1385 +/- ##
====================================================================
Coverage ? 82.93%
====================================================================
Files ? 12
Lines ? 5584
Branches ? 5584
====================================================================
Hits ? 4631
Misses ? 760
Partials ? 193
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
The defect
nested_dispatch_slot_pressure, added by #1377, reconstructs its row insideeach task like this:
Every concurrent task materialises a
&mut [u64]over the entire row andonly then narrows to
[start..end]. The writes are disjoint; the referencesare not. The violation is the retag, not the store:
from_raw_parts_mutasserts unique access over the full
NEST_INNERrange while sibling tasks arewriting into it.
The comment is the tell — "the reconstructed slice never overlaps another
task's" describes the intent, not what the code does.
The fix
Reconstruct only
start..end, which is the shape production already uses inparallel_output_rows_repeated(crates/onnx-runtime-ep-cpu/src/kernels/matmul_nbits.rs:4215):The
SAFETYcomment now states the invariantfor_each_rangeactuallyprovides (
task_range,src/task_runtime/mod.rs:325-331, partitions0..totalinto disjoint half-open ranges, exactly once each, anddispatchblocks until all complete) and says why the whole-row spelling is wrong.
Evidence
The two shapes isolated into standalone binaries — four scoped threads over a
256-element row, no other differences — and run under Miri. Reproduce with:
whole(#1377 as merged)range(this PR)Mirilane pins)-Zmiri-tree-borrows)Stacked Borrows, default flags,
-Zmiri-seed=0..7, identical every time:Tree Borrows reports the same defect as a race:
The tag is
<wildcard>because the pointer is laundered throughusize: theas usizecast exposes the provenance, and casting back yields a pointerMiri can no longer tie to a specific tag, so it substitutes the permissive
wildcard. That weakens Miri's tracking — it is why
-Zmiri-strict-provenancerefuses to run this probe at all — but it does not save this shape: the retag
still cannot claim a range a sibling thread is concurrently writing.
Impact and why nothing caught it
parallel_output_rows_repeatedwas always correct — I checked everyfrom_raw_parts_mutincrates/onnx-runtime-ep-cpu/and this test is theonly whole-buffer reconstruction inside a concurrent task.
#[ignore]d, so it only runs when invoked deliberately.Mirilane runscargo miri test -p onnx-runtime-ep-cpu --lib task_runtime::— the library unit tests, not this integration binary — so it was never in
scope. This PR does not widen that lane; doing so would be a separate change
with its own runtime cost, and is worth considering separately.
Found while independently validating the merged scheduler wave (#1232, #1238,
#1154, #1363, #1374, #1377), which went in under admin bypass during runner
saturation.
Note on the instrument
My first version of this probe selected the shape with an environment variable.
That was wrong and briefly gave me a false result in both directions: Miri
isolates the environment by default, so
std::env::varreturnedErrand thebinary silently always took one branch. Adding
-Zmiri-disable-isolationto"fix" that then made both shapes report UB, because the process was still
executing the
wholebranch. Splitting the two shapes into separate binarieswith no runtime branch removed the ambiguity, and the table above is from that
version. Recording it because a probe that appears to answer the question while
actually testing something else is the same failure mode as the bug itself.
Validation
Local, clean worktree, rebased on #1382.
cargo test -p onnx-runtime-ep-cpu --release --test task_runtime_latency -- --include-ignored— 2 passed, 0 failed; the probe still reports its table (0 of 31
dispatches declined at 1/2/4/8/16 dispatchers), so test(cpu-ep): measure nested dispatch slot pressure in the task pool #1377's published
measurement is unchanged.
cargo clippy --all-targets -p onnx-runtime-ep-cpu -- -D warnings— clean.cargo fmt --all -- --check— clean.