linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 5.10 1/3] mac80211: fix memory leak on filtered powersave frames
@ 2020-11-11 18:33 Felix Fietkau
  2020-11-11 18:33 ` [PATCH 5.10 2/3] mac80211: minstrel: remove deferred sampling code Felix Fietkau
  2020-11-11 18:33 ` [PATCH 5.10 3/3] mac80211: minstrel: fix tx status processing corner case Felix Fietkau
  0 siblings, 2 replies; 4+ messages in thread
From: Felix Fietkau @ 2020-11-11 18:33 UTC (permalink / raw)
  To: linux-wireless; +Cc: johannes

After the status rework, ieee80211_tx_status_ext is leaking un-acknowledged
packets for stations in powersave mode.
To fix this, move the code handling those packets from __ieee80211_tx_status
into ieee80211_tx_status_ext

Reported-by: Tobias Waldvogel <tobias.waldvogel@gmail.com>
Fixes: 3318111cf63d ("mac80211: reduce duplication in tx status functions")
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/status.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 6feb45135020..3485610755ef 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -49,7 +49,8 @@ static void ieee80211_handle_filtered_frame(struct ieee80211_local *local,
 	int ac;
 
 	if (info->flags & (IEEE80211_TX_CTL_NO_PS_BUFFER |
-			   IEEE80211_TX_CTL_AMPDU)) {
+			   IEEE80211_TX_CTL_AMPDU |
+			   IEEE80211_TX_CTL_HW_80211_ENCAP)) {
 		ieee80211_free_txskb(&local->hw, skb);
 		return;
 	}
@@ -915,15 +916,6 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
 			ieee80211_mpsp_trigger_process(
 				ieee80211_get_qos_ctl(hdr), sta, true, acked);
 
-		if (!acked && test_sta_flag(sta, WLAN_STA_PS_STA)) {
-			/*
-			 * The STA is in power save mode, so assume
-			 * that this TX packet failed because of that.
-			 */
-			ieee80211_handle_filtered_frame(local, sta, skb);
-			return;
-		}
-
 		if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL) &&
 		    (ieee80211_is_data(hdr->frame_control)) &&
 		    (rates_idx != -1))
@@ -1150,6 +1142,12 @@ void ieee80211_tx_status_ext(struct ieee80211_hw *hw,
 							    -info->status.ack_signal);
 				}
 			} else if (test_sta_flag(sta, WLAN_STA_PS_STA)) {
+				/*
+				 * The STA is in power save mode, so assume
+				 * that this TX packet failed because of that.
+				 */
+				if (skb)
+					ieee80211_handle_filtered_frame(local, sta, skb);
 				return;
 			} else if (noack_success) {
 				/* nothing to do here, do not account as lost */
-- 
2.28.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 5.10 2/3] mac80211: minstrel: remove deferred sampling code
  2020-11-11 18:33 [PATCH 5.10 1/3] mac80211: fix memory leak on filtered powersave frames Felix Fietkau
@ 2020-11-11 18:33 ` Felix Fietkau
  2021-07-28 14:04   ` Thomas Graf
  2020-11-11 18:33 ` [PATCH 5.10 3/3] mac80211: minstrel: fix tx status processing corner case Felix Fietkau
  1 sibling, 1 reply; 4+ messages in thread
From: Felix Fietkau @ 2020-11-11 18:33 UTC (permalink / raw)
  To: linux-wireless; +Cc: johannes

Deferring sampling attempts to the second stage has some bad interactions
with drivers that process the rate table in hardware and use the probe flag
to indicate probing packets (e.g. most mt76 drivers). On affected drivers
it can lead to probing not working at all.

If the link conditions turn worse, it might not be such a good idea to
do a lot of sampling for lower rates in this case.

Fix this by simply skipping the sample attempt instead of deferring it,
but keep the checks that would allow it to be sampled if it was skipped
too often, but only if it has less than 95% success probability.

Also ensure that IEEE80211_TX_CTL_RATE_CTRL_PROBE is set for all probing
packets.

Cc: stable@vger.kernel.org
Fixes: cccf129f820e ("mac80211: add the 'minstrel' rate control algorithm")
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/rc80211_minstrel.c | 25 ++++---------------------
 net/mac80211/rc80211_minstrel.h |  1 -
 2 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/net/mac80211/rc80211_minstrel.c b/net/mac80211/rc80211_minstrel.c
index 86bc469a28bc..a8a940e97a9a 100644
--- a/net/mac80211/rc80211_minstrel.c
+++ b/net/mac80211/rc80211_minstrel.c
@@ -287,12 +287,6 @@ minstrel_tx_status(void *priv, struct ieee80211_supported_band *sband,
 			mi->r[ndx].stats.success += success;
 	}
 
-	if ((info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE) && (i >= 0))
-		mi->sample_packets++;
-
-	if (mi->sample_deferred > 0)
-		mi->sample_deferred--;
-
 	if (time_after(jiffies, mi->last_stats_update +
 				mp->update_interval / (mp->new_avg ? 2 : 1)))
 		minstrel_update_stats(mp, mi);
@@ -367,7 +361,7 @@ minstrel_get_rate(void *priv, struct ieee80211_sta *sta,
 		return;
 
 	delta = (mi->total_packets * sampling_ratio / 100) -
-			(mi->sample_packets + mi->sample_deferred / 2);
+			mi->sample_packets;
 
 	/* delta < 0: no sampling required */
 	prev_sample = mi->prev_sample;
@@ -376,7 +370,6 @@ minstrel_get_rate(void *priv, struct ieee80211_sta *sta,
 		return;
 
 	if (mi->total_packets >= 10000) {
-		mi->sample_deferred = 0;
 		mi->sample_packets = 0;
 		mi->total_packets = 0;
 	} else if (delta > mi->n_rates * 2) {
@@ -401,19 +394,8 @@ minstrel_get_rate(void *priv, struct ieee80211_sta *sta,
 	 * rate sampling method should be used.
 	 * Respect such rates that are not sampled for 20 interations.
 	 */
-	if (mrr_capable &&
-	    msr->perfect_tx_time > mr->perfect_tx_time &&
-	    msr->stats.sample_skipped < 20) {
-		/* Only use IEEE80211_TX_CTL_RATE_CTRL_PROBE to mark
-		 * packets that have the sampling rate deferred to the
-		 * second MRR stage. Increase the sample counter only
-		 * if the deferred sample rate was actually used.
-		 * Use the sample_deferred counter to make sure that
-		 * the sampling is not done in large bursts */
-		info->flags |= IEEE80211_TX_CTL_RATE_CTRL_PROBE;
-		rate++;
-		mi->sample_deferred++;
-	} else {
+	if (msr->perfect_tx_time < mr->perfect_tx_time ||
+	    msr->stats.sample_skipped >= 20) {
 		if (!msr->sample_limit)
 			return;
 
@@ -433,6 +415,7 @@ minstrel_get_rate(void *priv, struct ieee80211_sta *sta,
 
 	rate->idx = mi->r[ndx].rix;
 	rate->count = minstrel_get_retry_count(&mi->r[ndx], info);
+	info->flags |= IEEE80211_TX_CTL_RATE_CTRL_PROBE;
 }
 
 
diff --git a/net/mac80211/rc80211_minstrel.h b/net/mac80211/rc80211_minstrel.h
index dbb43bcd3c45..86cd80b3ffde 100644
--- a/net/mac80211/rc80211_minstrel.h
+++ b/net/mac80211/rc80211_minstrel.h
@@ -126,7 +126,6 @@ struct minstrel_sta_info {
 	u8 max_prob_rate;
 	unsigned int total_packets;
 	unsigned int sample_packets;
-	int sample_deferred;
 
 	unsigned int sample_row;
 	unsigned int sample_column;
-- 
2.28.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 5.10 3/3] mac80211: minstrel: fix tx status processing corner case
  2020-11-11 18:33 [PATCH 5.10 1/3] mac80211: fix memory leak on filtered powersave frames Felix Fietkau
  2020-11-11 18:33 ` [PATCH 5.10 2/3] mac80211: minstrel: remove deferred sampling code Felix Fietkau
@ 2020-11-11 18:33 ` Felix Fietkau
  1 sibling, 0 replies; 4+ messages in thread
From: Felix Fietkau @ 2020-11-11 18:33 UTC (permalink / raw)
  To: linux-wireless; +Cc: johannes

Some drivers fill the status rate list without setting the rate index after
the final rate to -1. minstrel_ht already deals with this, but minstrel
doesn't, which causes it to get stuck at the lowest rate on these drivers.

Fix this by checking the count as well.

Cc: stable@vger.kernel.org
Fixes: cccf129f820e ("mac80211: add the 'minstrel' rate control algorithm")
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/rc80211_minstrel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/rc80211_minstrel.c b/net/mac80211/rc80211_minstrel.c
index a8a940e97a9a..b13b1da19386 100644
--- a/net/mac80211/rc80211_minstrel.c
+++ b/net/mac80211/rc80211_minstrel.c
@@ -274,7 +274,7 @@ minstrel_tx_status(void *priv, struct ieee80211_supported_band *sband,
 	success = !!(info->flags & IEEE80211_TX_STAT_ACK);
 
 	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
-		if (ar[i].idx < 0)
+		if (ar[i].idx < 0 || !ar[i].count)
 			break;
 
 		ndx = rix_to_ndx(mi, ar[i].idx);
-- 
2.28.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 5.10 2/3] mac80211: minstrel: remove deferred sampling code
  2020-11-11 18:33 ` [PATCH 5.10 2/3] mac80211: minstrel: remove deferred sampling code Felix Fietkau
@ 2021-07-28 14:04   ` Thomas Graf
  0 siblings, 0 replies; 4+ messages in thread
From: Thomas Graf @ 2021-07-28 14:04 UTC (permalink / raw)
  To: linux-wireless, nbd

Hi,

I tracked down an issue with using the Kernel 4.9.277 and 5.4.131 back 
to this patch.

In 802.11bg mode the effective transmit rate drops to 3MBit/s.
With that patch and perfect SNR (> 45dB) I get and rc_stats:

*********************
best   __________rate_________    ________statistics________ 
________last_______    ______sum-of________
rate  [name idx airtime max_tp]  [avg(tp) avg(prob) sd(prob)] 
[prob.|retry|suc|att]  [#success | #attempts]
         1     0    9738    0.9       0.9      93.7      0.5      75.0 
  1     3 4           562   572
         2     1    4922    1.6       1.6      97.0      1.5     100.0 
  1     2 2           559   576
         5.5   2    1858    4.7       4.7      99.9      0.0     100.0 
  2     2 2           542   565
        11     3     982    9.1       9.1      94.9      1.2     100.0 
  4     3 3           560   578
         6     4    1648    5.3       5.3      95.3      1.3     100.0 
  3     3 3           538   595
         9     5    1112    8.0       8.0      97.5      1.8     100.0 
  4     2 2           560   596
        12     6     844   10.5      10.5      92.4      2.0      75.0 
  5     3 4           572   615
        18     7     576   15.5      15.4      89.0      1.4      66.6 
  5     2 3           559   609
    D   24     8     440   20.4      20.4      99.4      1.1     100.0 
  6     2 2           560   604
   C    36     9     308   29.1      29.1      98.7      1.6     100.0 
  6     2 2           565   600
  B     48    10     240   37.3      36.3      87.3      1.4      66.6 
  6     2 3           608   671
A   P  54    11     216   41.6      41.6      97.3      1.7     100.0 
6     3 3           565   620

Total packet count::    ideal 6111      lookaround 652
*********************

After reverting that patch the transmit rate is back to 20MBit/s:
*********************
best   __________rate_________    ________statistics________ 
________last_______    ______sum-of________
rate  [name idx airtime max_tp]  [avg(tp) avg(prob) sd(prob)] 
[prob.|retry|suc|att]  [#success | #attempts]
         1     0    9738    0.9       0.9      99.8      0.6     100.0 
  1     0 0            41   44
         2     1    4922    1.6       1.6     100.0      0.0     100.0 
  1     0 0            45   45
         5.5   2    1858    4.7       4.7      99.9      0.0     100.0 
  2     0 0            43   44
        11     3     982    9.1       9.1      97.0      1.4     100.0 
  4     0 0            44   45
         6     4    1648    5.3       5.3      99.9      0.0     100.0 
  3     0 0            43   44
         9     5    1112    8.0       8.0      99.9      0.0     100.0 
  4     0 0            45   47
        12     6     844   10.5       8.8      74.9      0.7       0.0 
  5     0 0            43   47
        18     7     576   15.5      15.5     100.0      0.0     100.0 
  5     0 0            42   42
    D   24     8     440   20.4      20.4      99.9      0.0     100.0 
  6     0 0            60   62
   C P  36     9     308   29.1      29.1      99.9      0.0     100.0 
  6     0 0            44   46
A      48    10     240   37.3      37.3      93.8      1.2     100.0 
6     1 1           587   618
  B     54    11     216   41.6      32.3      69.8      2.1      33.3 
  6     0 0         24506   25849

Total packet count::    ideal 4962      lookaround 552
*********************

I'm using the ath9k driver with a sparklan WPEA-121N card.

For my case reverting that patch is enough.
What I see is that IEEE80211_TX_CTL_RATE_CTRL_PROBE is set but not used 
anywhere in the code. Maybe there is an easy and better fix than just 
reverting.

Regards,

Thomas

Am 11.11.2020 um 19:33 schrieb Felix Fietkau:
> Deferring sampling attempts to the second stage has some bad interactions
> with drivers that process the rate table in hardware and use the probe flag
> to indicate probing packets (e.g. most mt76 drivers). On affected drivers
> it can lead to probing not working at all.
> 
> If the link conditions turn worse, it might not be such a good idea to
> do a lot of sampling for lower rates in this case.
> 
> Fix this by simply skipping the sample attempt instead of deferring it,
> but keep the checks that would allow it to be sampled if it was skipped
> too often, but only if it has less than 95% success probability.
> 
> Also ensure that IEEE80211_TX_CTL_RATE_CTRL_PROBE is set for all probing
> packets.
> 
> Cc: stable@vger.kernel.org
> Fixes: cccf129f820e ("mac80211: add the 'minstrel' rate control algorithm")
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>   net/mac80211/rc80211_minstrel.c | 25 ++++---------------------
>   net/mac80211/rc80211_minstrel.h |  1 -
>   2 files changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/net/mac80211/rc80211_minstrel.c b/net/mac80211/rc80211_minstrel.c
> index 86bc469a28bc..a8a940e97a9a 100644
> --- a/net/mac80211/rc80211_minstrel.c
> +++ b/net/mac80211/rc80211_minstrel.c
> @@ -287,12 +287,6 @@ minstrel_tx_status(void *priv, struct ieee80211_supported_band *sband,
>   			mi->r[ndx].stats.success += success;
>   	}
>   
> -	if ((info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE) && (i >= 0))
> -		mi->sample_packets++;
> -
> -	if (mi->sample_deferred > 0)
> -		mi->sample_deferred--;
> -
>   	if (time_after(jiffies, mi->last_stats_update +
>   				mp->update_interval / (mp->new_avg ? 2 : 1)))
>   		minstrel_update_stats(mp, mi);
> @@ -367,7 +361,7 @@ minstrel_get_rate(void *priv, struct ieee80211_sta *sta,
>   		return;
>   
>   	delta = (mi->total_packets * sampling_ratio / 100) -
> -			(mi->sample_packets + mi->sample_deferred / 2);
> +			mi->sample_packets;
>   
>   	/* delta < 0: no sampling required */
>   	prev_sample = mi->prev_sample;
> @@ -376,7 +370,6 @@ minstrel_get_rate(void *priv, struct ieee80211_sta *sta,
>   		return;
>   
>   	if (mi->total_packets >= 10000) {
> -		mi->sample_deferred = 0;
>   		mi->sample_packets = 0;
>   		mi->total_packets = 0;
>   	} else if (delta > mi->n_rates * 2) {
> @@ -401,19 +394,8 @@ minstrel_get_rate(void *priv, struct ieee80211_sta *sta,
>   	 * rate sampling method should be used.
>   	 * Respect such rates that are not sampled for 20 interations.
>   	 */
> -	if (mrr_capable &&
> -	    msr->perfect_tx_time > mr->perfect_tx_time &&
> -	    msr->stats.sample_skipped < 20) {
> -		/* Only use IEEE80211_TX_CTL_RATE_CTRL_PROBE to mark
> -		 * packets that have the sampling rate deferred to the
> -		 * second MRR stage. Increase the sample counter only
> -		 * if the deferred sample rate was actually used.
> -		 * Use the sample_deferred counter to make sure that
> -		 * the sampling is not done in large bursts */
> -		info->flags |= IEEE80211_TX_CTL_RATE_CTRL_PROBE;
> -		rate++;
> -		mi->sample_deferred++;
> -	} else {
> +	if (msr->perfect_tx_time < mr->perfect_tx_time ||
> +	    msr->stats.sample_skipped >= 20) {
>   		if (!msr->sample_limit)
>   			return;
>   
> @@ -433,6 +415,7 @@ minstrel_get_rate(void *priv, struct ieee80211_sta *sta,
>   
>   	rate->idx = mi->r[ndx].rix;
>   	rate->count = minstrel_get_retry_count(&mi->r[ndx], info);
> +	info->flags |= IEEE80211_TX_CTL_RATE_CTRL_PROBE;
>   }
>   
>   
> diff --git a/net/mac80211/rc80211_minstrel.h b/net/mac80211/rc80211_minstrel.h
> index dbb43bcd3c45..86cd80b3ffde 100644
> --- a/net/mac80211/rc80211_minstrel.h
> +++ b/net/mac80211/rc80211_minstrel.h
> @@ -126,7 +126,6 @@ struct minstrel_sta_info {
>   	u8 max_prob_rate;
>   	unsigned int total_packets;
>   	unsigned int sample_packets;
> -	int sample_deferred;
>   
>   	unsigned int sample_row;
>   	unsigned int sample_column;
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-07-28 14:14 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-11 18:33 [PATCH 5.10 1/3] mac80211: fix memory leak on filtered powersave frames Felix Fietkau
2020-11-11 18:33 ` [PATCH 5.10 2/3] mac80211: minstrel: remove deferred sampling code Felix Fietkau
2021-07-28 14:04   ` Thomas Graf
2020-11-11 18:33 ` [PATCH 5.10 3/3] mac80211: minstrel: fix tx status processing corner case Felix Fietkau

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox