Re: [Regression] rt2800usb - Wifi performance issues and connection drops

From: Alexander Wetzel <alexander@wetzel-home.de>
To: Felix Fietkau <nbd@nbd.name>,
	Linux regressions mailing list <regressions@lists.linux.dev>
Cc: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Thomas Mann <rauchwolke@gmx.net>,
	Stanislaw Gruszka <stf_xl@wp.pl>,
	Helmut Schaa <helmut.schaa@googlemail.com>,
	Johannes Berg <johannes.berg@intel.com>
Subject: Re: [Regression] rt2800usb - Wifi performance issues and connection drops
Date: Thu, 9 Mar 2023 23:13:21 +0100	[thread overview]
Message-ID: <b4427052-9e94-bce7-b745-2473be5686fa@wetzel-home.de> (raw)
In-Reply-To: <42185fa2-4191-fcf5-9c0f-fd7098bb856b@nbd.name>

On 08.03.23 12:57, Felix Fietkau wrote:
> On 08.03.23 12:41, Alexander Wetzel wrote:
>> On 08.03.23 08:52, Felix Fietkau wrote:
>>
>>>> I'm also planning to provide some more debug patches, to figuring out
>>>> which part of commit 4444bc2116ae ("wifi: mac80211: Proper mark iTXQs
>>>> for resumption") fixes the issue for you. Assuming my understanding
>>>> above is correct the patch should not really fix/break anything for
>>>> you...With the findings above I would have expected your git bisec to
>>>> identify commit a790cc3a4fad ("wifi: mac80211: add wake_tx_queue
>>>> callback to drivers") as the first broken commit...
>>> I can't point to any specific series of events where it would go 
>>> wrong, but I suspect that the problem might be the fact that you're 
>>> doing tx scheduling from within ieee80211_handle_wake_tx_queue. I 
>>> don't see how it's properly protected from potentially being called 
>>> on different CPUs concurrently.
>>>
>>> Back when I was debugging some iTXQ issues in mt76, I also had 
>>> problems when tx scheduling could happen from multiple places. My 
>>> solution was to have a single worker thread that handles tx, which is 
>>> scheduled from the wake_tx_queue op.
>>> Maybe you could do something similar in mac80211 for non-iTXQ drivers.
>>>
>>
>> I think it's already doing all of that:
>> ieee80211_handle_wake_tx_queue() is the mac80211 implementation for the
>> wake_tx_queue op. The drivers without native iTXQ support simply link it
>> to this handler.
> I know. The problem I see is that I can't find anything that guarantees 
> that .wake_tx_queue_op is not being called concurrently from multiple 
> different places. ieee80211_handle_wake_tx_queue is doing the scheduling 
> directly, instead of deferring it to a single workqueue/tasklet/thread, 
> and multiple concurrent calls to it could potentially cause issues.

Good hint, thanks.
According to the latest debug log exactly that seems to be happening:

ieee80211_wake_queue() is called by the driver and wake_txqs_tasklet 
tasklet is started. But during execution of the drv_wake_tx_queue() from 
the tasklet userspace queues a new skb and also calls into 
drv_wake_tx_queue(), which is then run overlapping...

Not sure yet how that could cause the problem. But this breaks the 
assumption that drv_wake_tx_queue() are not overlapping. And TX fails 
directly after such an overlapping TX...

I'll probably just serialize the calls and then we verify if that helps...

Alexander