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