Skip to contentSkip to conversation

mt76: usb: reduce locking in mt76u_tx_tasklet

PATCH Superseded Lorenzo Bianconi 3 participants 6 messages Mar 24, 2019 – Mar 25, 2019 1 message not in the archive

Subsystems (2)
Arm MediaTek MediaTek MT76 wireless LAN driver

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Lorenzo Bianconi

author maintainer
#1 lore raw

PATCH 1/1 mt76: usb: reduce locking in mt76u_tx_tasklet

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 <[email protected]>

---
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(-)
drivers/net/wireless/mediatek/mt76/usb.c +14 -8
index 15aeda0582e7..f06112180694 100644
@@ -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)
Signature · 1 line
2.20.1
#2 lore raw

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_tx_tasklet(unsigned long data) · line 635

Quoted 11 lines · Lorenzo Bianconi
drivers/net/wireless/mediatek/mt76/usb.c +4 -3
+		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.

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_tx_tasklet(unsigned long data) · line 651

Quoted 6 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -1
 			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);

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_tx_tasklet(unsigned long data) · line 655

Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +4 -0
+
+		sq->swq_queued -= n_sw_queued;
+		q->queued -= n_queued;
+

Naming is confusing, it should rather be n_dequeued, n_sw_dequeued.

Stanislaw

#3 lore raw
Quoted 6 lines · Lorenzo Bianconi
Quoted 5 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -0
+		int idx;
+
 		sq = &dev->q_tx[i];
 		q = sq->q;
Quoted 8 lines

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_tx_tasklet(unsigned long data) · line 635

Quoted 6 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -3
-		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

Quoted 9 lines

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_tx_tasklet(unsigned long data) · line 651

Quoted 7 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -1
 			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?

Quoted 6 lines

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_tx_tasklet(unsigned long data) · line 655

Quoted 5 lines
drivers/net/wireless/mediatek/mt76/usb.c +4 -0
+
+		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

Quoted 2 lines

Stanislaw

#4 lore raw
Quoted 15 lines · Lorenzo Bianconi
Quoted 6 lines · Lorenzo Bianconi
Quoted 5 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -0
+		int idx;
+
 		sq = &dev->q_tx[i];
 		q = sq->q;
Quoted 8 lines

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_tx_tasklet(unsigned long data) · line 635

Quoted 6 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -3
-		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.

Quoted 2 lines

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

Why this is needed?

Quoted 11 lines
Quoted 8 lines

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_tx_tasklet(unsigned long data) · line 651

Quoted 6 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -1
 			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

#5 lore raw
Quoted 16 lines · Lorenzo Bianconi
Quoted 15 lines
Quoted 14 lines · Lorenzo Bianconi

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_tx_tasklet(unsigned long data) · line 635

Quoted 11 lines
drivers/net/wireless/mediatek/mt76/usb.c +4 -3
+		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.

Quoted 3 lines
Quoted 2 lines

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

Quoted 16 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/net/wireless/mediatek/mt76/usb.c +1 -1
 			dev->drv->tx_complete_skb(dev, i, &entry);
-			spin_lock_bh(&q->lock);
+			q->entry[idx].done = false;
 		}
Quoted 6 lines
Quoted 3 lines

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_tx_tasklet(unsigned long data) · line 651

Quoted 1 line
drivers/net/wireless/mediatek/mt76/usb.c +1 -0
+		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

#6 lore raw
Quoted 16 lines · Lorenzo Bianconi
Quoted 16 lines · Lorenzo Bianconi
Quoted 15 lines
Quoted 14 lines · Lorenzo Bianconi

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_tx_tasklet(unsigned long data) · line 635

Quoted 11 lines
drivers/net/wireless/mediatek/mt76/usb.c +4 -3
+		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.

Quoted 5 lines
Quoted 3 lines
Quoted 2 lines

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.

Quoted 1 line

probably moving it before tx_complete_skb will not make any difference

It will not, since code is performed in the same tasklet.

Stanislaw