trunk: largest-first pinning, --trunk-ring, io_uring reads (measured), and two re-read fixes - #57
Draft
Avicennasis wants to merge 4 commits into
Draft
trunk: largest-first pinning, --trunk-ring, io_uring reads (measured), and two re-read fixes#57Avicennasis wants to merge 4 commits into
Avicennasis wants to merge 4 commits into
Conversation
Three changes to the streamed trunk, and a test that can finally see them.
PIN LARGEST FIRST. Pinning was a prefix, layers 0..K-1. For the read volume
it avoids that is neutral -- any pinned set totalling B bytes removes exactly
B bytes per token -- but it is not neutral for the ring, whose uniform slot
must hold the largest layer still streaming. Prefix pinning takes layer 0
first, which at 2.34 GB is the largest in the model, so the budget kept
paying for a slot sized to a layer that no longer used it: about 1.17 GB
wasted at every budget above the floor. Sorting by size descending drops the
largest survivor, which shrinks the slot, which frees budget, which pins
more, so the two are solved together by the existing fixed-point loop.
Selection also keeps trying smaller layers after a large one no longer fits,
so the budget is filled rather than merely walked. K3_PIN_PREFIX=1 restores
the old order on the same binary.
A DEEPER RING. Two slots overlap one read with one layer of compute, which
is enough only while the two take about the same time. They do not: trunk
reads are 62.40 s of a 135.8 s token, so the reader idles part of every
layer waiting to be allowed to start the next. --trunk-ring N (default 2)
asks for more, and the prefetcher now queues as many upcoming layers as the
ring can hold instead of exactly one. Two invariants that the old two-slot
design could leave informal are now enforced in one place: a slot being read
into is never handed out again, and the slot the caller is computing on is
never evicted. The second one is the failure that produced fluent wrong
tokens when a single-slot ring was given a reader thread.
io_uring. pread is queue depth one, and an NVMe device does not reach its
rated bandwidth at depth one -- tools/devbw.py already probes at QD16 for
that reason. src/io/k3_uring.c splits a layer run into 8 MB requests and
keeps up to 16 outstanding, via raw syscalls rather than a liburing
dependency. It falls back to pread whenever io_uring is unavailable,
refused, or disabled with K3_NOURING=1, and no output bit depends on which
path runs. K3_SQPOLL=1 is available and off by default: it costs a kernel
thread spinning.
AND A TEST, tests/unit/test_trunk.c, because none of this was reachable
without a 108.81 GB checkpoint and all of it fails silently. It writes a
synthetic trunk whose every layer is filled with a pattern derived from its
index, walks it the way forward() does, and checks the bytes after every
fetch. Two real bugs were found by it and fixed here:
- the pinned-layer read on the main thread was driving the READER's
io_uring. One ring, two submitters: each reaps the other's completions
and both wait forever for one that has already been consumed.
- io_uring_enter may consume fewer SQEs than asked. Passing the count this
iteration happened to queue left the remainder in the ring forever, and
the next call, with nothing new to submit, waited on a completion that
was never coming. to_submit is now recomputed from the ring and
min_complete counts only what the kernel already holds.
Both presented as an intermittent hang, roughly one run in three.
hits and misses are now classified once per bind rather than incremented by
whichever thread did the read, so they sum to the bind count instead of
double counting every prefetched layer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0138q5i3RF7Pw28uT4Exoboa
(cherry picked from commit 9cd7520)
… reader Four things the port needed on top of the previous commit: - k3_trunk_open took a ring-depth argument; test_trunk.c still used the old shape. Pass 0, which selects the default of 2 the test was written against, so every existing assertion keeps its meaning. - test_trunk links k3_trunk.o and therefore now needs k3_uring.o too. - The one-slot guard assertion checked `io_state == NULL` as a proxy for "no reader thread". This reader keeps the io state alive with one slot (its mutex serialises slot bookkeeping either way) and starts the thread only at two or more slots, which is the invariant the test exists to protect. K3Trunk gains `reader_started`, set exactly when pthread_create succeeds, and the test asserts on that. The message about silent corruption of active layers stays. - A bind that failed (the truncated-trunk case) was still charged as a miss, because this reader classifies the bind on its first look, before the read. A failed bind publishes nothing and is not a cache event; hits + misses counts completed binds, and the miss is now taken back on the failure path. Verified after these changes, not before: TRUNK TESTS PASSED on the io_uring path and under K3_NOURING=1; `make test` green; a packed tiny trunk decodes the same ten ids on both paths as the resident reader.
Found by an anomaly in a real run: 491 GB read from a 29.81 GB trunk over ten
forward passes, against ~248 GB if every streamed layer were read once per
pass. Config A in the same session -- trunk fully resident -- reported 29.81
GB dead on, so it only appeared when layers actually streamed.
claim_slot_locked protected two things: a slot being read into, and the slot
the caller is computing on. It did not protect a slot holding a layer the
prefetcher had already fetched and published but the walk had not yet
reached. That slot is neither pending nor held, so a claim for a
further-ahead layer evicted it -- and the walk arrived a moment later and read
it again. It bites hardest immediately after a PINNED layer, because those
hold no ring slot, so `held` is -1 and every slot looks claimable.
upcoming() adds the third rule, bounded by the ring itself so that stale
slots from the previous pass -- layers 91 and 92 still sitting there when the
walk has wrapped to 0 -- stay evictable and a two-slot ring cannot deadlock at
the start of every pass.
The bug costs bytes, not correctness, which is why nothing caught it: the
token stream is identical either way, and the only symptom is a device rate
that flatters and an I/O share that is wrong. So the invariant is now
asserted rather than reasoned about. test_trunk checks, at every ring depth,
bytes_read <= pinned_bytes + passes * unpinned_bytes
It reproduces the bug at ring depth 2 (6.23 MB against a 4.72 MB bound) and
is exact at depths 1 and 3, which is also why the released-checkpoint run
showed it at depth 2.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0138q5i3RF7Pw28uT4Exoboa
(cherry picked from commit df6fd7e)
The re-read fix that landed last measured the wrong distance. It protected layers walk+1 .. walk+nslot by INDEX, which is the right window only when nothing is pinned: k3_trunk_prefetch SKIPS pinned layers while scanning rather than stopping at them, so with pinned layers ahead it queues a layer many indices out that is only a few slots out. The index window then called that layer stale, the next claim evicted it, and the walk read it again a moment later. upcoming() now counts the layers that would actually take a ring slot. That is identical to the old test whenever nothing is pinned, and strictly wider otherwise. HOW IT WAS FOUND, because the method is the point. A run on the released checkpoint sat 13.7% above the bound after the first fix. Two explanations were argued from the pinned set's shape and neither had evidence, because the two hand-built fixtures -- both shaped deliberately to expose exactly this bug -- passed the broken build. t_sweep enumerates the space instead: a 93-layer trunk, ring depths 1 to 4, budgets from a bare ring to fully resident. It failed 40 of 100 pinned shapes on that build, worst 35 layers re-read at 56 pinned with a four-slot ring, and passes all 100 with this. Also count reads PER LAYER and print the offenders in k3_trunk_report. The aggregate byte total could say a run went over and never which layers, which is what made the last two rounds guesswork. Clean under ThreadSanitizer and ASan/UBSan, and over six repeat runs -- the wider window protects more slots, so a deadlocked ring was the risk worth ruling out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0138q5i3RF7Pw28uT4Exoboa (cherry picked from commit 7eacd01)
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
The streamed trunk reader: layers are pinned largest-first instead of as a prefix, the ring depth is a flag (
--trunk-ring N, default 2), and on Linux each layer is read through io_uring with raw syscalls -- no liburing, 8 MB requests, up to 16 in flight -- falling back to the existingpreadloop when the kernel has no io_uring orK3_NOURING=1is set. Two follow-up commits fix the reader re-reading layers it had already prefetched.Why
A
preadloop leaves the device at queue depth 1 and an NVMe drive does not reach its rated bandwidth there. #56 (cablepull's chunked pread) attacks the same fact portably with OpenMP; this was meant to be the Linux-native answer. The numbers below, from the same box, same trunk, same night, three arms alternated, say the portable one wins on this hardware.Largest-first pinning: which layers are pinned is byte-neutral for the traffic avoided but not for the ring, whose uniform slot must hold the largest layer still streaming. Prefix pinning took the 2.34 GB dense layer first and then kept paying for a slot sized to it.
Provenance
All four engine commits are from @Deobot2's fork (Deobot2/kimi-k3-in-c,
9cd7520,df6fd7e,7eacd01), authored there by Claude Code and carried with authorship preserved and-xtrailers; this is the io_uring third of what #29 bundled with no description. Deobot2 should be the CONTRIBUTORS.md name. My one commit makes upstream'stest_trunk.cbuild and pass on the new reader; the notes on what each of its four changes is and why are in that commit message.The re-read fixes deserve an honest framing: the bug they fix was introduced by the rewrite itself, not present on
main. On a packed tiny trunk (make_tiny_checkpoint.py, 13 layers, 10 tokens,--trunk-gb 0.001)mainreads 25,759,744 bytes; the rewrite alone read 48,791,552; with the two fixes it reads 25,206,784 -- about 2% undermain, with identical output throughout.Verification
make testpasses (all weightless gates), including upstream's synthetic trunk test on both read paths (./bin/test_trunkandK3_NOURING=1 ./bin/test_trunk)make portablebuilds with no new warningsK3_NOURING=1, and at--trunk-ring 3, asmaindoes residentThe one test change that is not mechanical: upstream's one-slot guard asserted
io_state == NULLas a proxy for "no reader thread". This reader keeps the io state alive with one slot (its mutex serialises slot bookkeeping either way) and starts the thread only at two or more slots -- which is the invariant the assertion protects, and Deobot2's comment at that site records the exact wrong-token sequence the race produced when it was violated.K3Trunkgainsreader_started, set exactly whenpthread_createsucceeds, and the test asserts on that. Every property assertion around it (prefetch is a no-op, L0 survives, content correct) was already passing.Numbers, if this is a performance change
Same bench as the chunked-pread PR: released checkpoint,
--preset desktop, trunk on NVMe, experts on SATA,--gen 3, arms alternated, 3 runs each, 2026-09-05. Metric of record is the engine's whole-run trunk MB/s; expert MB/s (447-454 in every run) is the positive control.Trunk read bandwidth, MB/s
main(pread QD1)Wall clock, seconds
main(pread QD1)Read plainly: on this box the io_uring path is +5% over
mainand no faster end to end, while the chunked-pread reader is +42% and ~1.5x. Overlap with compute is 71-73% here against 89-90% there. I do not know why -- 8 MB requests at depth 16 should not lose to 64 MiB chunks at OpenMP width, and it may be the raw-syscall submit path, the request size, or the two-ring design -- and I would rather report the measurement than a theory. Two consequences for how to take this PR: (1) the value that survives the numbers is largest-first pinning,--trunk-ring, the two re-read fixes and the per-layer read accounting, all of which are independent of which syscall issues the read; (2) io_uring as shipped here is default-on on Linux, and on this evidence it should probably be opt-in (K3_URING=1) until someone measures a win. I left the code as ported so the two readers can be compared as written; say the word and I will flip the default, or split the reader out of this PR entirely.Risk
io_uring is opt-out, not opt-in, on Linux; a kernel with io_uring disabled by sysctl or seccomp takes the fallback silently -- the startup line says which path is in use, and
K3_NOURING=1forces the old one. The reader now owns two rings (main thread and reader thread) because an io_uring must not be driven from two threads; the two hangs Deobot2 found and fixed in exactly that area are recorded in their commit. Nothing here touches Darwin or Windows code paths beyond the fallback.