The WaitForEmpty defect that reached master (f50ea8b89) was found by
this suite on a loaded multi-core host - about a dozen early returns in
fifty thousand items - and the same suite passes every time on a quiet
single-core one. A race caught once in five runs is not caught by a
CI job that runs it once.
The producer/consumer half repeats WORKSEMA_REPS times, default one, and
the workflow sets two hundred. Only the last repetition prints its
counts unless one fails, in which case the run stops there and prints
that one. The deterministic halves - the contract and the state check -
run once, as they always did.
The suite already runs in CI through tests/run-all.sh, which picks up
every directory with a build.sh; what it did not do was repeat.
Passes here, plain and under TSan, at the default and at forty.
Reported from an audit of the last two hundred commits, with the
repository's own stress test failing on a multi-core host: WaitForEmpty
returning while work was still outstanding, a dozen or so times in fifty
thousand items, across repeated runs.
The primitive published "the worker has consumed up to epoch" and "the
worker is idle" as two atomics, and a waiter read both:
store_release(seen, epoch); /* consume */
store_release(idle, 0);
...
load_acquire(idle) && load_acquire(seen) == load_acquire(work.epoch)
Two atomics are two moments. A waiter could take idle=1 from before the
worker picked the work up and seen=epoch from after, and conclude "idle
and caught up" of a state the worker was never in. The comment claiming
the store order made the pair safe was wrong: no ordering between two
variables gives a reader a coherent snapshot of both, and an acquire
load cannot retroactively change a value already read from the other.
They are one atomic word now: the low bit is idle, the rest is the
epoch. consume publishes {epoch, processing} with one store, go_idle
publishes {epoch, idle} with one store, and a waiter reads the word once
- so every state it can observe is one the worker was actually in. A
notify that lands after the worker read the epoch and before it stores
idle leaves the state idle at the older epoch, which is not empty, and
the waiter keeps waiting, which is the point. go_idle takes the epoch it
went idle at, which every caller already had in hand.
Reading the state before the epoch is what makes a late notify count as
not-empty: a notify after that load leaves the epoch ahead of the state.
tests/spsc gains a check for the property itself, rather than for the
timing that exposes it: the worker's transitions are driven one at a
time and the state read after each, including the interleaving that used
to lie - notify, worker consumes, waiter looks - which must never report
empty. It is deterministic, so it holds on a single-core host, where the
timing-based half of the suite cannot fail. It cannot be run against the
header as it was: the state was not readable as one thing, which is the
bug.
The suite passes, plain and under TSan; the box here has one core, so
the multi-core failure the audit saw could not be reproduced, and the
fix is by construction rather than by having watched it stop failing.
MTGS.cpp, MTVU.cpp and GSRendererSW.cpp compile with the Makefile's
flags. Not run in the emulator.
53f62899c put WorkSema on the libretro-common eventcounts but kept it as a
class in common/. That was the wrong shape: the point of moving onto
libretro-common is to stop carrying wrappers over it. The class is gone.
Its three users -- MTGS, MTVU's VU_Thread, and the software rasterizer's
GSJobQueue, the last of which the earlier change missed because it looked
only at pcsx2/*.cpp and not the renderer subdirectories -- each embed a
WorkEventCount and drive it through free functions in
pcsx2/WorkEventCount.h. That header is the two eventcounts, three ints,
and the seven operations inlined over them; it includes retro_atomic.h
and the two eventcount headers and nothing from common/. The relax hint
the pre-park spin uses is defined there, self-contained, so the header
does not pull common/Threading.h back in; and the one-time CPU-count read
behind the spin budget is one function in MTGS.cpp, at file scope, since
the header must not carry platform code.
From common/, with WorkSema go SpinBudget, THREADING_CPU_RELAX and the
219 lines of state machine in Semaphore.cpp: 329 lines removed, none
added. The contract test moves with the code -- tests/spsc/worksema.cpp
now exercises the header's functions and links no common/ source at all,
and its seven assertions and the 50000-item pair with WaitForEmpty
interleaved are unchanged. Plain, TSan, ASan and UBSan are clean.
The build this was verified against is the default one, with OpenGL,
Vulkan, the software renderer and multi-ISA all on: that is what found
GSJobQueue, and it is what the previous change was not built with. The
core links at 16.6 MB against the stripped build's 11.1, all twelve
suites pass, and a whole-tree search including 3rdparty finds no
remaining reference to anything removed.
Three things, and the middle one is the reason for the other two.
libretro-common is brought level with RetroArch master. Five files
differed and one of them stays as it is -- include/compat/ifaddrs.h keeps
the extern "C" guard ps2 added for DEV9, which is not upstream and without
which the core fails to link; the other four are RetroArch being ahead,
checked line by line. Three files are new: retro_procbarrier.c, the
process-wide barrier, retro_eventcount.c, and retro_asym_eventcount.c,
which is built on both. Makefile.common builds all three.
WorkSema is rewritten on two of those. It was a hand-rolled state
machine whose NotifyOfWork -- what the EE pays per GS packet with the GS
thread awake, and what MTVU's own comment already named as the fetch_add
on its hot path -- measured 6.4 ns. It is now retro_asym_eventcount's
notify, a release store and a relaxed load with no lock prefix, measured
1.2 ns in the same binary and the same run. The symmetric retro_eventcount
was measured too, at 9.6 ns, and would have been a regression: it pays an
RMW and a seq_cst load to serve many producers, and each WorkSema here
has one. The ordering the hot side drops is paid where WaitForWork parks,
by retro_procbarrier; where no barrier exists the object runs the
symmetric protocol for its whole life, decided at init, and MTGS never
needs to know. WaitForEmpty's direction -- worker to producer, once a
frame -- is the plain retro_eventcount, whose prepare/commit contract is
what STATE_FLAG_WAITING_EMPTY was reinventing.
The seven operations keep their contract exactly, and tests/spsc/
worksema.cpp pins each: CheckForWork consumes a burst of notifies as one;
WaitForWork returns at once with work pending and parks without; a timed
wait is false only on a timeout with nothing pending; Kill unblocks every
waiter and makes WaitForEmpty false; Reset revives. One subtlety is
pinned because it bit the test first: "empty" means the worker went idle
after consuming, not merely consumed, and a WaitForEmpty issued between
the two blocks -- the old state machine did the same, checked. The
producer/consumer pair runs 50000 items with WaitForEmpty interleaved
2858 times and never sees it return with work outstanding. Plain, TSan,
ASan and UBSan are all clean; TSan sees the eventcounts' atomics, not the
barrier, so the lost-wake half is only load-bearing on more than one
core.
The last part is what the rewrite made dead, plus what an audit of every
common/ object against every importer in the finished build found
alongside it: KernelSemaphore::WaitFor, whose only caller was the old
timed wait; Mutex::TryLock; both StringUtil::ReplaceAll overloads; and
DecompressBlockBC4 and BC5 with their private helper and the two
Int8ToFloat conversions that served only them -- the replacement loader
dispatches BC1, BC2, BC3 and BC7 and nothing else. Every header in
common/ is still included by something, so nothing is removed at file
level; the audit was at symbol level and discounted vtable dispatch,
which is why SettingsInterface's methods, which nm cannot see a caller
for, are untouched.
The core links, all twelve suites pass, and the patch harness gains the
three objects its Semaphore.cpp link now needs.
LibretroAdmin