* [PATCH 2/2] rt2x00: do not pause queue unconditionally on error path
2017-12-19 11:33 [PATCH 1/2] rt2x00: pause almost full queue early Stanislaw Gruszka
@ 2017-12-19 11:33 ` Stanislaw Gruszka
2017-12-26 11:02 ` Enrico Mioso
2017-12-26 11:01 ` [PATCH 1/2] rt2x00: pause almost full queue early Enrico Mioso
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Stanislaw Gruszka @ 2017-12-19 11:33 UTC (permalink / raw)
To: linux-wireless
Cc: Helmut Schaa, Enrico Mioso, Daniel Golle, Felix Fietkau,
Stanislaw Gruszka
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 <sgruszka@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] rt2x00: do not pause queue unconditionally on error path
2017-12-19 11:33 ` [PATCH 2/2] rt2x00: do not pause queue unconditionally on error path Stanislaw Gruszka
@ 2017-12-26 11:02 ` Enrico Mioso
0 siblings, 0 replies; 6+ messages in thread
From: Enrico Mioso @ 2017-12-26 11:02 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: linux-wireless, Helmut Schaa, Enrico Mioso, Daniel Golle,
Felix Fietkau
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 <mrkiko.rs@gmail.com>
On Tue, 19 Dec 2017, Stanislaw Gruszka wrote:
> Date: Tue, 19 Dec 2017 12:33:56
> From: Stanislaw Gruszka <sgruszka@redhat.com>
> To: linux-wireless@vger.kernel.org
> Cc: Helmut Schaa <helmut.schaa@googlemail.com>,
> Enrico Mioso <mrkiko.rs@gmail.com>, Daniel Golle <daniel@makrotopia.org>,
> Felix Fietkau <nbd@nbd.name>, Stanislaw Gruszka <sgruszka@redhat.com>
> 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 <sgruszka@redhat.com>
> ---
> 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] rt2x00: pause almost full queue early
2017-12-19 11:33 [PATCH 1/2] rt2x00: pause almost full queue early Stanislaw Gruszka
2017-12-19 11:33 ` [PATCH 2/2] rt2x00: do not pause queue unconditionally on error path Stanislaw Gruszka
@ 2017-12-26 11:01 ` Enrico Mioso
2017-12-26 11:02 ` Enrico Mioso
2018-01-08 17:40 ` [1/2] " Kalle Valo
3 siblings, 0 replies; 6+ messages in thread
From: Enrico Mioso @ 2017-12-26 11:01 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: linux-wireless, Helmut Schaa, Enrico Mioso, Daniel Golle,
Felix Fietkau
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@gmail.com
On Tue, 19 Dec 2017, Stanislaw Gruszka wrote:
> Date: Tue, 19 Dec 2017 12:33:55
> From: Stanislaw Gruszka <sgruszka@redhat.com>
> To: linux-wireless@vger.kernel.org
> Cc: Helmut Schaa <helmut.schaa@googlemail.com>,
> Enrico Mioso <mrkiko.rs@gmail.com>, Daniel Golle <daniel@makrotopia.org>,
> Felix Fietkau <nbd@nbd.name>, Stanislaw Gruszka <sgruszka@redhat.com>
> 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 <sgruszka@redhat.com>
> ---
> 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] rt2x00: pause almost full queue early
2017-12-19 11:33 [PATCH 1/2] rt2x00: pause almost full queue early Stanislaw Gruszka
2017-12-19 11:33 ` [PATCH 2/2] rt2x00: do not pause queue unconditionally on error path Stanislaw Gruszka
2017-12-26 11:01 ` [PATCH 1/2] rt2x00: pause almost full queue early Enrico Mioso
@ 2017-12-26 11:02 ` Enrico Mioso
2018-01-08 17:40 ` [1/2] " Kalle Valo
3 siblings, 0 replies; 6+ messages in thread
From: Enrico Mioso @ 2017-12-26 11:02 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: linux-wireless, Helmut Schaa, Enrico Mioso, Daniel Golle,
Felix Fietkau
Oops, sorry, wrong tag
Tested-by: Enrico Mioso <mrkiko.rs@gmail.com>
On Tue, 19 Dec 2017, Stanislaw Gruszka wrote:
> Date: Tue, 19 Dec 2017 12:33:55
> From: Stanislaw Gruszka <sgruszka@redhat.com>
> To: linux-wireless@vger.kernel.org
> Cc: Helmut Schaa <helmut.schaa@googlemail.com>,
> Enrico Mioso <mrkiko.rs@gmail.com>, Daniel Golle <daniel@makrotopia.org>,
> Felix Fietkau <nbd@nbd.name>, Stanislaw Gruszka <sgruszka@redhat.com>
> 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 <sgruszka@redhat.com>
> ---
> 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [1/2] rt2x00: pause almost full queue early
2017-12-19 11:33 [PATCH 1/2] rt2x00: pause almost full queue early Stanislaw Gruszka
` (2 preceding siblings ...)
2017-12-26 11:02 ` Enrico Mioso
@ 2018-01-08 17:40 ` Kalle Valo
3 siblings, 0 replies; 6+ messages in thread
From: Kalle Valo @ 2018-01-08 17:40 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: linux-wireless, Helmut Schaa, Enrico Mioso, Daniel Golle,
Felix Fietkau, Stanislaw Gruszka
Stanislaw Gruszka <sgruszka@redhat.com> 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 <sgruszka@redhat.com>
> Tested-by: Enrico Mioso@gmail.com
> Tested-by: Enrico Mioso <mrkiko.rs@gmail.com>
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
^ permalink raw reply [flat|nested] 6+ messages in thread