Re: [PATCH] wifi: mac80211: Serialize calls to drv_wake_tx_queue()

[Felix Fietkau <nbd@nbd.name>]

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@gmx.net>
> Tested-by: Thomas Mann <rauchwolke@gmx.net>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217119
> Link: https://lore.kernel.org/r/b8efebc6-4399-d0b8-b2a0-66843314616b@leemhuis.info/
> CC: <stable@vger.kernel.org>
> Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
> ---
> 
> 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