marfrit/bes2600-dkms
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

bes2600: Patch C — direct-deliver RX SKBs from sdio_rx_work #3

Closed
claude-noether wants to merge 1 commits from bes2600/sdio-rx-direct-deliver into cleanups
pull from: bes2600/sdio-rx-direct-deliver
merge into: marfrit:cleanups
Conversation 1 Commits 1 Files Changed 3
+144 -9
claude-noether commented 2026-05-07 17:50:37 +00:00
Collaborator
Copy Link
Copy Source

Patch C of the Bug-#5 series. Collapses the sdio_rx_work → rx_queue → bh_work relay into a direct call from bes2600_sdio_extract_packets into a new helper bes2600_bh_handle_rx_skb in bh.c that does the per-SKB bookkeeping the bh thread used to do.

Pre-patch baseline (4 MB/s, ~5 min, srcversion 1B3B3ED0)

  • 387,532 RX packets, 578 MB, 1.36 MB/s observed receive (Bug #5 floor)
  • sdio_rx_work 34,994 dispatches → 86.4/s = 90.3 per 1000 RX pkts
  • bes2600_bh_work redispatches: 0 (single long-lived work item)

What this saves per RX frame

  • two spin_lock(&self->rx_queue_lock) acquires (queue-tail + dequeue)
  • one bh wait-queue wake-up per IRQ batch (self->irq_handler no longer called from sdio_rx_work)

Contract (cited in bh.c block comment)

  • wsm_handle_rx: wsm.h:2108, wsm.c:2211, EXPORT_SYMBOL at wsm.c:2463. Process context, sleepable. Caller MUST hold no bes2600 spinlock; SDIO mutex is released in sdio_rx_work before extract_packets is called.
  • SKB ownership: bes2600_bh_handle_rx_skb frees on every path (success and error).
  • TX-confirm wakes still flow: when wsm_release_tx_buffer returns 1, the new helper calls bes2600_bh_wakeup() to mirror the in-bh tx=1 signaling the old bh_rx_helper did.

Minimum-diff scope

Following items kept in source for clean bisection (each is dead code after this patch but has zero functional impact):

  • struct sbus_priv->rx_queue + ->rx_queue_lock — defined and initialised, never used. Removed in a follow-up hygiene patch.
  • bes2600_sdio_pipe_read — returns NULL (rx_queue always empty). Same.
  • bes2600_bh_rx_helper — still invoked from bh, returns 0 every wake. Wastes 1 spinlock + skb_dequeue per bh wake; quantified in Phase 7 to decide cleanup priority.

Phase 7 plan

Per notes/patch-c-phase4-plan-2026-05-07.md §4.8 in marfrit/besser:

  • N=3 stress ramp: 5 min @ 1 MB/s, 10 min @ 2 MB/s, 30 min @ 4 MB/s, 30 min @ 8 MB/s
  • Identical instrumentation pre/post (workqueue ftrace + perf record + rx_packets/rx_bytes counters)
  • Predicted delta in Phase 3 units (§4.5 plan):
    • rx_queue->lock acquire rate: 1914/s → 0 (high confidence)
    • _raw_spin_unlock_irqrestore CPU%: 20% → 12-15% (medium)
    • observed RX KB/s @ 4 MB/s: floor lifts toward ≥ 1 MB/s sustained (medium)
  • Module staged on ohm at /lib/modules/$(uname -r)/extra/bes2600.ko srcversion 84239431…. Awaiting Markus's reboot button per feedback_user_pushes_reboot_button.
  • Patch B rollback at /var/tmp/bes2600.patchB.rollback.ko if Phase 7 reveals a regression.

Test plan

  • Reload module on ohm (rmmod + modprobe, or reboot)
  • Verify module loads cleanly: lsmod | grep bes2600, dmesg for any new WARN/BUG
  • Verify wpa_supplicant + iw associate against AVM AP
  • Run pre-patch baseline rep N=3 with old module FIRST (re-anchor), then post-patch N=3 with new module
  • Compare workqueue dispatch rate, _raw_spin_unlock_irqrestore CPU%, observed receive KB/s
  • If any regression: rollback to /var/tmp/bes2600.patchB.rollback.ko, file finding, loop back to Phase 4
Patch C of the Bug-#5 series. Collapses the `sdio_rx_work → rx_queue → bh_work` relay into a direct call from `bes2600_sdio_extract_packets` into a new helper `bes2600_bh_handle_rx_skb` in `bh.c` that does the per-SKB bookkeeping the bh thread used to do. ## Pre-patch baseline (4 MB/s, ~5 min, srcversion 1B3B3ED0) - 387,532 RX packets, 578 MB, **1.36 MB/s observed receive** (Bug #5 floor) - `sdio_rx_work` 34,994 dispatches → 86.4/s = **90.3 per 1000 RX pkts** - `bes2600_bh_work` redispatches: **0** (single long-lived work item) ## What this saves per RX frame - two `spin_lock(&self->rx_queue_lock)` acquires (queue-tail + dequeue) - one bh wait-queue wake-up per IRQ batch (`self->irq_handler` no longer called from sdio_rx_work) ## Contract (cited in bh.c block comment) - `wsm_handle_rx`: `wsm.h:2108`, `wsm.c:2211`, `EXPORT_SYMBOL` at `wsm.c:2463`. Process context, sleepable. Caller MUST hold no bes2600 spinlock; SDIO mutex is released in `sdio_rx_work` before `extract_packets` is called. - SKB ownership: `bes2600_bh_handle_rx_skb` frees on every path (success and error). - TX-confirm wakes still flow: when `wsm_release_tx_buffer` returns 1, the new helper calls `bes2600_bh_wakeup()` to mirror the in-bh `tx=1` signaling the old `bh_rx_helper` did. ## Minimum-diff scope Following items kept in source for clean bisection (each is dead code after this patch but has zero functional impact): - `struct sbus_priv->rx_queue` + `->rx_queue_lock` — defined and initialised, never used. Removed in a follow-up hygiene patch. - `bes2600_sdio_pipe_read` — returns NULL (rx_queue always empty). Same. - `bes2600_bh_rx_helper` — still invoked from bh, returns 0 every wake. Wastes 1 spinlock + skb_dequeue per bh wake; quantified in Phase 7 to decide cleanup priority. ## Phase 7 plan Per `notes/patch-c-phase4-plan-2026-05-07.md` §4.8 in marfrit/besser: - N=3 stress ramp: 5 min @ 1 MB/s, 10 min @ 2 MB/s, 30 min @ 4 MB/s, 30 min @ 8 MB/s - Identical instrumentation pre/post (workqueue ftrace + perf record + rx_packets/rx_bytes counters) - Predicted delta in Phase 3 units (§4.5 plan): - `rx_queue->lock` acquire rate: 1914/s → 0 (high confidence) - `_raw_spin_unlock_irqrestore` CPU%: 20% → 12-15% (medium) - observed RX KB/s @ 4 MB/s: floor lifts toward ≥ 1 MB/s sustained (medium) - Module staged on ohm at `/lib/modules/$(uname -r)/extra/bes2600.ko` srcversion `84239431…`. Awaiting Markus's reboot button per `feedback_user_pushes_reboot_button`. - Patch B rollback at `/var/tmp/bes2600.patchB.rollback.ko` if Phase 7 reveals a regression. ## Test plan - [ ] Reload module on ohm (rmmod + modprobe, or reboot) - [ ] Verify module loads cleanly: `lsmod | grep bes2600`, `dmesg` for any new WARN/BUG - [ ] Verify wpa_supplicant + iw associate against AVM AP - [ ] Run pre-patch baseline rep N=3 with old module FIRST (re-anchor), then post-patch N=3 with new module - [ ] Compare workqueue dispatch rate, _raw_spin_unlock_irqrestore CPU%, observed receive KB/s - [ ] If any regression: rollback to /var/tmp/bes2600.patchB.rollback.ko, file finding, loop back to Phase 4
claude-noether added 1 commit 2026-05-07 17:50:37 +00:00
bes2600: deliver RX SKBs directly into wsm_handle_rx from sdio_rx_work c4797b1dbf 
Patch C — collapse the sdio_rx_work → rx_queue → bh_work relay into a
direct call from bes2600_sdio_extract_packets into a new helper
bes2600_bh_handle_rx_skb that performs the per-SKB bookkeeping
previously done inside bes2600_bh_rx_helper after pipe_read.

What this saves per RX frame:
  - two spinlock acquires on self->rx_queue->lock
    (skb_queue_tail in extract_packets + skb_dequeue in pipe_read)
  - one bh wait-queue wake-up per IRQ batch
    (sdio_rx_work no longer calls self->irq_handler)

Pre-patch baseline on ohm (4 MB/s sender, ~5 min, srcversion 1B3B3ED0):
  - 387,532 RX packets, 578 MB, 1.36 MB/s observed receive
  - sdio_rx_work dispatched 34,994 times = 86.4/s = 90.3 per 1000 RX pkts
  - sdio_tx_work dispatched 111,770 times = 276.1/s
  - bes2600_bh_work redispatches: 0  (single long-lived work item,
    refutes earlier review claim of "9 events per frame")

API contract for wsm_handle_rx (cited in bh.c block comment):
  - declared wsm.h:2108, defined wsm.c:2211, EXPORT_SYMBOL wsm.c:2463
  - process context, sleepable
  - caller MUST hold no bes2600 spinlock; SDIO mutex is released at
    bes2600_sdio.c before extract_packets is called
  - SKB ownership: zeroes *skb_p if consumed, caller frees otherwise.
    bes2600_bh_handle_rx_skb frees on every path (success + error)

After patch, bh thread retains responsibility for TX work.  TX-confirm
packets that release a TX buffer wake the bh thread via
bes2600_bh_wakeup() inside bes2600_bh_handle_rx_skb (mirrors the in-bh
tx=1 signaling the old bh_rx_helper used to do).  Early-boot IRQs
before fw_started are still served via the GPIO IRQ handler's fallback
self->irq_handler path; that branch is unchanged.

Minimum-diff scope:
  - struct sbus_priv->rx_queue + ->rx_queue_lock are still defined and
    initialised (allocated but never used after this patch).  Removed
    in a follow-up hygiene patch, not bundled here.
  - bes2600_sdio_pipe_read still exists; returns NULL after this patch
    (rx_queue always empty).  Removed in same follow-up.
  - bes2600_bh_rx_helper still exists and still calls pipe_read; it
    now always returns 0.  Wasted 1 spinlock + skb_dequeue per bh wake;
    quantified in Phase 7 to decide whether worth a follow-up.

Awaiting Phase 7 verification on ohm.  Per Phase 4 plan §4.5 predicted
delta:
  - rx_queue->lock acquire rate: 1914/s -> 0  (high confidence)
  - _raw_spin_unlock_irqrestore CPU%: 20% -> 12-15%  (medium)
  - observed RX KB/s @ 4 MB/s: floor lifts toward >= 1 MB/s sustained
    (medium; rep 3's 75 KB/s death has multiple causes)

Phase 7 will N=3 ramp 1 -> 2 -> 4 -> 8 MB/s with identical
instrumentation pre/post.  See notes/patch-c-phase4-plan-2026-05-07.md
in marfrit/besser for the plan; this commit is the Phase 6 deliverable.
claude-noether commented 2026-05-07 18:47:48 +00:00
Author
Collaborator
Copy Link
Copy Source

Phase 7 verdict: FAILED — closing as broken-by-design

Loaded module srcversion 84239431… on ohm, kernel 6.19.10. Boot -1 of session, after a 5-min @ 4 MB/s nc stress run started.

May 07 20:18:10 ohm kernel: WARNING: at wsm_release_tx_buffer+0x84/0xa0 [bes2600], CPU#0: kworker/0:3H/3912
May 07 20:18:10 ohm kernel: Workqueue: bes_sdio sdio_rx_work [bes2600]
May 07 20:18:10 ohm kernel: pc : wsm_release_tx_buffer+0x84/0xa0 [bes2600]
May 07 20:18:10 ohm kernel: lr : bes2600_bh_handle_rx_skb+0x134/0x370 [bes2600]
May 07 20:18:10 ohm kernel: Call trace:
May 07 20:18:10 ohm kernel:  wsm_release_tx_buffer+0x84/0xa0 [bes2600]
May 07 20:18:10 ohm kernel:  sdio_rx_work+0x2a8/0x540 [bes2600]
May 07 20:18:10 ohm kernel: bes2600_wlan mmc2:0001:1: wsm_release_tx_buffer failed: -1

Storm fired at T+13s into stress and continued; chip wedged shortly after, host fell off network.

Root cause

wsm_release_tx_buffer() (bh.c:222) does unlocked read-modify-write on hw_priv->hw_bufs_used. Pre-Patch-C, the BH thread was the sole writer (TX-side wsm_alloc_tx_buffer increment AND RX-confirm-side decrement both ran in BH context — single-threaded by structural invariant, no lock annotation made it visible). Patch C moved the RX-confirm-side decrement into sdio_rx_work workqueue context. Now BH thread and sdio_rx_work race on the counter; one decrement underflows below zero, the WARN fires, return is -1, bookkeeping is corrupt, TX wedges.

Phase 6 contract analysis miss

The block comment in bh.c::bes2600_bh_handle_rx_skb correctly cited wsm_handle_rx's sleepability and held-lock invariants. It did NOT enumerate hw_bufs_used as shared-state-mutated-by-the-callee. Lesson saved as feedback_phase6_contract_threadsafety memory: when moving a function call between threading contexts, audit every field the function (and every transitive callee) mutates, not just the function signature.

Resolution

Closing this PR as superseded. Phase 4 v2 will be a two-step:

  1. Patch C-prep: NFC refactor — convert hw_bufs_used and hw_bufs_used_vif[] to atomic_t. Use atomic_fetch_sub_release() in wsm_release_tx_buffer (returns prior value for the >= numInpChBufs - 1 predicate). Update ~30 read sites to atomic_read(). Lands first, separately verifiable as no-functional-change.
  2. Patch C v2: re-attempt the sdio_rx_work direct-deliver on top of C-prep. Identical structural change, but now the racing counter is safe.

Rollback is staged on ohm: /var/tmp/bes2600.patchB.rollback.ko → /lib/modules/$(uname -r)/extra/bes2600.ko, depmod done. Markus's reboot button to land Patch B again. Broken Patch C module preserved at /var/tmp/bes2600.patchC-broken.ko for forensics.

Option B (atomic_t) chosen over Option A (spinlock) and Option C (route confirm back through BH) per inline discussion: atomic_t matches mac80211 driver convention (mt76, iwlwifi, ath11k all use atomic_t for hw-buffer accounting) and preserves the structural win Patch C was trying to deliver.

## Phase 7 verdict: FAILED — closing as broken-by-design **Loaded module srcversion `84239431…` on ohm, kernel 6.19.10. Boot -1 of session, after a 5-min @ 4 MB/s nc stress run started.** ``` May 07 20:18:10 ohm kernel: WARNING: at wsm_release_tx_buffer+0x84/0xa0 [bes2600], CPU#0: kworker/0:3H/3912 May 07 20:18:10 ohm kernel: Workqueue: bes_sdio sdio_rx_work [bes2600] May 07 20:18:10 ohm kernel: pc : wsm_release_tx_buffer+0x84/0xa0 [bes2600] May 07 20:18:10 ohm kernel: lr : bes2600_bh_handle_rx_skb+0x134/0x370 [bes2600] May 07 20:18:10 ohm kernel: Call trace: May 07 20:18:10 ohm kernel: wsm_release_tx_buffer+0x84/0xa0 [bes2600] May 07 20:18:10 ohm kernel: sdio_rx_work+0x2a8/0x540 [bes2600] May 07 20:18:10 ohm kernel: bes2600_wlan mmc2:0001:1: wsm_release_tx_buffer failed: -1 ``` Storm fired at T+13s into stress and continued; chip wedged shortly after, host fell off network. ## Root cause `wsm_release_tx_buffer()` (bh.c:222) does **unlocked** read-modify-write on `hw_priv->hw_bufs_used`. Pre-Patch-C, the BH thread was the sole writer (TX-side `wsm_alloc_tx_buffer` increment AND RX-confirm-side decrement both ran in BH context — single-threaded by structural invariant, no lock annotation made it visible). Patch C moved the RX-confirm-side decrement into `sdio_rx_work` workqueue context. Now BH thread and sdio_rx_work race on the counter; one decrement underflows below zero, the WARN fires, return is -1, bookkeeping is corrupt, TX wedges. ## Phase 6 contract analysis miss The block comment in `bh.c::bes2600_bh_handle_rx_skb` correctly cited `wsm_handle_rx`'s sleepability and held-lock invariants. It did NOT enumerate `hw_bufs_used` as shared-state-mutated-by-the-callee. Lesson saved as `feedback_phase6_contract_threadsafety` memory: when moving a function call between threading contexts, audit every field the function (and every transitive callee) mutates, not just the function signature. ## Resolution Closing this PR as superseded. Phase 4 v2 will be a two-step: 1. **Patch C-prep**: NFC refactor — convert `hw_bufs_used` and `hw_bufs_used_vif[]` to `atomic_t`. Use `atomic_fetch_sub_release()` in `wsm_release_tx_buffer` (returns prior value for the `>= numInpChBufs - 1` predicate). Update ~30 read sites to `atomic_read()`. Lands first, separately verifiable as no-functional-change. 2. **Patch C v2**: re-attempt the `sdio_rx_work` direct-deliver on top of C-prep. Identical structural change, but now the racing counter is safe. Rollback is staged on ohm: `/var/tmp/bes2600.patchB.rollback.ko` → `/lib/modules/$(uname -r)/extra/bes2600.ko`, depmod done. Markus's reboot button to land Patch B again. Broken Patch C module preserved at `/var/tmp/bes2600.patchC-broken.ko` for forensics. Option B (atomic_t) chosen over Option A (spinlock) and Option C (route confirm back through BH) per inline discussion: atomic_t matches mac80211 driver convention (mt76, iwlwifi, ath11k all use atomic_t for hw-buffer accounting) and preserves the structural win Patch C was trying to deliver.
claude-noether closed this pull request 2026-05-07 18:47:48 +00:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claude-noether (Claude (noether)) marfrit (Markus Fritsche)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marfrit/bes2600-dkms#3
Delete Branch "bes2600/sdio-rx-direct-deliver"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?