Repository navigation
G15: fix stamp wrap and VM slot exhaustion - #233
erykwieliczko wants to merge 2 commits into
Conversation
maralcbr
left a comment
There was a problem hiding this comment.
Thanks @erykwieliczko, this is a careful fix and the CTS evidence is strong. The stamp change is right: stamp_seed now follows the firmware's 2^24-step cycle through zero (encode.rs:68-70), and every place that used 0 to mean "no wait" or "done" is now an Option (engine.rs:161,392, recovery/logic.rs:148-152, poll.rs:506-525). I found no zero sentinel left on stamps or targets in g15/. The compute cell's initial 0 (graph.rs:255) can't read as reached, because the range only spans zero after about 4.1M computes. Per-publication binding also holds up. Publication only appends behind flights of the same VM (engine.rs:566-575), so a horizon j2.bind() reuses the VM's binding and can't hit ENOSPC. Bindings drop only once next == sent, and an unbound VM's unmaps still invalidate its last slot through bind_token (mmu.rs:752-771), so reusing a token is safe.
One thing before merge:
- Three G15 host suites no longer build or pass. CI's "host tests (tools)" job fails on
tools/asahi/g15-input-error-tests.py,g15-zero-words-tests.pyandg15-pb-growth-tests.py. I get the same result locally with thehost-tests.jsonarguments: all three exit 1 at5782625d6e19and 0 at13c3371ce23d. They extract the production files, so their mocks need to follow the new interfaces:g15-pb-growth-tests.py:626-642: the mockVmStateneedsidand the mockJobneedsslot(), sincepools.rs:628and:916now use them (E0609/E0599 today).g15-input-error-tests.py:63: the mockJobneedsrelease_binding(), whichjob.rs:152now calls. The control fails as NOBUILD.g15-zero-words-tests.py:29: please refresh thehw.rshash, with a note that the change only removesMAX_VMS. Keep the T6030 function hash at:33; it still matches.
Smaller things:
- The "until reboot" wording is too broad (
vm.rs:16-17,uat.rs:28-29). Only jobs retained after a fail-stop keep their slot until reboot. A recovered hang's failed jobs aren't retained (recovery.rs:351-353), so their binding goes when the scheduler drops the last reference, as the comment atengine.rs:684-685already says. Suggest: "failed published jobs keep it until their last reference drops (until reboot for jobs retained after a fail-stop)". - nit:
rustfmt --checkwants braces around the compute arm atengine.rs:923.
Behaviour changes to note in the PR body: VM creation is no longer capped at 63 (MAX_VMS is gone), and with robust_isolation=1 many VMs now share the one slot with an ASID flush per switch, instead of the second VM getting ENOSPC.
Checked: tools/testing/selftests/drivers/gpu/asahi-g15/run.py passes locally (76 ok, 2 ignored, including the new firmware_stamp_wrap and progress_at_zero_wrap). check-commits.py is clean for both commits. In CI, package, KUnit, boot, compile, device trees and the aurora-sep host tests pass. Only the tools group fails, with the three suites above. With those fixed and the tools group green, I'll approve. For release, not this merge: we have no M3 in our lab, so G15 hardware coverage rests on your M3 Pro runs. A short smoke test on a second G15 machine before release would help.
| //! lives; at most [`super::hw::sizes::MAX_VMS`] such VMs (ENOSPC beyond, checked before | ||
| //! binding so the slot allocator never blocks). | ||
| //! - leave it unbound until a runnable job needs a TTBAT slot. Each published job holds | ||
| //! its binding through completion; failed published jobs retain theirs until reboot. |
There was a problem hiding this comment.
Only fail-stop retained jobs keep the binding until reboot. A recovered hang's failed jobs release it when their last reference drops (recovery.rs:351-353). Suggest "until their last reference drops (until reboot for jobs retained after a fail-stop)".
| //! User VMs are bound to exclusive TTBAT slots for their lifetime through `mmu::Uat::bind` | ||
| //! (`vm.rs`); the shared layer writes TTBAT `(root | 1 | slot << 48, TABLES | 1 | | ||
| //! Jobs acquire exclusive TTBAT slots without waiting at publication (`mmu::Uat::try_bind`) | ||
| //! and return them after successful retirement; failed published jobs retain their slots. |
There was a problem hiding this comment.
Same here: "failed published jobs retain their slots" holds only for fail-stop retained jobs.
| let encoded = (|| { | ||
| for k in 0..n2 { | ||
| match command(&j2, k).ok_or(EINVAL)? { | ||
| Command::Compute(c) => self.prefix_compute(&j2, c, &mut pf, f, false)?, |
There was a problem hiding this comment.
nit: rustfmt --check wants this arm in braces.
| .iter() | ||
| .enumerate() | ||
| .any(|(i, f)| self.receipts(i).renders != 0 && f.job.vm.slot == ctx); | ||
| .any(|(i, f)| self.receipts(i).renders != 0 && f.job.slot() == ctx); |
There was a problem hiding this comment.
g15-pb-growth-tests.py extracts this file, and its mock Job has no slot() (and the mock VmState has no id for :628), so the suite no longer builds.
| } | ||
| } | ||
| if result.is_ok() || !engine { | ||
| self.release_binding(); |
There was a problem hiding this comment.
g15-input-error-tests.py's mock Job needs release_binding() for this call; its control fails as NOBUILD.
| @@ -518,8 +518,6 @@ pub(crate) mod sizes { | |||
| pub(crate) const AHEAD_RECORDS: u32 = 64; | |||
| /// Policy bound on logical queues reported in QUEUE_LIMITS (not a firmware limit). | |||
| pub(crate) const MAX_QUEUES: u32 = 58; | |||
There was a problem hiding this comment.
Removing MAX_VMS changes the hw.rs hash pinned at g15-zero-words-tests.py:29; please refresh it with a note.
scottjones
left a comment
There was a problem hiding this comment.
Thanks Eryk, this is a nice cleanup, and it follows on well from your fixes in #202 and #214. We'd like the tests sorted before merge; the driver logic looks right to us.
Moving the ASID to publication and dropping the 63 VM cap makes sense, since only one VM is ever in flight. The stamp wrap logic also looks right to us. We built the asahi driver at the head and at the merge base (G15 only, G15 with M3_T8122, and M3_T8122 only) with W=1. There are no new errors, and the G15 builds have no new warnings.
Blocker: host tests are red. host tests (tools) fails in CI run 40253690456, and we see the same thing locally. All three pass on the merge base:
tools/asahi/g15-input-error-tests.py: the stubJobhas norelease_binding(), so the control build fails.tools/asahi/g15-pb-growth-tests.py: the stubJobhas noslot()and the stubVmStatehas noid.tools/asahi/g15-zero-words-tests.py: the hw.rs golden hash changed becauseMAX_VMSwas removed. The hash in the test needs a deliberate refresh.
Nothing on the host tests the binding lifecycle. The new stamp tests are good. We mutated stamp_reached, stamp_seed, pending_stamp and the cwait check, and each mutation made a test fail. The slot changes are different. We made each of these changes one at a time, and all six G15 suites that still build passed every time:
- removed
job.release_binding()inretire_oldest - made
Job::finishrelease the binding on engine failures too - removed
slot_wait = trueon ENOSPC
The second one is the dangerous one: it hands a failed job's ASID to another VM. A small host test around Job::bind/release_binding and the retire/finish rules would catch it. Even one that only pins "a failed engine completion keeps its binding" would do. Your hardware runs are convincing, but nothing guards this on the host.
Two questions we couldn't settle by reading, so they rest on your hardware runs. Could you confirm them?
- Stamp 0: the firmware doesn't treat a stamp of 0 specially? The old code avoided it on purpose.
- Per-context firmware state: now that a slot moves between VMs after every job, does any per-context state outlive a flight's retirement? We're thinking of InitBM's
ctxand the PB growth requests keyed by it (pools.rs:910-916). Before, a VM kept its slot for life.
Just so you know: on ENOSPC the publish loop stops at the picked job, so jobs whose VM already holds a slot wait behind it (engine.rs:586). It only happens under exhaustion and doesn't last long, so it's fine as is.
Small one: with M3_T8122=y and G15=n, Uat::try_bind is unused and W=1 warns that it's never used. A #[cfg] or #[allow(dead_code)] would keep G15-off builds clean.
maralcbr
left a comment
There was a problem hiding this comment.
Reviewed at 5782625 (source only; no build or hardware on my side).
The direction is good: binding ASIDs at publication removes the 63-live-VM cap, and making absent waits and progress Options instead of overloading 0 is the right way to let stamps wrap through zero. The CTS result and the wrap stress run are strong evidence. Before merge:
1. CI is red (host tests (tools), run 40253690456). Three G15 host suites fail:
g15-input-error-tests.py: the extracted model offinish()now callsJob::release_binding(), which the model'sJobdoesn't have.g15-pb-growth-tests.py: the extracted code now usesJob::slot()andVmState::id, missing from the model.g15-zero-words-tests.py: the whole-file golden hash ofhw.rsis stale, sinceMAX_VMSwas removed.
These are broken host checks, not a production build failure, but they need updating with the refactor.
2. Build evidence for G15. The ci / compile job annotates all ten changed G15 Rust files as "not compiled by this job". Your m3plasma-5782625 kernel implies it builds, but please attach a build of this head with G15 enabled (or point at the job that compiles it).
3. Zero stamp at the barriers. At wrap, encode::barrier() sends a literal WAIT = 0 to the firmware, because barriers are emitted by predecessor type, not by value. If the firmware treated 0 as "no wait", ordering would quietly loosen exactly at the wrap, and CTS or throughput runs would rarely catch it. The first zero targets fall at about ordinal 4,128,768 (compute), 8,781,824 (TA) and 12,779,520 (FS). Did the 13M-pair run check dependent output across those transitions, for TA→FS, FS→compute and compute→compute, with plain Stamp barriers rather than only Hold (which also holds the predecessor and could mask a bad stamp comparison)?
4. A job resuming on a different ASID. The code looks sound: bindings are refcounted per VM, so a live flight keeps its slot. But ordinary split-job runs probably keep getting the preferred slot back. An instrumented split-job test that forces one job to resume on a different ASID number, and checks its output, would close this.
5. Doc nit. vm.rs (and the uat.rs comment) say failed published jobs keep their binding until reboot. In the code, a recovered hang completes the job and releases the binding once its references drop; only fail-stop retains it. Worth spelling out, since it matters for slot exhaustion.
Include zero in the firmware stamp cycle. Keep absent dependencies and completed progress separate from wrapped zero stamps. Generated-by: LLM Signed-off-by: Eryk Wieliczko <eryk@wieliczko.ninja>
Idle VMs must not reserve hardware ASIDs. Bind without waiting at publication, release at successful retirement, and retain failed published jobs. Logical VM creation is limited by memory rather than slot count. Generated-by: LLM Signed-off-by: Eryk Wieliczko <eryk@wieliczko.ninja>
5782625 to
8525dd8
Compare
chriskearney
left a comment
There was a problem hiding this comment.
Reviewed 8525dd8e380a708356ec1ffaef0bbdf266cb328b against 937b727ef9a3a9ff09489d597ef4b28f3b6fdbfa.
Requesting changes because the current head still breaks three host suites. I checked the actual tools-job log, which identifies merge b62f22a3 of this head into the target:
g15-input-error-tests.py: the control is NOBUILD because the mockJoblacksrelease_binding().g15-pb-growth-tests.py: the mockJob::slot()andVmState::idare missing.g15-zero-words-tests.py: thehw.rsgolden hash needs the deliberate update for removal ofMAX_VMS.
All three exit 1 where the runner expects 0. The current patch contains no tools/asahi updates, so the earlier reviewer findings remain current after the rebase. Please update the mocks/hash and get the tools job green. Preserve real runtime mutation checks; a control that fails compilation is not a successful negative test. The binding-retention-on-engine-failure coverage requested in the existing review also remains relevant.
The production changes look coherent in our bounded source review: absent progress is distinct from stamp zero, publication acquires the binding, and retirement releases it only after published work retires. Our four CPU stamp-helper controls passed. We also completed native-provider benchmarks and normal returns on J516S/J293/J313 with the combined #233+#238 and Mesa #17 stack; the detailed table is in our #238 review. M3 Quake measured 186.5 → 658.7 FPS, while the two M1 Quake comparisons were roughly flat. That is positive combined-stack evidence, not an isolated attribution to this PR or hardware proof of stamp-wrap/binding-pressure correctness. Existing OMT/log warnings and incomplete CTS qualification are retained in that report.
The benchmark result does not override the current CI failure or resolve the first-attempt wrong-draw evidence documented by the author.
…cleanly; 58-queue cap found Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Tested on J516C (M3 Max, T6031 / G15C): head Stamp wrap: pass. 4,300,032 compute command buffers (64 per submit, every result checked) took the engine's compute count from 21,359 to 4,321,391, past the wrap at 4,128,768 ( VM slots: pass. N processes each open a Vulkan device, all wait until every device is open, then each runs 50 checked submits.
The PR-side failures are not ASIDs: each failing process gets
Separately, ENOMEM for an advertised count limit is a surprising errno; happy to open an issue for it. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Superseded by #253, now merged as Eryk's |
Bind ASIDs without waiting at publication and release them when published
prefixes retire. Retry temporary exhaustion without failing healthy VMs.
Idle VMs no longer reserve hardware slots.
Rebased onto
integration/auroraat937b727ef9a3a9ff09489d597ef4b28f3b6fdbfa.This PR's new head is
8525dd8e380a708356ec1ffaef0bbdf266cb328b;#238 remains stacked on it. Both patches are unchanged by the rebase.
The combined stack is packaged and booted on J514S: kernel
7.2.11-m3plasma-8824dd6-ARCHwith Mesagit-eaf69705c3fromomacom/mesa#17. Kernel host checks: 80 pass, 2 ignored;
50 GEM lifetime cases and Settings/theme checks pass. The broader CTS sample
has 7,054 passes, and the 16-set group has 2,694 passes. The seven-case
regression group has six passes and one wrong-result flake that passed its
retry (
uaddcarry.uint_highp_geometry); its cause is unresolved. Full CTSon this rebased stack is in progress, not yet qualified as a full pass.
Earlier validation included 100 submitting VMs through one ASID, split jobs,
PB reclaim, recovered-hang isolation and 13 million mixed compute/render
pairs across all stamp wraps. The historical full Vulkan 1.4 run had
3,247,552 cases, zero failures/crashes on kernel
m3plasma-5782625andMesa
git-ea2c18f45c; that result belongs to the pre-rebase stack.