Uh oh!
There was an error while loading. Please reload this page.
The CPU suite never runs anything in parallel — turn it on, under TSan, and fix the deadlock that finds - #97
Merged
Conversation
…t found The CPU suite has never executed a single line of concurrent code. `parallel_for` only reaches the thread pool when a loop covers more than GRAIN_SIZE (32768) elements, and every workload in tests/lib-cpu/ is below that: all 13 test binaries make **zero** `clone` syscalls. So the thread pool, `invoke_parallel`, and every "accumulate into a shared output" path in pushpull/restrict are compiled and reported green by 59,886 checks without ever having run multi-threaded. Turning that on found a real bug on the first attempt. * `impl/kernels/parallel.h` -- GRAIN_SIZE becomes overridable with -DFF_GRAIN_SIZE. It is a threshold, never a correctness switch: results must be identical at any value, and 32768 stays the shipping value. The only intended use is the CI leg below. * `.github/workflows/ci.yml` -- a `test-cpu (tsan, grain=1)` job: the same suite, same 13 binaries, same 59,886 checks, rebuilt with -DFF_GRAIN_SIZE=1 under ThreadSanitizer with FF_NUM_THREADS=4. Much stronger than adding one large-volume test case, and the gate's numbers do not move. * `impl/kernels/threadpool.h` -- two defects that leg reports immediately: - `Worker::mExit` is a plain `bool` written by the pool's destructor on the main thread and read by the worker's own thread. TSan flags it on every one of the 13 binaries. Now `std::atomic<bool>`. - `Worker::wake()` notifies the condition variable without ever touching `mCvMut`, and `threadFunc()` waits on it with no predicate. Classic lost wakeup: the worker sees an empty queue, the pusher enqueues and notifies, and the worker then sleeps on work that is already there while the submitting thread blocks in `future::get()`. This is a **hang**, and it is not theoretical -- `test_pushpull` deadlocked under TSan (main thread in `invoke_parallel`, all workers in `pthread_cond_wait`), and with no sanitizer at all a 12-line reproducer hangs 100% of the time at FF_NUM_THREADS=16 or 64. Since `parallel_for` is always called from the main thread, every task lands on `mWorkers.front()`, so a single missed wakeup on that one worker stalls the whole call. `wake()` now takes and releases `mCvMut` before notifying and the wait is predicated on (exiting || queue non-empty). * `tests/kernels/atomic/test.cpp` + `make test-atomics` -- pins what `ff::anyAtomicAdd` actually is on the CPU. `has_atomic_add<T>` answers false for *every* T in every standard (it probes `&std::atomic<T>::fetch_add`, which is an overload set, so SFINAE rejects it), so the CAS specialisation `AtomicAdd<true>` is never instantiated and the selected implementation is a plain non-atomic `+=`. That is safe only because the same predicate routes push/count/restriction onto batch-only parallelism, where threads write disjoint output slices. Deliberately not part of `make test`. Verified: `make test` 59,886 checks / 13 suites / 0 failures, unchanged. Same 59,886 / 13 / 0 with -DFF_GRAIN_SIZE=1, and again under TSan with the thread-pool fix, with no sanitizer reports.
Brings in #87, #90, #91 and #95. One conflict, in include/fastfields/impl/kernels/parallel.h: #91 renamed FF_NAMESPACE_BEGIN(FF) to FF_NAMESPACE_BEGIN(FF_NS) on the line this branch inserts the FF_GRAIN_SIZE block above. Resolved by keeping both -- the new block, then main's FF_NS spelling. Everything else auto-merged. #91's renames do not touch anything this branch depends on: has_atomic_add / anyAtomicAdd keep their names, FF_NS still expands to ff, and the CUDEV -> FF_CUDEV rename is confined to the CUDA half of atomic.h. FF_GRAIN_SIZE, the one macro this branch adds to an installed header, already satisfies #91's FF_-prefix rule -- `tools/rename-macros.py --check` reports "0 file(s) would change" and "include/ is clean". Re-verified on the merge result: * tools/test-baseline.sh --legs default,lib -> byte-identical to tools/test-baseline.expected. 13 suites, 59,886 checks, 0 failures. * -DFF_GRAIN_SIZE=1 -> 59,886 / 13 / 0. * -DFF_GRAIN_SIZE=1 + TSan, FF_NUM_THREADS=4, halt_on_error=1 -> 59,886 / 13 / 0, zero reports. * clone syscalls: 0 across all 13 binaries at the shipping grain size, 2 per binary at FF_GRAIN_SIZE=1. The threshold is unchanged by the merge (0 clones at n=32768, 2 at n=32769, on main and on this branch alike). * The thread-pool defects still reproduce on main at f63c7d8: the data race is deterministic under TSan (threadpool.h:148 write / :164 read) and the lost-wakeup deadlock is stochastic (8/320 trials over FF_NUM_THREADS 8/16/32/64). Both are gone on this branch: 0/320 hangs, 0 TSan reports.
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 freeto 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.
Answers "is the CPU atomic path ever executed?". No — and worse than expected in one direction, better in another. The investigation also turned up a reproducible hang in the shipped thread pool.
1. The CPU suite has never executed a line of concurrent code
parallel_forreaches the thread pool only when a loop covers strictly more thanGRAIN_SIZE(32768) elements. Every workload intests/lib-cpu/is below that. Measured withstrace -e trace=clone,clone3on the 13 test binaries:Zero. The pool is never even constructed (
get_global_pool()is lazy). The threshold is exact — a probe atn = 32768makes 0 clones, atn = 32769it makes 2, identically onmainand on this branch. Sointernal::invoke_parallel, all ofthreadpool.h, and every "several threads accumulate into one output" path inpushpull/restrictare compiled, shipped, and reported green by 59,886 passing checks without ever having run multi-threaded.2. The CAS path is not merely unexecuted — it is uninstantiable
AtomicAdd<true>(thestd::atomiccompare-exchange loop) is selected byhas_atomic_add<T>. That predicate answers false for everyT, in every standard:The reason is not "
fetch_addfor floats is C++20".has_fetch_addprobesdecltype(&C::fetch_add), andstd::atomic<T>::fetch_addis an overload set (with and without amemory_order), so taking its address is ambiguous, SFINAE rejects it, and the detector answers "no" — for integers too, in C++20 too.And if it ever did answer yes, it would not compile:
AtomicAdd<true>::atomicAddreturnsvoidwhileanyAtomicAddreturnsT. Shown by patching only the detector:Not hypothetical rot:
git log --followshowsAtomicAdd<true>previously stored a pointer into an uninitialisedstd::atomicand returned from a void function (f6aa841, "was nonsense"), and shipped that way for months. Nothing noticed, because nothing instantiates it.So the CPU is safe by construction, not by atomicity.
has_atomic_addalso gates the parallelisation strategy at six sites —impl/cpu/pushpull.h:125,206,428,688(push,count,pull_backward,grad_backward) andimpl/cpu/restrict.h:55,204:Since the predicate is always false, the scatter ops always take the disjoint-slice branch and the non-atomic
+=is never contended. Correct — but one "fix" to the detector away from silent lost updates. Hence the newtests/kernels/atomic/test.cpp, which pins it rather than leaving it as a comment.3. Turning parallelism on found a real deadlock
-DFF_GRAIN_SIZE=1+ TSan, first run:test_pushpullhung. gdb: main thread blocked infuture::get()insideinvoke_parallel(0, 9, grain=1), both workers asleep inpthread_cond_waitatthreadpool.h:183.Two defects, both in
threadpool.h:Worker::mExitis a plainbool(line 160), written by the pool destructor on the main thread (line 148) and read by the worker's own thread (line 164).wake()(line 132) callsmCv.notify_one()without ever touchingmCvMut, andthreadFunc()(line 183) waits on it with no predicate. The worker can check "no work", the pusher can enqueue and notify, and the worker then sleeps on work already in its queue. Becauseparallel_foris always called from the main thread,pushWork()never matches a worker id and every task lands onmWorkers.front()— so one missed wakeup on that single worker stalls the entire call.Both reproduce on
mainat f63c7d8 with a 12-line driver (parallel_for(0, 9, grain=1)in a loop). The two defects fail differently, and it is worth being precise about which:main@ f63c7d8FF_NUM_THREADS=4threadpool.h:148write /:164read) — deterministicFF_NUM_THREADS8/16/32/64FF_NUM_THREADS/OMP_NUM_THREADS/MKL_NUM_THREADSare all honoured, and the default ishardware_concurrency()/2— 32 workers on a 64-core node. Any productionparallel_forover more than 32768 elements is exposed.4. What this PR changes
impl/kernels/parallel.h—GRAIN_SIZEoverridable via-DFF_GRAIN_SIZE. A threshold, never a correctness switch; 32768 stays the shipping value. Satisfies refactor: prefix every remaining public macro with FF_ #91's prefix rule..github/workflows/ci.yml— atest-cpu (tsan, grain=1)job: the same 13 suites, same 59,886 checks, rebuilt with-DFF_GRAIN_SIZE=1under ThreadSanitizer withFF_NUM_THREADS=4andhalt_on_error=1. Stronger than adding one big-volume test case, and the gate's numbers do not move. Separate job because TSan and ASan cannot share a binary.impl/kernels/threadpool.h—mExitbecomesstd::atomic<bool>;wake()takes and releasesmCvMutbefore notifying; the wait is predicated on(exiting || queue non-empty). (Incidentally silences a pre-existing-Wreorder-ctoronWorker's init list.)tests/kernels/atomic/test.cpp+make test-atomics— pins §2 in-tree. Deliberately not part ofmake test.5. Verification, all re-run on the merge result
tools/test-baseline.sh --legs default,liblib)make test-lib-cpu -DFF_GRAIN_SIZE=1-fsanitize=thread,FF_NUM_THREADS=4,halt_on_error=1The first row is byte-identical to
tools/test-baseline.expectedfor everydefaultandlibrow (diffclean), so the gate is verified in the project's own diff-able format rather than by hand-counting.python3 tools/rename-macros.py --check→0 file(s) would change/include/ is clean.codespell --config .codespellrc→ clean.FF_GRAIN_SIZE=1.sanitizeleg.Known residual, deliberately not fixed here
A worker whose own queue is empty but which could steal still waits until someone wakes it, and
requestSteal()mutatesmStealIterunsynchronised — harmless while only the main thread submits, a race the moment a nestedparallel_forappears. Fixing either is a redesign of the pool, not a patch.Does not touch
atomic.h: the CUDA side of that file, the missing half/bf16 CAS fallbacks, the integer-CAS retyping bugs, the BSD-3 attribution gap, and the-gencodematrix are reported separately.