On 13.03.23 21:15, Alexander Wetzel wrote:
drv_wake_tx_queue() has no protection against running concurrent
multiple times. It's normally executed within the task calling
ndo_start_xmit(). But wake_txqs_tasklet is also calling into it,
regardless if the function is already running on another CPU or not.
While drivers with native iTXQ support are able to handle that, calls to
ieee80211_handle_wake_tx_queue() - used by the drivers without
native iTXQ support - must be serialized. Otherwise drivers can get
unexpected overlapping drv_tx() calls from mac80211. Which causes issues
for at least rt2800usb.
To avoid what seems to be a not needed distinction between native and
drivers using ieee80211_handle_wake_tx_queue(), the serialization is
done for drv_wake_tx_queue() here.
The serialization works by detecting and blocking concurrent calls into
drv_wake_tx_queue() and - when needed - restarting all queues after the
wake_tx_queue ops returned from the driver.
This fix here is only required when a tree has 'c850e31f79f0 ("wifi:
mac80211: add internal handler for wake_tx_queue")', which introduced
the buggy code path in mac80211. Drivers were switched to it with
'a790cc3a4fad ("wifi: mac80211: add wake_tx_queue callback to
drivers")'. But only after fixing an independent bug with commit
'4444bc2116ae ("wifi: mac80211: Proper mark iTXQs for resumption")'
problematic concurrent calls to drv_wake_tx_queue() really happened and
exposed the initial issue.
Fixes: c850e31f79f0 ("wifi: mac80211: add internal handler for wake_tx_queue")
Reported-by: Thomas Mann <rauchwolke@xxxxxxx>
Tested-by: Thomas Mann <rauchwolke@xxxxxxx>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217119
Link: https://lore.kernel.org/r/b8efebc6-4399-d0b8-b2a0-66843314616b@xxxxxxxxxxxxx/
CC: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Alexander Wetzel <alexander@xxxxxxxxxxxxxx>
---
There are multiple variations how we can fix the issue.
But this fix has to go directly into 6.2 to solve an ongoing
regression.
I tried to prevent an outright redesign while still having a
full fix:
This serialization approach confuses me a bit, and I worry that it might
cause performance regressions on mt76 somehow, though I can't point to
any specific scenario in which this would happen.
Most questionable decision here probably is, if we should fix it in
drv_wake_tx_queue() or only in ieee80211_handle_wake_tx_queue().
But the decision how to serialize - tasklet vs some kind of locking
- may also be an issue.
Personally, I did not like the overhead of checking all iTXQ for every
drv_wake_tx_queue(). These are happening per-packet AND can be easily
avoided when we are not using a tasklet. Most of the time.
Moving the scheduling to a tasklet wouldn't involve checking all iTXQ
for every wake call, only the active ones that have packets queued.
If you use a kthread instead, multiple concurrent events (and in some
cases even consecutive ones when busy enough) can be handled in a single
scheduling run, which can be flexible about which CPU to run on as well.
I also assume that drivers don't *want* concurrent drv_wake_tx_queue()
calls and it's a benefit to all drivers to serialize it.
I think the main problem with your patch is that it does not solve the
issue completely, just one instance of it. Concurrent calls to
ieee80211_handle_wake_tx_queue for multiple iTXQs belonging to the same
WMM AC are just as problematic as concurrent calls for the same iTXQ.
Your patch does not seem to handle that.
Also, mt76 is completely fine with concurrent drv_wake_tx_queue calls,
as it only uses them to schedule the main tx kthread.
In my opinion, the best approach is still to use a single kthread for
the ieee80211_handle_wake_tx_queue case and leave drivers alone that
take care of scheduling on their own.
- Felix
