modem: run the two OFDM decoders in parallel, one thread each - #164
Merged
Conversation
rafael2k
force-pushed
the
rx-decoder-parallel
branch
2 times, most recently
from
August 10, 2026 22:59
faafbc7 to
daecb3c
Compare
Groundwork for running the two RX decoders in parallel: before adding threads, establish that the existing ones are clean. ThreadSanitizer over a running mercury (not the unit tests -- see below) reported three races on trunk. Two are ours; this fixes both. 1. g_initialized / g_running in arq.c were `volatile bool`, written by main in arq_init while the modem RX thread reads them through arq_get_runtime_snapshot. volatile provides no inter-thread ordering and is not atomic in C, so this is a data race in the formal sense and TSan says so. C11 atomics keep plain read/write syntax, so only the declarations change. 2. shutdown_ was worse than a missing qualifier. The header declared it one way and FIVE translation units -- audioio, tcp_interfaces, broadcast, ui_communication, modem -- each hand-rolled their own `extern volatile bool shutdown_;`. Declaring the same object with different types across translation units is undefined behaviour independently of the threading, and it is why changing the header alone had no effect on the audioio read: that TU never saw it. All five now match the definition. The test stub in test_tcp_interfaces.c had the same private declaration and the compiler rejected it once the rest agreed -- which is the point: a mismatch is now a build error instead of silent UB. Idle-run TSan warnings: 3 -> 1, and the survivor is entirely inside libhamlib with no mercury frames. Worth recording about the tooling: CI runs TSan on the UNIT tests only (`make -C tests SANITIZE_TSAN=1 test`), and those never spawn the modem, ARQ or audio threads -- so the whole threaded core has been unverified, which is how these sat unnoticed. `make SANITIZE_TSAN=1` does work for the main binary; the findings above come from running that under `-x null` for 40 s. Also note the integration harness redirects each mercury's stderr to a temp file it discards on success, so a TSan run through it reports nothing useful without changing the harness. Both are worth addressing separately; neither is fixed here. Noise, since that was the worry: 25 s and 60 s runs produced the same three warnings and nothing new in steady state, and the suppressions file is still empty. This is not a firehose. Gate: unit suite green, integration go test -count=1 green (246 s), TSan build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The control and payload decoders always hold DIFFERENT freedv instances -- control is DATAC16, and the payload ladder (DATAC15/4/3/1/17/QAM16C2) never includes it -- yet both ran under one global modem_freedv_lock, held across freedv_rawdatarx(). On an idle receiver that search is ~90 % of RX CPU (issue #162), so the two planes could never use more than one core between them. This is the prerequisite for running them on separate threads: without it, adding a thread would have produced exactly zero speedup and looked like threading "not helping". Two domains now, taken pool-then-instance where both are needed: modem_pool_lock - the pool table (built in init_modem, torn down in shutdown_modem) and the active-instance fields g_modem->{freedv,mode,payload_bytes_per_modem_frame}. modem_inst_lock[] - one per pooled instance, guarding USE of that instance. Instances are created once and live until shutdown, so a pointer read under the pool lock stays valid after it is dropped -- which is what lets send_modulated_data take the pool lock only long enough to read g_modem->freedv and then hold that instance's lock for the modulation itself. TX contention improves rather than regresses: it used to block BOTH decoders for the whole TX build, and now blocks at most the one decoder holding the same mode, only while keyed. The subtle part, and the bug this nearly shipped with. Moving the decode to a per-instance lock is only safe if EVERY access to an instance uses that same lock. Converting send_modulated_data first left rx_decoder_target_chunk_samples calling freedv_nin() under the POOL lock -- and freedv_nin reads mutable demodulator state, so TX and that read were on one struct freedv under two different mutexes, i.e. no exclusion at all. That is a race the old global lock did not have. Auditing every freedv_* call made while holding the pool lock found six; three touch mutable state (freedv_nin, and freedv_harq_reset/freedv_set_harq in bind_mode) and are now under the instance lock. The rest are getters of immutable config, but the rule going forward is the simple one: any access to an instance takes that instance's lock, no case-by-case judgement about which getters happen to be safe today. It showed up as an intermittent integration failure that also doubled the run time (505 s against a 246 s norm) -- worth recording, because a single green retry would have buried it. Verification: integration 4/4 green after the fix at 246/248/257 s with no outlier; ThreadSanitizer over a running mercury reports the same single pre-existing hamlib rig_open race as the baseline and, importantly, ZERO lock-order inversions, which is positive confirmation that the pool->instance ordering is consistent -- there is no path holding an instance lock that then takes the pool lock. Unit suite green. receive_modulated_data is deliberately untouched: it is reachable only from run_tests_rx and already decodes outside the pool lock behind an epoch check. No parallelism yet -- the two decoders still run in one thread. This only removes the reason they could not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Idle RX ran both planes -- DATAC16 control and the payload mode -- back to back inside a single rx_thread, so their demod costs added up in one thread. On a Pi 4 that thread is the whole real-time budget (issue #162): one core at ~96 %, the capture ring backing up, "RX backlog exceeded ~2 s cap" before any client has connected. Nothing forces them to share a thread. The two planes decode the same samples independently and never touch each other's state; the instance locks added in 30fea23 already make concurrent freedv use safe. So rx_thread becomes a dispatcher: it drains capture_buffer once and tees each chunk into a per-plane ring, and a worker thread per plane drains its own ring and decodes. The fan-out lives here rather than in audioio because capture_buffer has four writers (null/fifo/sock/ALSA) plus the SHM path where an external process writes. - flush reaches both workers through flush_req; each worker owns its demod_count, so a flush is a request rather than a direct reset - metrics accumulate per worker and the dispatcher drains them, keeping the existing max(control_snr, payload_snr) merge - a full worker ring drops the chunk and logs rate-limited, never blocks the dispatcher -- one slow plane must not stall capture - teardown stops and joins the workers before releasing rings or decoder state Measured: two threads at ~10 % each, 40 % process total, against one thread at ~96 % before. Gate: unit suite green; integration 245.3 / 245.2 / 245.0 s (baseline 246 s). TSan clean -- the only warning left is the pre-existing hamlib debugmsgsave2 one, no mercury frames, no lock-order inversions. TSan did catch a real race on the first run: the worker's mode field was a plain int written by the dispatcher and read by the worker, now _Atomic like the other shared fields.
rafael2k
force-pushed
the
rx-decoder-parallel
branch
from
August 10, 2026 23:44
daecb3c to
0ebed6b
Compare
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
1 participant
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.
Addresses the CPU half of #162 (craigerl/KM6LYW, digipi on a Pi 4): 100 % CPU
and
RX backlog exceeded ~2 s cap, appearing before any client connected— which is what rules out the ARQ layer and points at the always-on capture
path.
What was wrong
An idle station demodulates two planes over the same samples: DATAC16 control
and the payload mode, both searching continuously for a preamble that is not
there. Both ran back to back inside a single
rx_thread, so their costs addedup in one thread. On a Pi 4 that thread is the entire real-time budget: one
core pegged, the capture ring backing up, backlog warnings.
Nothing forced them to share a thread. The two planes decode the same samples
independently and never touch each other's state.
What this does
rx_threadbecomes a dispatcher: it drainscapture_bufferonce and tees eachchunk into a per-plane ring; a worker thread per plane drains its own ring and
decodes.
The fan-out lives in
rx_threadrather than in audioio becausecapture_bufferhas four writers (null/fifo/sock/ALSA) plus the SHM path wherean external process is the writer.
flush_req; each worker owns itsdemod_count, so a flush is a request rather than a direct resetexisting
max(control_snr, payload_snr)mergedispatcher — one slow plane must not stall capture
Three commits, smallest first:
3b29e6bshutdown_/g_running/g_initializedatomic, and declare them once — five TUs each had a privateextern volatile bool shutdown_;30fea23ef01d24Measured
Two threads at ~10 % each, 40 % process total, against one thread at ~96 %.
Gate
go test -count=1: 245.3 / 245.2 / 245.0 s against a 246 sbaseline — real bidirectional ARQ transfers, no regression
debugmsgsave2one, no mercury frames, 0 lock-order inversionsTSan earned its keep here — it caught a real race on the first run. The
worker's
modefield was a plainint, written by the dispatcher and read bythe worker; it is
_Atomicnow like the other shared fields. An earliermismatch in the lock split (
send_modulated_dataon the instance lock whilerx_decoder_target_chunk_samplesstayed on the pool lock) had already shown upas a 505 s intermittent against a 246 s norm, so all six pool-lock-held
freedv_*calls were audited; the rule is now "any instance access takes thatinstance's lock".
Scope — worth being explicit
This redistributes decode work across cores, it does not reduce it. That is
the right fix for the reported symptom (a single thread missing its real-time
deadline and backing up the ring) and a Pi 4 has 4 cores, so it turns "one core
pegged, ring overflowing" into "two cores busy, ring keeping up".
The work reduction is #163 (FFT burst acquisition, 2.2×, sensitivity
verified identical). The two are independent and touch different files
(
modem/modem.cvsmodem/freedv/ofdm.c); they compose, and both are based on57a4543. Landing both is what gets a Pi 4 comfortable.