Message ID | 20230530091532.2711675-1-petter@technux.se (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Kalle Valo |
Headers | show |
Series | wifi: rtw88: usb: Make work queues high prio | expand |
petter@technux.se writes: > From: Petter Mabacker <petter.mabacker@esab.se> > > 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@realtek.com> 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@esab.se> > --- > 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@kernel.org/
petter@technux.se writes: >> From: Petter Mabacker <petter.mabacker@esab.se> >> >> 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@realtek.com> 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@esab.se> >> --- >> 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@kernel.org/ 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. > >-- >https://patchwork.kernel.org/project/linux-wireless/list/ > >https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> -----Original Message----- > From: petter@technux.se <petter@technux.se> > Sent: Tuesday, May 30, 2023 9:09 PM > To: kvalo@kernel.org > Cc: Larry.Finger@lwfinger.net; andreas@fatal.se; iam@valdikss.org.ru; kernel@pengutronix.de; > linux-wireless@vger.kernel.org; linux@ulli-kroll.de; petter.mabacker@esab.se; petter@technux.se; Ping-Ke > Shih <pkshih@realtek.com>; s.hauer@pengutronix.de > Subject: Re: [PATCH] wifi: rtw88: usb: Make work queues high prio > > petter@technux.se writes: > > >> From: Petter Mabacker <petter.mabacker@esab.se> > >> > >> 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@realtek.com> 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@esab.se> > >> --- > >> 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@kernel.org/ > > 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
>> -----Original Message----- >> From: petter@technux.se <petter@technux.se> >> Sent: Tuesday, May 30, 2023 9:09 PM >> To: kvalo@kernel.org >> Cc: Larry.Finger@lwfinger.net; andreas@fatal.se; iam@valdikss.org.ru; kernel@pengutronix.de; >> linux-wireless@vger.kernel.org; linux@ulli-kroll.de; petter.mabacker@esab.se; petter@technux.se; Ping-Ke >> Shih <pkshih@realtek.com>; s.hauer@pengutronix.de >> Subject: Re: [PATCH] wifi: rtw88: usb: Make work queues high prio >> >> petter@technux.se writes: >> >> >> From: Petter Mabacker <petter.mabacker@esab.se> >> >> >> >> 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@realtek.com> 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@esab.se> >> >> --- >> >> 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@kernel.org/ >> >> 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@technux.se/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
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;