2017-12-19 11:34:08

by Stanislaw Gruszka

[permalink] [raw]
Subject: [PATCH 1/2] rt2x00: pause almost full queue early

Do not drop &queue->tx_lock and acquire it again to pause queue when
available entries are below the threshold.

Patch should mitigate problem of frequently printed errors:
"Error - Dropping frame due to full tx queue"

Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/ralink/rt2x00/rt2x00mac.c | 10 ----------
drivers/net/wireless/ralink/rt2x00/rt2x00queue.c | 8 ++++++++
2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
index ecc96312a370..c8a6f163102f 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
@@ -152,16 +152,6 @@ void rt2x00mac_tx(struct ieee80211_hw *hw,
if (unlikely(rt2x00queue_write_tx_frame(queue, skb, control->sta, false)))
goto exit_fail;

- /*
- * Pausing queue has to be serialized with rt2x00lib_txdone(). Note
- * we should not use spin_lock_bh variant as bottom halve was already
- * disabled before ieee80211_xmit() call.
- */
- spin_lock(&queue->tx_lock);
- if (rt2x00queue_threshold(queue))
- rt2x00queue_pause_queue(queue);
- spin_unlock(&queue->tx_lock);
-
return;

exit_fail:
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00queue.c b/drivers/net/wireless/ralink/rt2x00/rt2x00queue.c
index a2c1ca5c76d1..1ad51e56bc59 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00queue.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00queue.c
@@ -715,6 +715,14 @@ int rt2x00queue_write_tx_frame(struct data_queue *queue, struct sk_buff *skb,
rt2x00queue_kick_tx_queue(queue, &txdesc);

out:
+ /*
+ * Pausing queue has to be serialized with rt2x00lib_txdone(), so we
+ * do this under queue->tx_lock. Bottom halve was already disabled
+ * before ieee80211_xmit() call.
+ */
+ if (rt2x00queue_threshold(queue))
+ rt2x00queue_pause_queue(queue);
+
spin_unlock(&queue->tx_lock);
return ret;
}
--
1.9.3


2017-12-26 11:02:30

by Enrico Mioso

[permalink] [raw]
Subject: Re: [PATCH 2/2] rt2x00: do not pause queue unconditionally on error path

On a WL-330N3G device, these patches seems to make the situation much better.
In other words, I wasn't able to reproduce the stall on this chipset.
On MT7620 a stall was never seen directly, but tests with these patches revealed no stall.
So I think it's good to have these merged.

Tested-by: Enrico Mioso <[email protected]>


On Tue, 19 Dec 2017, Stanislaw Gruszka wrote:

> Date: Tue, 19 Dec 2017 12:33:56
> From: Stanislaw Gruszka <[email protected]>
> To: [email protected]
> Cc: Helmut Schaa <[email protected]>,
> Enrico Mioso <[email protected]>, Daniel Golle <[email protected]>,
> Felix Fietkau <[email protected]>, Stanislaw Gruszka <[email protected]>
> Subject: [PATCH 2/2] rt2x00: do not pause queue unconditionally on error path
>
> Pausing queue without checking threshold is racy with txdone path.
> Moreover we do not need pause queue on any error, but only if queue
> is full - in case when we send RTS frame ( other cases of almost full
> queue are already handled in rt2x00queue_write_tx_frame() ).
>
> Patch fixes of theoretically possible problem of pausing empty
> queue.
>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
> drivers/net/wireless/ralink/rt2x00/rt2x00mac.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
> index c8a6f163102f..a971bc7a6b63 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
> @@ -142,22 +142,28 @@ void rt2x00mac_tx(struct ieee80211_hw *hw,
> if (!rt2x00dev->ops->hw->set_rts_threshold &&
> (tx_info->control.rates[0].flags & (IEEE80211_TX_RC_USE_RTS_CTS |
> IEEE80211_TX_RC_USE_CTS_PROTECT))) {
> - if (rt2x00queue_available(queue) <= 1)
> - goto exit_fail;
> + if (rt2x00queue_available(queue) <= 1) {
> + /*
> + * Recheck for full queue under lock to avoid race
> + * conditions with rt2x00lib_txdone().
> + */
> + spin_lock(&queue->tx_lock);
> + if (rt2x00queue_threshold(queue))
> + rt2x00queue_pause_queue(queue);
> + spin_unlock(&queue->tx_lock);
> +
> + goto exit_free_skb;
> + }
>
> if (rt2x00mac_tx_rts_cts(rt2x00dev, queue, skb))
> - goto exit_fail;
> + goto exit_free_skb;
> }
>
> if (unlikely(rt2x00queue_write_tx_frame(queue, skb, control->sta, false)))
> - goto exit_fail;
> + goto exit_free_skb;
>
> return;
>
> - exit_fail:
> - spin_lock(&queue->tx_lock);
> - rt2x00queue_pause_queue(queue);
> - spin_unlock(&queue->tx_lock);
> exit_free_skb:
> ieee80211_free_txskb(hw, skb);
> }
> --
> 1.9.3
>
>

2017-12-26 11:02:49

by Enrico Mioso

[permalink] [raw]
Subject: Re: [PATCH 1/2] rt2x00: pause almost full queue early

Oops, sorry, wrong tag

Tested-by: Enrico Mioso <[email protected]>


On Tue, 19 Dec 2017, Stanislaw Gruszka wrote:

> Date: Tue, 19 Dec 2017 12:33:55
> From: Stanislaw Gruszka <[email protected]>
> To: [email protected]
> Cc: Helmut Schaa <[email protected]>,
> Enrico Mioso <[email protected]>, Daniel Golle <[email protected]>,
> Felix Fietkau <[email protected]>, Stanislaw Gruszka <[email protected]>
> Subject: [PATCH 1/2] rt2x00: pause almost full queue early
>
> Do not drop &queue->tx_lock and acquire it again to pause queue when
> available entries are below the threshold.
>
> Patch should mitigate problem of frequently printed errors:
> "Error - Dropping frame due to full tx queue"
>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
> drivers/net/wireless/ralink/rt2x00/rt2x00mac.c | 10 ----------
> drivers/net/wireless/ralink/rt2x00/rt2x00queue.c | 8 ++++++++
> 2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
> index ecc96312a370..c8a6f163102f 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
> @@ -152,16 +152,6 @@ void rt2x00mac_tx(struct ieee80211_hw *hw,
> if (unlikely(rt2x00queue_write_tx_frame(queue, skb, control->sta, false)))
> goto exit_fail;
>
> - /*
> - * Pausing queue has to be serialized with rt2x00lib_txdone(). Note
> - * we should not use spin_lock_bh variant as bottom halve was already
> - * disabled before ieee80211_xmit() call.
> - */
> - spin_lock(&queue->tx_lock);
> - if (rt2x00queue_threshold(queue))
> - rt2x00queue_pause_queue(queue);
> - spin_unlock(&queue->tx_lock);
> -
> return;
>
> exit_fail:
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00queue.c b/drivers/net/wireless/ralink/rt2x00/rt2x00queue.c
> index a2c1ca5c76d1..1ad51e56bc59 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00queue.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00queue.c
> @@ -715,6 +715,14 @@ int rt2x00queue_write_tx_frame(struct data_queue *queue, struct sk_buff *skb,
> rt2x00queue_kick_tx_queue(queue, &txdesc);
>
> out:
> + /*
> + * Pausing queue has to be serialized with rt2x00lib_txdone(), so we
> + * do this under queue->tx_lock. Bottom halve was already disabled
> + * before ieee80211_xmit() call.
> + */
> + if (rt2x00queue_threshold(queue))
> + rt2x00queue_pause_queue(queue);
> +
> spin_unlock(&queue->tx_lock);
> return ret;
> }
> --
> 1.9.3
>
>

2017-12-26 11:01:52

by Enrico Mioso

[permalink] [raw]
Subject: Re: [PATCH 1/2] rt2x00: pause almost full queue early

On a WL-330N3G device, these patches seems to make the situation much better.
In other words, I wasn't able to reproduce the stall on this chipset.
On MT7620 a stall was never seen directly, but tests with these patches revealed no stall.
So I think it's good to have these merged.

Tested-by: Enrico [email protected]


On Tue, 19 Dec 2017, Stanislaw Gruszka wrote:

> Date: Tue, 19 Dec 2017 12:33:55
> From: Stanislaw Gruszka <[email protected]>
> To: [email protected]
> Cc: Helmut Schaa <[email protected]>,
> Enrico Mioso <[email protected]>, Daniel Golle <[email protected]>,
> Felix Fietkau <[email protected]>, Stanislaw Gruszka <[email protected]>
> Subject: [PATCH 1/2] rt2x00: pause almost full queue early
>
> Do not drop &queue->tx_lock and acquire it again to pause queue when
> available entries are below the threshold.
>
> Patch should mitigate problem of frequently printed errors:
> "Error - Dropping frame due to full tx queue"
>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
> drivers/net/wireless/ralink/rt2x00/rt2x00mac.c | 10 ----------
> drivers/net/wireless/ralink/rt2x00/rt2x00queue.c | 8 ++++++++
> 2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
> index ecc96312a370..c8a6f163102f 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
> @@ -152,16 +152,6 @@ void rt2x00mac_tx(struct ieee80211_hw *hw,
> if (unlikely(rt2x00queue_write_tx_frame(queue, skb, control->sta, false)))
> goto exit_fail;
>
> - /*
> - * Pausing queue has to be serialized with rt2x00lib_txdone(). Note
> - * we should not use spin_lock_bh variant as bottom halve was already
> - * disabled before ieee80211_xmit() call.
> - */
> - spin_lock(&queue->tx_lock);
> - if (rt2x00queue_threshold(queue))
> - rt2x00queue_pause_queue(queue);
> - spin_unlock(&queue->tx_lock);
> -
> return;
>
> exit_fail:
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00queue.c b/drivers/net/wireless/ralink/rt2x00/rt2x00queue.c
> index a2c1ca5c76d1..1ad51e56bc59 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00queue.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00queue.c
> @@ -715,6 +715,14 @@ int rt2x00queue_write_tx_frame(struct data_queue *queue, struct sk_buff *skb,
> rt2x00queue_kick_tx_queue(queue, &txdesc);
>
> out:
> + /*
> + * Pausing queue has to be serialized with rt2x00lib_txdone(), so we
> + * do this under queue->tx_lock. Bottom halve was already disabled
> + * before ieee80211_xmit() call.
> + */
> + if (rt2x00queue_threshold(queue))
> + rt2x00queue_pause_queue(queue);
> +
> spin_unlock(&queue->tx_lock);
> return ret;
> }
> --
> 1.9.3
>
>

2017-12-19 11:34:17

by Stanislaw Gruszka

[permalink] [raw]
Subject: [PATCH 2/2] rt2x00: do not pause queue unconditionally on error path

Pausing queue without checking threshold is racy with txdone path.
Moreover we do not need pause queue on any error, but only if queue
is full - in case when we send RTS frame ( other cases of almost full
queue are already handled in rt2x00queue_write_tx_frame() ).

Patch fixes of theoretically possible problem of pausing empty
queue.

Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/ralink/rt2x00/rt2x00mac.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
index c8a6f163102f..a971bc7a6b63 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
@@ -142,22 +142,28 @@ void rt2x00mac_tx(struct ieee80211_hw *hw,
if (!rt2x00dev->ops->hw->set_rts_threshold &&
(tx_info->control.rates[0].flags & (IEEE80211_TX_RC_USE_RTS_CTS |
IEEE80211_TX_RC_USE_CTS_PROTECT))) {
- if (rt2x00queue_available(queue) <= 1)
- goto exit_fail;
+ if (rt2x00queue_available(queue) <= 1) {
+ /*
+ * Recheck for full queue under lock to avoid race
+ * conditions with rt2x00lib_txdone().
+ */
+ spin_lock(&queue->tx_lock);
+ if (rt2x00queue_threshold(queue))
+ rt2x00queue_pause_queue(queue);
+ spin_unlock(&queue->tx_lock);
+
+ goto exit_free_skb;
+ }

if (rt2x00mac_tx_rts_cts(rt2x00dev, queue, skb))
- goto exit_fail;
+ goto exit_free_skb;
}

if (unlikely(rt2x00queue_write_tx_frame(queue, skb, control->sta, false)))
- goto exit_fail;
+ goto exit_free_skb;

return;

- exit_fail:
- spin_lock(&queue->tx_lock);
- rt2x00queue_pause_queue(queue);
- spin_unlock(&queue->tx_lock);
exit_free_skb:
ieee80211_free_txskb(hw, skb);
}
--
1.9.3

2018-01-08 17:40:18

by Kalle Valo

[permalink] [raw]
Subject: Re: [1/2] rt2x00: pause almost full queue early

Stanislaw Gruszka <[email protected]> wrote:

> Do not drop &queue->tx_lock and acquire it again to pause queue when
> available entries are below the threshold.
>
> Patch should mitigate problem of frequently printed errors:
> "Error - Dropping frame due to full tx queue"
>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> Tested-by: Enrico [email protected]
> Tested-by: Enrico Mioso <[email protected]>

2 patches applied to wireless-drivers-next.git, thanks.

3d8f162cb8ec rt2x00: pause almost full queue early
6dd80efd75ce rt2x00: do not pause queue unconditionally on error path

--
https://patchwork.kernel.org/patch/10123101/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches