On 14.03.23 12:20, Alexander Wetzel wrote:
assuming that TXQ drivers actually still call
ieee80211_txq_schedule_end() which says it's deprecated.
That even has _bh() so the tasklet can't be running anyway ...
So if the concurrency really is only TX vs. tasklet, then you could even
just keep the BHs disabled (in _start spin_unlock only and then in _end
local_bh_disable)?
Which may also be the solution for the regression in 6.2:
Do it now for ieee80211_handle_wake_tx_queue() and apply this patch
to the development tree only.
I'd argue the other way around - do it for all to fix these issues, and
then audit drivers such as iwlwifi or even make concurrency here opt-in.
Felix did see some benefits of the concurrency I think though, so he
might have a different opinion.
What about handling it that way:
Keep serializing drv_wake_tx_queue(). But use a new spinlock the drivers
can opt out from.
1) No flag set:
drv_wake_tx_queue() can be running only once at any time
2) IEEE80211_HW_CONCURRENT_AC_TX
drv_wake_tx_queue() can be running once per AC at any time
3) IEEE80211_HW_CONCURRENT
current behavior.
I think if you really insist on handling this through drv_wake_tx_queue
locking, it really shouldn't be done through extra hw flags, because the
locking requirements are too different.
I took a quick look at all the drivers that implement iTXQ themselves
and what their requirements are.
Only two drivers need changes:
- ath10k: needs a per-AC lock, because it does a scheduling round in the
wake_tx_queue call.
- iwlwifi: needs a global lock, since it touches a common list. None of
your proposed locking types are enough for this one.
The rest seem fine to me:
- ath9k has a per-hw-queue lock
- mt76 only schedules a kthread
- rtw88 uses a global lock + workqueue for scheduling
- rtw89 uses a (potentially unnecessary) ieee80211_schedule_txq call +
workqueue for scheduling
If you want to address this in the least invasive way, add a per-AC lock
to ath10k's wake_tx_queue handler, a global lock to iwlwifi, and a
per-AC lock inside ieee80211_handle_wake_tx_queue().
That way no locking is needed around the drv_wake_tx_queue calls.
- Felix
