* [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet [not found] <cover.1553437543.git.lorenzo@kernel.org> @ 2019-03-24 14:51 ` Lorenzo Bianconi 2019-03-25 12:50 ` Stanislaw Gruszka 0 siblings, 1 reply; 6+ messages in thread From: Lorenzo Bianconi @ 2019-03-24 14:51 UTC (permalink / raw) To: nbd; +Cc: linux-wireless, lorenzo.bianconi, sgruszka 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet 2019-03-24 14:51 ` [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet Lorenzo Bianconi @ 2019-03-25 12:50 ` Stanislaw Gruszka 2019-03-25 13:00 ` Lorenzo Bianconi 0 siblings, 1 reply; 6+ messages in thread From: Stanislaw Gruszka @ 2019-03-25 12:50 UTC (permalink / raw) To: Lorenzo Bianconi; +Cc: nbd, linux-wireless, lorenzo.bianconi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet 2019-03-25 12:50 ` Stanislaw Gruszka @ 2019-03-25 13:00 ` Lorenzo Bianconi 2019-03-25 13:10 ` Stanislaw Gruszka 0 siblings, 1 reply; 6+ messages in thread From: Lorenzo Bianconi @ 2019-03-25 13:00 UTC (permalink / raw) To: Stanislaw Gruszka; +Cc: Lorenzo Bianconi, nbd, linux-wireless [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet 2019-03-25 13:00 ` Lorenzo Bianconi @ 2019-03-25 13:10 ` Stanislaw Gruszka 2019-03-25 13:47 ` Lorenzo Bianconi 0 siblings, 1 reply; 6+ messages in thread From: Stanislaw Gruszka @ 2019-03-25 13:10 UTC (permalink / raw) To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, nbd, linux-wireless 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet 2019-03-25 13:10 ` Stanislaw Gruszka @ 2019-03-25 13:47 ` Lorenzo Bianconi 2019-03-25 14:31 ` Stanislaw Gruszka 0 siblings, 1 reply; 6+ messages in thread From: Lorenzo Bianconi @ 2019-03-25 13:47 UTC (permalink / raw) To: Stanislaw Gruszka; +Cc: Lorenzo Bianconi, nbd, linux-wireless [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet 2019-03-25 13:47 ` Lorenzo Bianconi @ 2019-03-25 14:31 ` Stanislaw Gruszka 0 siblings, 0 replies; 6+ messages in thread From: Stanislaw Gruszka @ 2019-03-25 14:31 UTC (permalink / raw) To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, nbd, linux-wireless 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-25 14:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1553437543.git.lorenzo@kernel.org>
2019-03-24 14:51 ` [PATCH] mt76: usb: reduce locking in mt76u_tx_tasklet Lorenzo Bianconi
2019-03-25 12:50 ` Stanislaw Gruszka
2019-03-25 13:00 ` Lorenzo Bianconi
2019-03-25 13:10 ` Stanislaw Gruszka
2019-03-25 13:47 ` Lorenzo Bianconi
2019-03-25 14:31 ` Stanislaw Gruszka
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.