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(-)
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
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