Make MP:CAS on an SVREF place actually atomic - #1838
Open
dg1sbg wants to merge 1 commit into
Open
Conversation
MP:CAS on (SVREF v i) compiled, and was correct single-threaded, and did not arbitrate between threads. Four threads running 50000 compare-and-swap increments each landed 171000-182000 of 200000, losing 9-15% of the updates, on every run. The atomic expander for SVREF in src/lisp/kernel/lsp/atomics.lisp emits CORE:ACAS, and CORE:ACAS has two implementations. The native one is a genuine compare-and-swap: bir-to-bmir.lisp transforms the call to the CORE:ACAS primop and primop.lisp lowers that to an LLVM cmpxchg on the element address. The other is the C++ fallback core__acas in src/core/array.cc, taken whenever the Cleavir transform does not run. That is not a corner: the bytecode VM has no atomic opcode at all, so besides every --build-mode=bytecode build, EVAL, the REPL and LOAD of a source file reach the fallback in *every* build. That fallback was a plain aref, an EQ compare and a plain aset, as its own comment admitted. Measured on this tree before the change: the native arm lands exactly, the bytecode and interpreted arms both lose updates. A simple vector stores its elements in a plain GCArray_moveable<T_sp>, so unlike Rack -- which stores GCArray_atomic<T_sp>, and is why every CLOS-side casser already arbitrated -- there is no std::atomic to call. Operate on the element's tagged word directly with the atomic builtins instead. That needs no GC cooperation: Clasp has no write barrier of any kind, no GC variant relocates objects (Boehm is mark-sweep with interior pointers, the MMTk variant runs Immix with copying compiled out), the only relocating operation is snapshot save and it runs with the world stopped, and gc_yield is called only on function entry so no safepoint can fall inside the swap. It is also precisely what the native path already emits. CORE:ATOMIC-AREF and (SETF CORE:ATOMIC-AREF) told the same lie and are fixed the same way. All three now resolve the element address through asAbstractSimpleVectorRange, so displaced and fill-pointer general arrays keep working, and as a side effect the row-major index is computed once instead of twice. Specialized arrays have no tagged word to swap: the reader and writer fall back to the ordinary accessors exactly as before, while ACAS signals a TYPE-ERROR instead of silently performing a non-atomic read-compare-write that could not have succeeded anyway, since a freshly boxed element is never EQ to the expected value. As with core__car_atomic in cons.cc these ignore the requested memory order and use the strongest one. That is always valid, and this is by construction the out-of-line path. A static_assert requires the tagged word to swap lock-free. Were that ever untrue on some target the builtin would quietly emit a libatomic call, which takes a lock inside the swap window; failing the build is the better outcome. The refusal is deliberately asymmetric, and mirrors the native path's own asymmetry: bir-to-bmir.lisp transforms CORE:ACAS only for SIMPLE-VECTOR, so ACAS refusing other storage makes the two implementations agree on their domain, whereas it transforms CORE:ATOMIC-AREF for single-float, double-float, base-char and character arrays too, so making the reader and writer refuse those would have made the same source succeed natively and error in bytecode. They keep their existing behaviour instead. The new tests in the mp suite reach CORE:ACAS through FDEFINITION so that the runtime implementation is exercised even in a build whose compiler would inline a cmpxchg for the MP:CAS form instead. They cover contention, the single-threaded and single-swap controls, displacement, and the refusal of specialized arrays. Fixes #1.
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
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.
MP:CASon anSVREFplace compiles, is correct single-threaded, and does not arbitratebetween threads. Four threads running 50000 compare-and-swap increments each land
171000-182000 of 200000 — 9-15% of the updates silently lost, on every run.
This is worse than a refusal, because every single-threaded test passes.
SVREFhas beenthe documented place that works for portable compare-and-swap, so code built on that
reading is silently unsound.
Why
The
SVREFatomic expander (src/lisp/kernel/lsp/atomics.lisp:349-367) emitsCORE:ACAS,which has two implementations:
bir-to-bmir.lisp:426transforms the call to theCORE:ACASprimop andprimop.lisp:679lowers it to an LLVMcmpxchg. Genuinely atomic.core__acas(src/core/array.cc) — a plainaref, anEQcompare anda plain
aset, with the order discarded. Its own comment said so: "BUT: They aren'tactually atomic."
The fallback is not a corner case. The bytecode VM has no atomic opcode at all, so besides
every
--build-mode=bytecodebuild,EVAL, the REPL andLOADof a source file reach itin every build.
CORE:ATOMIC-AREFand itsSETFhad the identical defect.Measured in one process before the change:
SIMPLE-CORE-FUN)The fix
A simple vector stores elements in a plain
GCArray_moveable<T_sp>, so unlikeRack—which stores
GCArray_atomic<T_sp>, and is why every CLOS-side casser already arbitrated —there is no
std::atomicto call. The three entry points now operate on the element'stagged word with the atomic builtins.
This needs no GC cooperation: Clasp has no write barrier of any kind, no GC variant
relocates objects (Boehm is mark-sweep with interior pointers; the MMTk variant runs Immix
with copying compiled out), the only relocating operation is snapshot save and it runs with
the world stopped, and
gc_yieldis called only on function entry so no safepoint can fallinside the swap. It is also precisely what the native path already emits — verified by
disassembly: one inline
casal, andllvm-nm -ushows no__atomic_*libcall.All three resolve the address through
asAbstractSimpleVectorRange, so displaced andfill-pointer general arrays keep working, and the row-major index is computed once instead
of twice.
Behaviour change worth reviewing
CORE:ACASon storage that is not a simple vector ofTnow signals aTYPE-ERRORofexpected type
(ARRAY T), where it previously performed a non-atomic read-compare-writethat could not have succeeded anyway — a freshly boxed element is never
EQto theexpected value.
The refusal is deliberately asymmetric and mirrors the native path's own asymmetry:
bir-to-bmir.lisptransformsCORE:ACASonly forSIMPLE-VECTOR, so refusing otherstorage makes the two implementations agree on their domain; but it transforms
CORE:ATOMIC-AREFforsingle-float,double-float,base-charandcharacterarraystoo, so making the reader and writer refuse those would have made the same source succeed
natively and error in bytecode. They keep their existing behaviour instead.
A
static_assertrequires the tagged word to swap lock-free, so a target without an8-byte lock-free CAS fails the build rather than quietly emitting a libatomic call that
takes a lock inside the swap window.
Tests
Seven tests in the existing
mpsuite. The "out-of-line" variants reachCORE:ACASthrough
FDEFINITIONso the runtime implementation is exercised even where the compilerwould inline a
cmpxchginstead. They cover contention, the single-threaded andsingle-swap controls, displacement, and the refusal.
They are a real gate: against the pre-fix binary 3 of the 7 fail; after the fix all 7 pass.
The 4 that pass pre-fix are the controls, which must keep passing — a "fix" that made every
swap succeed, or every swap refuse, would satisfy the arithmetic and fail those.
Verification
macOS arm64,
boehmprecise, plain Clasp (no extensions), built from this branch's parentplus this commit:
out-of-line call.
the 5 known ones by name (
SBCL-CROSS-COMPILE-4,INCLUDE-LEVEL-2B,INCLUDE-LEVEL-3,WEAK-KEY-OR-VALUE-WEAKNESS,TYPES-CLASSES-10).clang-formatclean.Reported as dg1sbg#1, which carries the standalone reproducer.