Skip to contentSkip to conversation

mt76: usb: reduce locking in mt76u_tx_tasklet

RFC In review Lorenzo Bianconi 4 participants 8 messages Mar 18, 2019 – Mar 21, 2019 1 message not in the archive

Subsystems (2)
Arm MediaTek MediaTek MT76 wireless LAN driver
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

RFC 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]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +11 -7
index ac03acdae279..8cd70c32d77a 100644
@@ -634,29 +634,33 @@ 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;
+
 		sq = &dev->q_tx[i];
 		q = sq->q;
 
-		spin_lock_bh(&q->lock);
-		while (true) {
+		while (q->queued > n_queued) {
 			buf = &q->entry[q->head].ubuf;
-			if (!buf->done || !q->queued)
+			if (!buf->done)
 				break;
 
 			if (q->entry[q->head].schedule) {
 				q->entry[q->head].schedule = false;
-				sq->swq_queued--;
+				n_sw_queued++;
 			}
 
 			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);
 		}
 
+		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;
Signature · 1 line
2.20.1
#2 lore raw
Quoted 19 lines · 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 <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +2 -0
index ac03acdae279..8cd70c32d77a 100644
@@ -634,29 +634,33 @@ 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;
+

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

Quoted 10 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -3
 		sq = &dev->q_tx[i];
 		q = sq->q;
 
-		spin_lock_bh(&q->lock);
-		while (true) {
+		while (q->queued > n_queued) {
 			buf = &q->entry[q->head].ubuf;
-			if (!buf->done || !q->queued)
+			if (!buf->done)
 				break;

I'm still thinking if this is safe or not. Is somewhat tricky to read variable outside the lock because in such case there is no time guarantee when variable written on one CPU gets updated value on different CPU. And for USB is not only q->queued but also buf->done.

Stanislaw

#3 lore raw
Quoted 23 lines · Lorenzo Bianconi
Quoted 22 lines

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]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +2 -0
index ac03acdae279..8cd70c32d77a 100644
@@ -634,29 +634,33 @@ 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;
+
 		sq = &dev->q_tx[i];
 		q = sq->q;
Quoted 12 lines

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

Quoted 7 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -3
-		spin_lock_bh(&q->lock);
-		while (true) {
+		while (q->queued > n_queued) {
 			buf = &q->entry[q->head].ubuf;
-			if (!buf->done || !q->queued)
+			if (!buf->done)
 				break;

I'm still thinking if this is safe or not. Is somewhat tricky to read variable outside the lock because in such case there is no time guarantee when variable written on one CPU gets updated value on different CPU. And for USB is not only q->queued but also buf->done.

Hi Stanislaw,

I was wondering if this is safe as well, but q->queued is updated holding q->lock and I guess it will ensure to not overlap tx and status code path. Regarding buf->done, it is already updated without holding the lock in mt76u_complete_tx

Regards,
Lorenzo

Quoted 3 lines

Stanislaw

#4 lore raw
Quoted 36 lines · Lorenzo Bianconi
Quoted 23 lines · Lorenzo Bianconi
Quoted 22 lines

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]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +2 -0
index ac03acdae279..8cd70c32d77a 100644
@@ -634,29 +634,33 @@ 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;
+
 		sq = &dev->q_tx[i];
 		q = sq->q;
Quoted 12 lines

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

Quoted 7 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -3
-		spin_lock_bh(&q->lock);
-		while (true) {
+		while (q->queued > n_queued) {
 			buf = &q->entry[q->head].ubuf;
-			if (!buf->done || !q->queued)
+			if (!buf->done)
 				break;

I'm still thinking if this is safe or not. Is somewhat tricky to read variable outside the lock because in such case there is no time guarantee when variable written on one CPU gets updated value on different CPU. And for USB is not only q->queued but also buf->done.

Quoted 4 lines

Hi Stanislaw,

I was wondering if this is safe as well, but q->queued is updated holding q->lock and I guess it will ensure to not overlap tx and status code path.

Overlap will not happen, at worst what can happen is q->queued will be smaller on tx_tasklet than on tx_queue_skb.

Quoted 1 line

Regarding buf->done, it is already updated without holding the lock in mt76u_complete_tx

That's actually a bug, but it's not important, if tx_tasklet will not see updated buf->done <- true value by mt76u_complete_tx on different cpu, it will not complete skb. It will be done on next tx_tasklet iteration. Worse thing would be opposite situation.

Stanislaw

#5 lore raw
Quoted 37 lines · Lorenzo Bianconi
Quoted 36 lines
Quoted 35 lines · Lorenzo Bianconi
Quoted 19 lines

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]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +2 -0
index ac03acdae279..8cd70c32d77a 100644
@@ -634,29 +634,33 @@ 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;
+

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

Quoted 10 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -3
 		sq = &dev->q_tx[i];
 		q = sq->q;
 
-		spin_lock_bh(&q->lock);
-		while (true) {
+		while (q->queued > n_queued) {
 			buf = &q->entry[q->head].ubuf;
-			if (!buf->done || !q->queued)
+			if (!buf->done)
 				break;

I'm still thinking if this is safe or not. Is somewhat tricky to read variable outside the lock because in such case there is no time guarantee when variable written on one CPU gets updated value on different CPU. And for USB is not only q->queued but also buf->done.

Quoted 7 lines
Quoted 4 lines

Hi Stanislaw,

I was wondering if this is safe as well, but q->queued is updated holding q->lock and I guess it will ensure to not overlap tx and status code path.

Overlap will not happen, at worst what can happen is q->queued will be smaller on tx_tasklet than on tx_queue_skb.

Yes, that is the point :)

Quoted 7 lines
Quoted 2 lines

Regarding buf->done, it is already updated without holding the lock in mt76u_complete_tx

That's actually a bug, but it's not important, if tx_tasklet will not see updated buf->done <- true value by mt76u_complete_tx on different cpu, it will not complete skb. It will be done on next tx_tasklet iteration. Worse thing would be opposite situation.

Can this really occur? (since queued is update holding the lock)

Quoted 2 lines

Stanislaw

#6 lore raw
Quoted 48 lines · Lorenzo Bianconi
Quoted 37 lines · Lorenzo Bianconi
Quoted 36 lines
Quoted 35 lines · Lorenzo Bianconi
Quoted 19 lines

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]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +2 -0
index ac03acdae279..8cd70c32d77a 100644
@@ -634,29 +634,33 @@ 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;
+

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

Quoted 10 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -3
 		sq = &dev->q_tx[i];
 		q = sq->q;
 
-		spin_lock_bh(&q->lock);
-		while (true) {
+		while (q->queued > n_queued) {
 			buf = &q->entry[q->head].ubuf;
-			if (!buf->done || !q->queued)
+			if (!buf->done)
 				break;

I'm still thinking if this is safe or not. Is somewhat tricky to read variable outside the lock because in such case there is no time guarantee when variable written on one CPU gets updated value on different CPU. And for USB is not only q->queued but also buf->done.

Quoted 7 lines
Quoted 4 lines

Hi Stanislaw,

I was wondering if this is safe as well, but q->queued is updated holding q->lock and I guess it will ensure to not overlap tx and status code path.

Overlap will not happen, at worst what can happen is q->queued will be smaller on tx_tasklet than on tx_queue_skb.

Yes, that is the point :)

Quoted 1 line
Quoted 8 lines
Quoted 6 lines
Quoted 1 line

Regarding buf->done, it is already updated without holding the lock in mt76u_complete_tx

That's actually a bug, but it's not important, if tx_tasklet will not see updated buf->done <- true value by mt76u_complete_tx on different cpu, it will not complete skb. It will be done on next tx_tasklet iteration. Worse thing would be opposite situation.

Can this really occur?

I was thinking about that and yes it can occur. If q->queued and buf->done writes/read will be reordered by CPUs. To prevent that you will need to use smp_wmb/smp_rmb pair, but it's just simpler and more convenient to use lock.

Quoted 1 line

(since queued is update holding the lock)

Holding the lock on one thread without holding it on concurrent thread is irrelevant, it's the same as not holding any lock at all.

Stanislaw

author maintainer
#7 lore raw
Quoted 56 lines · Lorenzo Bianconi
Quoted 55 lines
Quoted 37 lines · Lorenzo Bianconi
Quoted 36 lines
Quoted 35 lines · Lorenzo Bianconi
Quoted 19 lines

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]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +2 -0
index ac03acdae279..8cd70c32d77a 100644
@@ -634,29 +634,33 @@ 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;
+

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

Quoted 10 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -3
 		sq = &dev->q_tx[i];
 		q = sq->q;
 
-		spin_lock_bh(&q->lock);
-		while (true) {
+		while (q->queued > n_queued) {
 			buf = &q->entry[q->head].ubuf;
-			if (!buf->done || !q->queued)
+			if (!buf->done)
 				break;

I'm still thinking if this is safe or not. Is somewhat tricky to read variable outside the lock because in such case there is no time guarantee when variable written on one CPU gets updated value on different CPU. And for USB is not only q->queued but also buf->done.

Quoted 7 lines
Quoted 4 lines

Hi Stanislaw,

I was wondering if this is safe as well, but q->queued is updated holding q->lock and I guess it will ensure to not overlap tx and status code path.

Overlap will not happen, at worst what can happen is q->queued will be smaller on tx_tasklet than on tx_queue_skb.

Yes, that is the point :)

Quoted 7 lines
Quoted 2 lines

Regarding buf->done, it is already updated without holding the lock in mt76u_complete_tx

That's actually a bug, but it's not important, if tx_tasklet will not see updated buf->done <- true value by mt76u_complete_tx on different cpu, it will not complete skb. It will be done on next tx_tasklet iteration. Worse thing would be opposite situation.

Quoted 5 lines
Quoted 1 line

Can this really occur?

I was thinking about that and yes it can occur. If q->queued and buf->done writes/read will be reordered by CPUs. To prevent that you will need to use smp_wmb/smp_rmb pair, but it's just simpler and more convenient to use lock.

good point, I will go through it.

Regards,
Lorenzo

Quoted 6 lines
Quoted 2 lines

(since queued is update holding the lock)

Holding the lock on one thread without holding it on concurrent thread is irrelevant, it's the same as not holding any lock at all.

Stanislaw

maintainer
#8 lore raw
Quoted 56 lines · Lorenzo Bianconi
Quoted 56 lines · Lorenzo Bianconi
Quoted 55 lines
Quoted 37 lines · Lorenzo Bianconi
Quoted 36 lines
Quoted 35 lines · Lorenzo Bianconi
Quoted 19 lines

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]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +2 -0
index ac03acdae279..8cd70c32d77a 100644
@@ -634,29 +634,33 @@ 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;
+

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

Quoted 10 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -3
 		sq = &dev->q_tx[i];
 		q = sq->q;
 
-		spin_lock_bh(&q->lock);
-		while (true) {
+		while (q->queued > n_queued) {
 			buf = &q->entry[q->head].ubuf;
-			if (!buf->done || !q->queued)
+			if (!buf->done)
 				break;

I'm still thinking if this is safe or not. Is somewhat tricky to read variable outside the lock because in such case there is no time guarantee when variable written on one CPU gets updated value on different CPU. And for USB is not only q->queued but also buf->done.

Quoted 7 lines
Quoted 4 lines

Hi Stanislaw,

I was wondering if this is safe as well, but q->queued is updated holding q->lock and I guess it will ensure to not overlap tx and status code path.

Overlap will not happen, at worst what can happen is q->queued will be smaller on tx_tasklet than on tx_queue_skb.

Yes, that is the point :)

Quoted 7 lines
Quoted 2 lines

Regarding buf->done, it is already updated without holding the lock in mt76u_complete_tx

That's actually a bug, but it's not important, if tx_tasklet will not see updated buf->done <- true value by mt76u_complete_tx on different cpu, it will not complete skb. It will be done on next tx_tasklet iteration. Worse thing would be opposite situation.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

Can this really occur?

I was thinking about that and yes it can occur. If q->queued and buf->done writes/read will be reordered by CPUs. To prevent that you will need to use smp_wmb/smp_rmb pair, but it's just simpler and more convenient to use lock.

good point, I will go through it.

Another simple solution would be to set buf->done = false in mt76u_tx_tasklet after tx_complete_skb instead of doing it at enqueue time.

- Felix