[PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet

[PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet

[Lorenzo Bianconi]

Similar to pci counterpart, reduce locking in mt76u_tx_tasklet since
q->head is managed just in mt76u_tx_tasklet and q->queued is updated
holding q->lock

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Changes since RFC:
- reset done to false in mt76u_tx_tasklet instead of in mt76u_tx_queue_skb
---
 drivers/net/wireless/mediatek/mt76/usb.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 15aeda0582e7..f06112180694 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -624,28 +624,35 @@ static void mt76u_tx_tasklet(unsigned long data)
 	int i;
 
 	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
+		u32 n_queued = 0, n_sw_queued = 0;
+		int idx;
+
 		sq = &dev->q_tx[i];
 		q = sq->q;
 
-		spin_lock_bh(&q->lock);
-		while (true) {
-			if (!q->entry[q->head].done || !q->queued)
+		while (q->queued > n_queued) {
+			if (!q->entry[q->head].done)
 				break;
 
 			if (q->entry[q->head].schedule) {
 				q->entry[q->head].schedule = false;
-				sq->swq_queued--;
+				n_sw_queued++;
 			}
 
+			idx = q->head;
 			entry = q->entry[q->head];
 			q->head = (q->head + 1) % q->ndesc;
-			q->queued--;
+			n_queued++;
 
-			spin_unlock_bh(&q->lock);
 			dev->drv->tx_complete_skb(dev, i, &entry);
-			spin_lock_bh(&q->lock);
+			q->entry[idx].done = false;
 		}
 
+		spin_lock_bh(&q->lock);
+
+		sq->swq_queued -= n_sw_queued;
+		q->queued -= n_queued;
+
 		wake = q->stopped && q->queued < q->ndesc - 8;
 		if (wake)
 			q->stopped = false;
@@ -741,7 +748,6 @@ mt76u_tx_queue_skb(struct mt76_dev *dev, enum mt76_txq_id qid,
 	if (err < 0)
 		return err;
 
-	q->entry[idx].done = false;
 	urb = q->entry[idx].urb;
 	err = mt76u_tx_setup_buffers(dev, skb, urb);
 	if (err < 0)
-- 
2.20.1

Re: [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet

[Stanislaw Gruszka]

On Sun, Mar 24, 2019 at 03:51:43PM +0100, Lorenzo Bianconi wrote:
> +		int idx;
> +
>  		sq = &dev->q_tx[i];
>  		q = sq->q;
>  
> -		spin_lock_bh(&q->lock);
> -		while (true) {
> -			if (!q->entry[q->head].done || !q->queued)
> +		while (q->queued > n_queued) {
> +			if (!q->entry[q->head].done)
>  				break;
If you place done = false here you will not need additional idx
variable.

>  			dev->drv->tx_complete_skb(dev, i, &entry);
> -			spin_lock_bh(&q->lock);
> +			q->entry[idx].done = false;
>  		}
>  
> +		spin_lock_bh(&q->lock);
This patch does not apply for me as there is missing
mt76_txq_schedule(dev, sq);

> +
> +		sq->swq_queued -= n_sw_queued;
> +		q->queued -= n_queued;
> +
Naming is confusing, it should rather be n_dequeued, n_sw_dequeued.

Stanislaw

Re: [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

> On Sun, Mar 24, 2019 at 03:51:43PM +0100, Lorenzo Bianconi wrote:
> > +		int idx;
> > +
> >  		sq = &dev->q_tx[i];
> >  		q = sq->q;
> >  
> > -		spin_lock_bh(&q->lock);
> > -		while (true) {
> > -			if (!q->entry[q->head].done || !q->queued)
> > +		while (q->queued > n_queued) {
> > +			if (!q->entry[q->head].done)
> >  				break;
> If you place done = false here you will not need additional idx
> variable.

As Felix suggested, I would set done to false at the end of the loop, after
tx_complete_skb

> 
> >  			dev->drv->tx_complete_skb(dev, i, &entry);
> > -			spin_lock_bh(&q->lock);
> > +			q->entry[idx].done = false;
> >  		}
> >  
> > +		spin_lock_bh(&q->lock);
> This patch does not apply for me as there is missing
> mt76_txq_schedule(dev, sq);

Sorry I forgot to mention this patch is based on
https://patchwork.kernel.org/patch/10856027/. Have you applied it?

> 
> > +
> > +		sq->swq_queued -= n_sw_queued;
> > +		q->queued -= n_queued;
> > +
> Naming is confusing, it should rather be n_dequeued, n_sw_dequeued.

I just followed dma counterpart naming convention, but I can modify it.

Regards,
Lorenzo

> 
> Stanislaw

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet

[Stanislaw Gruszka]

On Mon, Mar 25, 2019 at 02:00:36PM +0100, Lorenzo Bianconi wrote:
> > On Sun, Mar 24, 2019 at 03:51:43PM +0100, Lorenzo Bianconi wrote:
> > > +		int idx;
> > > +
> > >  		sq = &dev->q_tx[i];
> > >  		q = sq->q;
> > >  
> > > -		spin_lock_bh(&q->lock);
> > > -		while (true) {
> > > -			if (!q->entry[q->head].done || !q->queued)
> > > +		while (q->queued > n_queued) {
> > > +			if (!q->entry[q->head].done)
> > >  				break;
> > If you place done = false here you will not need additional idx
> > variable.
> 
> As Felix suggested, I would set done to false at the end of the loop, after
> tx_complete_skb
Why this is needed? 

> > >  			dev->drv->tx_complete_skb(dev, i, &entry);
> > > -			spin_lock_bh(&q->lock);
> > > +			q->entry[idx].done = false;
> > >  		}
> > >  
> > > +		spin_lock_bh(&q->lock);
> > This patch does not apply for me as there is missing
> > mt76_txq_schedule(dev, sq);
> 
> Sorry I forgot to mention this patch is based on
> https://patchwork.kernel.org/patch/10856027/. Have you applied it?
No.

Stanislaw

Re: [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

> On Mon, Mar 25, 2019 at 02:00:36PM +0100, Lorenzo Bianconi wrote:
> > > On Sun, Mar 24, 2019 at 03:51:43PM +0100, Lorenzo Bianconi wrote:
> > > > +		int idx;
> > > > +
> > > >  		sq = &dev->q_tx[i];
> > > >  		q = sq->q;
> > > >  
> > > > -		spin_lock_bh(&q->lock);
> > > > -		while (true) {
> > > > -			if (!q->entry[q->head].done || !q->queued)
> > > > +		while (q->queued > n_queued) {
> > > > +			if (!q->entry[q->head].done)
> > > >  				break;
> > > If you place done = false here you will not need additional idx
> > > variable.
> > 
> > As Felix suggested, I would set done to false at the end of the loop, after
> > tx_complete_skb
> Why this is needed? 

logically I think it should be the last thing to do on the current skb but
probably moving it before tx_complete_skb will not make any difference

> 
> > > >  			dev->drv->tx_complete_skb(dev, i, &entry);
> > > > -			spin_lock_bh(&q->lock);
> > > > +			q->entry[idx].done = false;
> > > >  		}
> > > >  
> > > > +		spin_lock_bh(&q->lock);
> > > This patch does not apply for me as there is missing
> > > mt76_txq_schedule(dev, sq);
> > 
> > Sorry I forgot to mention this patch is based on
> > https://patchwork.kernel.org/patch/10856027/. Have you applied it?
> No.
> 
> Stanislaw
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet

[Stanislaw Gruszka]

On Mon, Mar 25, 2019 at 02:47:35PM +0100, Lorenzo Bianconi wrote:
> > On Mon, Mar 25, 2019 at 02:00:36PM +0100, Lorenzo Bianconi wrote:
> > > > On Sun, Mar 24, 2019 at 03:51:43PM +0100, Lorenzo Bianconi wrote:
> > > > > +		int idx;
> > > > > +
> > > > >  		sq = &dev->q_tx[i];
> > > > >  		q = sq->q;
> > > > >  
> > > > > -		spin_lock_bh(&q->lock);
> > > > > -		while (true) {
> > > > > -			if (!q->entry[q->head].done || !q->queued)
> > > > > +		while (q->queued > n_queued) {
> > > > > +			if (!q->entry[q->head].done)
> > > > >  				break;
> > > > If you place done = false here you will not need additional idx
> > > > variable.
> > > 
> > > As Felix suggested, I would set done to false at the end of the loop, after
> > > tx_complete_skb
> > Why this is needed? 
> 
> logically I think it should be the last thing to do on the current skb but
Why? This is only marker that urb complete was done.

> probably moving it before tx_complete_skb will not make any difference
It will not, since code is performed in the same tasklet.

Stanislaw