Repository navigation
A descriptor call holds its object without a lock - #1234
Conversation
A `read` or `write` through a descriptor took the process's one descriptor-table lock to take its hold and again to give it up, so two threads doing I/O on different descriptors met on it twice a call. In #1150's metal A/B the pipe benchmark's two threads slept on it 128,000 to 153,000 times a run once the kernel stopped serializing them itself. The table's representation now carries the hold. A descriptor's word names an object's slot, and a slot counts its references, the descriptors naming it and the calls holding it: - A call counts itself onto the slot its descriptor names only while the count is not 0, then keeps the reference only if the descriptor still names that slot. That is one compare-exchange and two loads; giving the hold up is one subtraction. - A slot is free while its count is 0, and only the writer takes a count off 0, after writing the object. Whoever holds a reference reads the object, so nothing is written under a reader and a slot is reused only after its last hold. - Whoever takes a count to 0 is answered the object, read while it still held its reference, and lets it go: once, after the last call. - A descriptor changes in one store, so a call beside a `dup2` or a `replace` holds the old object or the new, and a closed number is free at once, as lowest-free numbering needs and as before. What changes a descriptor (`install`, `dup`, `dup2`, `close`, `replace`, the flag) still takes one lock, as a `Writer` the table hands out to one holder at a time; the mutators are its methods, so none is reachable without it. The lock is held across no blocking call: a released object is let go after it, as before. The first use still installs the endowed descriptors under it, and a call looks at one flag to know that happened. Each slot is a cache line of its own, so calls on two descriptors write no line they share. The table is all zeros now and costs an image no initialised data; the old one's free objects were not. A call that finds its descriptor naming a free slot reads the descriptor again: that state is a close in flight, and no longer told from a broken table on this path. The writer, under whose lock it cannot be in flight, still answers it as broken. `fd::table::model` runs a call beside a close, a close and a reuse of the slot, a reopen of the number, a second call and a `dup2` under loom, the object in loom's cell. Three controls show it can red: a call that counts itself onto a free slot, one that does not look at the descriptor again, and a writer that publishes a count unordered after its object. Closes issues/every-descriptor-call-takes-one-process-wide-lock-twice.md: a hold takes no lock at all. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Review of #1234 at d8f9a49 (base 34cb4c9)I read the commit, the full diff, ArchitectureThe fit is right. The table stays in the SDK copy that owns it. Nothing changes in the kernel, Size: production code is +186 lines ( Invariants, checked against the code
Concurrency models and controlsThe five cases cover the real races between a call and the writer:
A dup cannot race a close, because both are writer operations serialized by Each control fails for its stated reason (
None of the three removes the reader→writer edge that invariant 1 depends on ( CostThe PR says the table's .bss is "address space". It is committed memory. A Alternatives:
My judgment is that the padding is defensible on the call path but has not been measured separately. The A/B cannot isolate it (S7). Overlap with #1226The conflict is textual only. #1226 calls One placement rule matters when merging. EvidenceVerified:
The fixes required below touch only comments and the description, so the guest and metal evidence at d8f9a49 stays valid. The only rerun needed is a compile. FindingsBLOCKER
SUGGESTION
What metal must show for LAND
SEND BACK. The blockers above are two comment fixes and two description corrections. I found no defect in the concurrency code. Once they are fixed: LAND, pending the metal results above. |
…ments that say what is true `fd-reuse-relaxed` makes the writer's load that finds a slot free `Relaxed`. Both reuse cases red with loom's causality violation, so the models have teeth on the edge "no use after close" rests on: the last call's read of an object before the writer's next write to its slot. The reuse case closes the second descriptor before the call ends, so it also covers a call left the object its own descriptor never named. The model implements `Word` on loom's atomic directly. `Cell` is `ObjectCell` and the owner's writer type `TableWriter`. The slot comment says two objects, not two descriptors, since a dup shares a slot. The model's header no longer claims every memory order (loom does not explore load buffering) or a map of controls it does not keep: build/ci.rs's `CONTROLS` is that map. Calls on two served files still meet on `files::owner`'s `OPEN` lock: recorded in issues/calls-on-two-served-files-meet-on-one-process-wide-lock.md. Nothing the descriptor calls execute changes: a rename and a constant ordering compile to the same code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A call whose descriptor names a free slot reread the word until it changed. That state is a close in flight or a broken table, and only the writer can tell the two apart, so a broken table spun the call forever where main panicked. The table's `acquire` now answers `None` there, and the owner holds the descriptor under the writer's lock once: a closed descriptor is refused, and a broken table panics once the lock is let go. The settling call is cold, out of line and tail-called, so the lock-free loop keeps its shape: one more callee-saved register saved and a frame of 0x20 bytes instead of 0x18, against d8f9a49, with the instructions otherwise the same. The A/B on metal measured d8f9a49. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review r2 of #1234 at dafb2e8 (base 34cb4c9)This is a focused follow-up covering two things: whether r1's required items are resolved, and whether the new code is correct. I read 35d452b and dafb2e8 in full against d8f9a49, along with the current ArchitectureS3 puts the decision with the authority that can make it. The lock-free path cannot tell a finished close from a broken table, and only the writer can. So r1's required items: resolved
New codeS1
S3 (
S2, S4, S5: taken as described.
S6: Evidence
The A/B at d8f9a49: honest and sufficient.
The prose summary of the delta is a little too strong. See suggestion 1. FindingsBLOCKER None. SUGGESTION
LAND. r1's blockers are resolved, and S1–S6 are correct as implemented. S3 panics only on a real inconsistency and introduces no lock cycle under this PR or #1226's current placement. The metal evidence at d8f9a49 carries to this head. Landing remains the orchestrator's decision. |
|
Orchestrator: focused re-review r2 LAND at dafb2e8. I corrected the PR body per its suggestions 1 and 2: the exact disassembly delta, and that a call's hold also takes the writer lock on a free slot. The A/B at d8f9a49, with pipe throughput +10–27% and no slower row on either bench, stands with the stated delta. Queueing. |
Problem
A
readorwritethrough a descriptor took the process's one descriptor-table lock to take its hold and again to give it up, so two threads doing I/O on different descriptors met on it twice a call. In #1150's metal A/B the pipe benchmark's two threads slept on that lock 128,000 to 153,000 times a run once the kernel stopped serializing their calls.Result
A descriptor call's hold takes no lock, except on the table's first use and when its word names a free slot, where it takes the writer lock once to settle. The table's representation carries the hold: a descriptor's word names an object's slot, and a slot counts its references (the descriptors naming it and the calls holding it).
dup2/replaceholds the old object or the new; a closed number is free at once (lowest-free numbering unchanged).Changing a descriptor (
install,dup,dup2,close,replace, the inheritable flag) still takes one lock, as aWriterhanded to one holder at a time; the mutators are its methods, so none is reachable without it. Endowed descriptors are still installed under that lock on first use; a call checks oneAcquireflag to know that happened. A call whose descriptor names a free slot takes that lock once, and the writer decides: a descriptor closed meanwhile is refused, and a broken table panics, as it did on main.Closes
issues/every-descriptor-call-takes-one-process-wide-lock-twice.md. The manual describes no descriptor-table locking, so it is unchanged.Invariants and where they are held
hold), and the reader's object read precedes its decrement (AcqRel), which the writer'sAcquireload of 0 orders before its write.fd-reuse-relaxedshows the models red without that edge.closeclears the word at once, as main does and POSIX requires (libc'spollrelies on it); the slot stays counted until the lastunuse. The stronger form, "a number is reused only after its holds are released", does not hold here and should not.dup/dup2/close/replace/flag semanticsWritermethods; host unit tests infd::table::tests.copyandinheritablecall; it was never one atomic snapshot of the table.files::adopt_exportedasks a file server (Dir::adopt,fstat) while it is held. Main did the same under itsTABLElock, and every other call has to wait for the endowment anyway.Consequential decisions
#[repr(align(64))], so calls on two objects write no line they share (two descriptors thatdupone object share its slot). The table is.bssin a loaded segment, which is committed memory: 272 KiB per process (16 KiB of words, 4096 × 64 B of slots) against main's 136 KiB (u16words and 32-B entries in.data). Of the +136 KiB, 128 KiB is the price of the padding: natural 32-B slots would cost 144 KiB but put neighbouring objects, such as a pipe's two ends, on one line. The padding is not measured separately; the A/B below cannot isolate it. Physical frames are similar on both sides, since only touched pages are populated.Table::acquireanswersNonethere, and the owner holds the descriptor under the writer lock in a cold, out-of-line function the loop tail-calls. Rereading the word lock-free, as d8f9a49 did, would spin forever on a broken table, which is an internal invariant violation and must fail loudly. The settling path costs the lock-free loop one more callee-saved register saved and restored, and a frame of 0x20 bytes instead of 0x18 (below).Review r1
build/ci.rs'sCONTROLSis that map); this description states committed memory, the first-use exception and the corrected invariant 2.fd-reuse-relaxedmakes the writer's free-slot loadRelaxed; both reuse cases red with loom's "Causality violation: Concurrent read and write accesses toUnsafeCell".Wordis implemented on loom's atomic directly. OneWordtrait acrossfd::table,transportandlog::ringis declined here: their operation sets differ (fetch_subhere,swapin the transport), and merging them changes two other modules' seams this PR does not need.ObjectCellandTableWriter.issues/calls-on-two-served-files-meet-on-one-process-wide-lock.md.Checks
At dafb2e8 (base 34cb4c9; main has changed none of these files since, and the merge is clean). Host checks: evidence r2.
host-35d452b2a/ran at the round's first commit.host-s3-patch/ran the same checks on that commit with S3's patch applied, which is byte-for-byte dafb2e8's tree; S3 changes nothing the build-system test reads. Metal: evidence r3. r1, at d8f9a49, is beside both.Chosen under the owner's targeted-testing rule: the change is the SDK's descriptor table, its
ownermodule, four rows ofbuild/ci.rs'sCONTROLSand one issue record. The fullcargo run -- --ci hostwas not run; the merge queue runs it.cargo test -p toyos --lib fd::cargo test -p toyos --features fd-hold-revives --lib -- --exact <its 3 cases>cargo test -p toyos --features fd-hold-unchecked --lib -- --exact <its case>cargo test -p toyos --features fd-publish-relaxed --lib -- --exact <its 2 cases>Relaxedcargo test -p toyos --features fd-reuse-relaxed --lib -- --exact <its 2 cases>RelaxedloadUnsafeCell"cargo test -p toyos-build --lib -- every_model_control_is_run ci::CONTROLSrows match declared features, and the judge's own testsevery_model_control_is_runand the 20ci::tests passed (6 ignored are drivers that never run alone)cargo clippy -p toyos --all-targets,-p toyos --lib --features fd-table, and-p toyos --all-targets --features <the four controls>, each-- <ADOPTED> -D warningstoyosshapes ofbuild/clippy.rs, the owner module only underfd-table, and the controls' armsThe control commands are the ones
build/ci.rs'srun_controlissues; each named case's... FAILEDline is present and no run selected 0 tests, which is its judge.The guest rows and the metal A/B measured d8f9a49, not this head. The landed hot loop differs from what they measured only by S3's register save. The ToyOS compiler from the sysroot built at d8f9a49 compiled the
toyoscrate withfd-tableforx86_64-unknown-toyosat d8f9a49, at 35d452b and at dafb2e8's source (r2/disasm/). At 35d452b all eleven descriptor functions are byte-identical to d8f9a49 once crate hashes are masked: the ninetoyos_fd_*entry points,toyos_fd_acquire's closure (the lock-free loop) and the owner'stable. With S3 only that closure changes (before/after diff). It pushes and popsr12as well, its frame is 0x20 bytes instead of 0x18, and the slot pointer moves fromrsitor8; the success path also gains one unconditional jump, the loop head loses its alignment padding, the result is stored differently and the path that loses a race saves one more register: about one instruction more than d8f9a49 on the success path, and the settling path is a tail call. The local build at d8f9a49 reproduces the production sysroot'stoyos_fd_acquire, its closure,toyos_fd_unuseandtoyos_fd_inheritablebyte for byte.tableand the writer-side entry points differ there only inside the writer lock's wait path, from std's own build of the crate.Guest rows at d8f9a49: run 43036214335, green, 7 of 7:
libc_children(x86_64):libc_recv_beside_close(arecvblocked across its descriptor's close and the number's reuse),stdio_named,spawn_cloexec,poll_closedand the rest of libc's descriptor and spawn cases, in real processes.stream_holders,netstack_holders_exported_to_children(x86_64): a stream's holders across processes, and descriptors exported to children.a_launch_gives_each_unsent_stdio_slot_a_null(x86_64): the endowed descriptors installed on first use.std_threads_count_the_cpus_and_give_back_ended_threads,osbench_capability_groups_exit_0(x86_64): threaded std programs and osbench end to end.virt_smp(aarch64, emulated): an aarch64 SMP userland boots through the new table. Emulation shows no weak ordering; the loom models check that.Metal, at d8f9a49 (evidence r3), every boot's driver verdict
passed:osbench A/B: the
osbenchbenchcase image, base 34cb4c9 against d8f9a49, 4 boots a side on the T14 and on the AMD bench, booted base-head-head-base twice. Onpipe.*,ipc.*andfs.*read_4k*/write_4k*(25 rows a bench), the median p50 of each side's boots was compared. No head row is slower than base by more than base's own boot-to-boot spread, and none is over twice its machine's recorded value (the scoreboard's ToyOS median, where it lists the row). The small pipe writes the lock serialized gained the most:pipe.throughput.64pipe.throughput.256pipe.throughput.1024pipe.throughput.4096fs.mem.read_4k_atfs.disk.read_4k_atipc.reqrep.8The 128 KiB of slot padding is not isolated by this A/B.
T14
testcasesshared boot: 256 jobs ended. 251 exit 0, andwake_onto_busy's 36 is its p99 in µs, which its row passes. The four reds are not this change's:served_file_offsetand206_libc_refusalsare main's stale tests after The loader is the program's, and the kernel parses no ELF #1189 and A child's arguments and environment are memory its parent lays out #1200. They failed alike in A move names the rights it delivers: a ring and a manifest's SysCap stay where they were given #1204's boot, and tests: two shared-boot members assert what a spawn does on main #1219 fixes both.usbd_spare_vectoris an AMD-bench row judged on the T14 (issues/a-metal-row-names-no-machine-so-another-benchs-boot-carries-it.md).i219_msiis the recorded network-dependent outcome (issues/the-intel-nics-room-and-wake-are-unread-on-hardware.md).Unresolved
ownermutators insdk/toyos/src/fd.rs; the branches conflict textually. Whichever lands second puts A process holds its stdio stream state once, with its descriptor table #1226'schanged(fd)afterchange(..)returns, never inside its closure. A process holds its stdio stream state once, with its descriptor table #1226'saskholdsASKINGand callsfd::hold, which under this PR takes the writer lock on a first use and when it finds its descriptor naming a free slot, so achangedinside the closure would take the two locks in the opposite order.🤖 Generated with Claude Code