>> -----Original Message-----
>> From: petter@xxxxxxxxxx <petter@xxxxxxxxxx>
>> Sent: Tuesday, May 30, 2023 9:09 PM
>> To: kvalo@xxxxxxxxxx
>> Cc: Larry.Finger@xxxxxxxxxxxx; andreas@xxxxxxxx; iam@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
>> linux-wireless@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxxx; petter.mabacker@xxxxxxx; petter@xxxxxxxxxx; Ping-Ke
>> Shih <pkshih@xxxxxxxxxxx>; s.hauer@xxxxxxxxxxxxxx
>> Subject: Re: [PATCH] wifi: rtw88: usb: Make work queues high prio
>>
>> petter@xxxxxxxxxx writes:
>>
>> >> From: Petter Mabacker <petter.mabacker@xxxxxxx>
>> >>
>> >> The rtw8822cu driver have problem to handle high rx or tx rates compared
>> >> with high load (such as high I/O) on slower systems, such as for example
>> >> i.MX6 SoloX and similar platforms.
>> >>
>> >> The problems are more frequent when having the access point close to the
>> >> device. On slower systems it's often enough to download a large file,
>> >> combined with generating I/O load to trigger:
>> >>
>> >> [ 374.763424] rtw_8822cu 1-1.2:1.2: failed to get tx report from firmware
>> >> [ 377.771790] rtw_8822cu 1-1.2:1.2: failed to send h2c command
>> >> [ 407.813460] rtw_8822cu 1-1.2:1.2: firmware failed to report density after scan
>> >> [ 414.965826] rtw_8822cu 1-1.2:1.2: failed to send h2c command
>> >> [ 444.993462] rtw_8822cu 1-1.2:1.2: firmware failed to report density after scan
>> >> [ 452.144551] rtw_8822cu 1-1.2:1.2: failed to send h2c command
>> >> [ 482.183445] rtw_8822cu 1-1.2:1.2: firmware failed to report density after scan
>> >> [ 489.426263] rtw_8822cu 1-1.2:1.2: failed to send h2c command
>> >>
>> >> Another way is to simply perform a wifi rescan.
>> >>
>> >> Benchmarking shows that setting a high prio workqueue for tx/rx will
>> >> significally improve things. Also compared alloc_workqueue with
>> >> alloc_ordered_workqueue, but even thou the later seems to slightly
>> >> improve things it's still quite easy to reproduce the above issues. So
>> >> that leads to the decision to go for alloc_workqueue.
>> >>
>> >> Thanks to Ping-Ke Shih <pkshih@xxxxxxxxxxx> that came up with the idea
>> >> of exploring tweaking of the work queue's within a similar discussion.
>> >>
>> >> Fixes: a82dfd33d1237 ("wifi: rtw88: Add common USB chip support")
>> >> Signed-off-by: Petter Mabacker <petter.mabacker@xxxxxxx>
>> >> ---
>> >> drivers/net/wireless/realtek/rtw88/usb.c | 4 ++--
>> >> 1 file changed, 2 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
>> >> index 44a5fafb9905..bfe0845528ec 100644
>> >> --- a/drivers/net/wireless/realtek/rtw88/usb.c
>> >> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
>> >> @@ -716,7 +716,7 @@ static int rtw_usb_init_rx(struct rtw_dev *rtwdev)
>> >> struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
>> >> int i;
>> >>
>> >> - rtwusb->rxwq = create_singlethread_workqueue("rtw88_usb: rx wq");
>> >> + rtwusb->rxwq = alloc_workqueue("rtw88_usb: rx wq", WQ_UNBOUND | WQ_HIGHPRI, 0);
>> >> if (!rtwusb->rxwq) {
>> >> rtw_err(rtwdev, "failed to create RX work queue\n");
>> >> return -ENOMEM;
>> >> @@ -750,7 +750,7 @@ static int rtw_usb_init_tx(struct rtw_dev *rtwdev)
>> >> struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
>> >> int i;
>> >>
>> >> - rtwusb->txwq = create_singlethread_workqueue("rtw88_usb: tx wq");
>> >> + rtwusb->txwq = alloc_workqueue("rtw88_usb: tx wq", WQ_UNBOUND | WQ_HIGHPRI, 0);
>> >> if (!rtwusb->txwq) {
>> >> rtw_err(rtwdev, "failed to create TX work queue\n");
>> >> return -ENOMEM;
>>
>> >Should this workqueue be ordered or not? Please check Tejun's patchset
>> >about using ordered queues:
>>
>> >https://lore.kernel.org/lkml/20230421025046.4008499-1-tj@xxxxxxxxxx/
>>
>> Thanks for pointing out this interesting patchset. As described in the
>> commit msg, I did play around with alloc_ordered_workqueue. But at least
>> on the slower systems I tested it on (i.MX6 SoloX and BCM2835) it worked
>> a bit better, but I was still able to reproduce the above mention issue.
>> So I tried to instead use alloc_workqueue and set max_active=0 and that
>> seems to be enough to make things a lot more stable.
>>
>> However after reading Tejun's patchet I'm very intersted of feedback if
>> you or someone else have comments about using alloc_workqueue with
>> max_active=0 , or if this can give some other issues? It seems to work
>> fine for me when running it also on a i.MX8 multicore system.
>>
>Both rtwusb->rxwq and rtwusb->txwq are only queued single one work respectively,
>so I thought alloc_workqueue() and alloc_ordered_workqueue() would get the same
>results, but it seems not. That is a little weird to me.
>
>I'm not familiar with workqueue, but I think we can bisect arguments to address
>what impact the results.
>
>First we can expand macro alloc_ordered_workqueue() below
> rtwusb->txwq = alloc_ordered_workqueue("rtw88_usb: tx wq", WQ_HIGHPRI);
>into
> rtwusb->txwq = alloc_workqueue("rtw88_usb: tx wq",
> WQ_UNBOUND | __WQ_ORDERED | __WQ_ORDERED_EXPLICIT |
> WQ_HIGHPRI, 1);
>
>Secondly, compare the one you are using:
> rtwusb->txwq = alloc_workqueue("rtw88_usb: tx wq", WQ_UNBOUND | WQ_HIGHPRI, 0);
>
>Then, we can align the arguments one-by-one to know which argument dominates
>the result.
>
>Ping-Ke
>
Thanks for feedback. I have encountered some issues with HW offload
scan, that might have fooled me during my benchmarking (I used rescan as
part of my stresstest). See
https://lore.kernel.org/linux-wireless/20230612133023.321060-1-petter@xxxxxxxxxx/T/#u
for more info. But I have temporarily disabled the HW offload scan so I
will re-try my benchmarking with above suggestions in mind. I will post
here when I have some results.
BR Petter
