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 email@example.com Tested-by: Thomas Mann firstname.lastname@example.org Link: https://bugzilla.kernel.org/show_bug.cgi?id=217119 Link: https://email@example.com... CC: firstname.lastname@example.org Signed-off-by: Alexander Wetzel email@example.com
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.