bes2600: Patch D — atomicize ba_lock counters, drop the spinlock #7

2026-05-07T22:18:43Z

claude-noether commented

2026-05-07 22:18:43 +00:00

Per the Opus structural critique (PR #8 §2.3 / ba_lock TODO debt) and Sonnet review item 4. The block-ack policy 4 int counters (ba_acc, ba_cnt, ba_acc_rx, ba_cnt_rx) bumped per data frame under spin_lock_bh(&hw_priv->ba_lock) are the heaviest per-frame sync cost remaining after Patch C v3.

What this patch does

Convert ba_acc, ba_cnt, ba_acc_rx, ba_cnt_rx, ba_ena to atomic_t
Add atomic_t ba_armed flag (race-free once-per-window timer arm via cmpxchg)
Remove spinlock_t ba_lock entirely
Producer hot path: atomic_inc + atomic_add + atomic_cmpxchg, no lock
Consumer paths: atomic_read + atomic_set, no lock (stats approximation tolerated)
5 files, +85/-56 net

Concurrency invariant

Producers race freely; the cmpxchg on ba_armed ensures at most ONE producer claims the timer-arm in any window. Counter increments may interleave with the timer callback's snapshot+reset (slight under-reporting on the boundary tick) — same approximation the original spinlock allowed under contention; acceptable for periodic stats.

Build verified

bes2600.ko builds clean on ohm sandbox: srcversion 912F75DF0F97145DB74F34D, 0 warnings, 0 errors.

Test plan

Reload module on ohm (reboot per dev-allocation override)
Smoke-test association + traffic
Optional Phase 7 stress at 4 MB/s — expect small _raw_spin_unlock_irqrestore drop
Fallback: /var/tmp/bes2600.patchF.rollback.ko on ohm

Remaining ba_lock-shaped TODO debt

bes2600.h still has /*TODO: Same as above */ comments on neighboring fields (ba_hist, ba_timer, ba_work). Those are single-writer or already-internally-locked.

Per the Opus structural critique (PR #8 §2.3 / ba_lock TODO debt) and Sonnet review item 4. The block-ack policy 4 int counters (ba_acc, ba_cnt, ba_acc_rx, ba_cnt_rx) bumped per data frame under `spin_lock_bh(&hw_priv->ba_lock)` are the heaviest per-frame sync cost remaining after Patch C v3. ## What this patch does - Convert ba_acc, ba_cnt, ba_acc_rx, ba_cnt_rx, ba_ena to `atomic_t` - Add `atomic_t ba_armed` flag (race-free once-per-window timer arm via cmpxchg) - Remove `spinlock_t ba_lock` entirely - Producer hot path: `atomic_inc` + `atomic_add` + `atomic_cmpxchg`, no lock - Consumer paths: `atomic_read` + `atomic_set`, no lock (stats approximation tolerated) - 5 files, +85/-56 net ## Concurrency invariant Producers race freely; the cmpxchg on ba_armed ensures at most ONE producer claims the timer-arm in any window. Counter increments may interleave with the timer callback's snapshot+reset (slight under-reporting on the boundary tick) — same approximation the original spinlock allowed under contention; acceptable for periodic stats. ## Build verified bes2600.ko builds clean on ohm sandbox: srcversion **912F75DF0F97145DB74F34D**, 0 warnings, 0 errors. ## Test plan - [ ] Reload module on ohm (reboot per dev-allocation override) - [ ] Smoke-test association + traffic - [ ] Optional Phase 7 stress at 4 MB/s — expect small _raw_spin_unlock_irqrestore drop - [ ] Fallback: /var/tmp/bes2600.patchF.rollback.ko on ohm ## Remaining ba_lock-shaped TODO debt bes2600.h still has `/*TODO: Same as above */` comments on neighboring fields (ba_hist, ba_timer, ba_work). Those are single-writer or already-internally-locked.

claude-noether added 1 commit 2026-05-07 22:18:43 +00:00

bes2600: Patch D — atomicize ba_lock counters, drop the spinlock 44b296647b

The block-ack policy uses 4 int counters (ba_acc, ba_cnt, ba_acc_rx,
ba_cnt_rx) bumped per data frame in the TX and RX hot paths under
spin_lock_bh(&hw_priv->ba_lock).  The lock was the heaviest per-frame
synchronization cost remaining after Patch C v3 (which fixed the
sdio_rx_work relay).  Per the Opus structural critique (PR #8), this
pattern matches mac80211 driver convention for per-frame statistics:
atomic_t suffices, no lock needed.

Field-by-field changes in struct bes2600_common:
  ba_acc, ba_cnt, ba_acc_rx, ba_cnt_rx: int -> atomic_t
  ba_armed:                              new atomic_t (timer-arm flag)
  ba_ena:                                bool -> atomic_t
  ba_lock:                               removed (spinlock_t deleted)
  ba_hist:                               int (single-writer = ba_timer)

Producer hot path (txrx.c TX submit + RX receive):
  - atomic_add for the byte accumulator
  - atomic_inc for the frame counter
  - atomic_cmpxchg(&ba_armed, 0, 1) to claim the once-per-window
    mod_timer arm — at most ONE producer succeeds; race-free
  - no spin_lock_bh

Consumer paths (sta.c bes2600_ba_timer, sta.c disconnect-reset, sta.c
bes2600_ba_work, debug.c debugfs reader):
  - atomic_read snapshots all 4 counters into locals; the threshold
    predicate (acc/cnt >= THLD) tolerates approximate snapshots — the
    timer fires periodically, a single misclassification just delays
    the policy update by one tick
  - atomic_set zeroes the counters at end of timer-callback window;
    racing producer increments after the snapshot are lost (acceptable
    for stats; same approximation the original lock allowed under
    contention)
  - atomic_set(&ba_armed, 0) re-enables the next window's arm

Followup-amenable simplification: ba_hist remains int because only
the single ba_timer callback writes it; multiple writers would need
to upgrade it too.

This patch follows the cw1200-mainline-idiom established by Patch C v3
(structural fix, not bandaid).  The cw1200 reference doesn't have a
similar lock to compare; bes2600 inherited this from a later
Bestechnic addition rather than the upstream tree.

marfrit merged commit 3dbabf3092 into cleanups

2026-05-07 22:19:53 +00:00

marfrit referenced this issue from a commit

2026-05-07 22:19:55 +00:00

bes2600: Patch D — atomicize ba_lock counters, drop the spinlock (#7)

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marfrit/bes2600-dkms#7