| Message ID | 1f489df8bc635660a1d1fb72400e5562504c0555.1553437543.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Felix Fietkau |
| Headers | show |
| Series | mt76: usb: reduce locking in mt76u_tx_tasklet | expand |
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
> 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
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
> 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 >
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
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)
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(-)