Links with low data rates use much smaller aggregates and are much more
sensitive to latency added by bufferbloat.
Tune the assumed aggregation length based on the tx rate duration.
Signed-off-by: Felix Fietkau <[email protected]>
---
net/mac80211/airtime.c | 37 +++++++++++++++++++++++++------------
1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/net/mac80211/airtime.c b/net/mac80211/airtime.c
index 2d959b22b141..6a46771f9d54 100644
--- a/net/mac80211/airtime.c
+++ b/net/mac80211/airtime.c
@@ -646,27 +646,40 @@ u32 ieee80211_calc_expected_tx_airtime(struct ieee80211_hw *hw,
if (pubsta) {
struct sta_info *sta = container_of(pubsta, struct sta_info,
sta);
+ struct ieee80211_rx_status stat;
struct ieee80211_tx_rate *rate = &sta->tx_stats.last_rate;
struct rate_info *ri = &sta->tx_stats.last_rate_info;
- u32 airtime;
+ u32 duration, overhead;
+ u8 agg_shift;
- if (!(rate->flags & (IEEE80211_TX_RC_VHT_MCS |
- IEEE80211_TX_RC_MCS)))
- ampdu = false;
+ ieee80211_fill_rx_status(&stat, hw, rate, ri, band, len);
+ if (stat.encoding == RX_ENC_LEGACY || !ampdu)
+ return ieee80211_calc_rx_airtime(hw, &stat, len);
+
+ duration = ieee80211_get_rate_duration(hw, &stat, &overhead);
/*
* Assume that HT/VHT transmission on any AC except VO will
* use aggregation. Since we don't have reliable reporting
- * of aggregation length, assume an average of 16.
+ * of aggregation length, assume an average size based on the
+ * tx rate.
* This will not be very accurate, but much better than simply
- * assuming un-aggregated tx.
+ * assuming un-aggregated tx in all cases.
*/
- airtime = ieee80211_calc_tx_airtime_rate(hw, rate, ri, band,
- ampdu ? len * 16 : len);
- if (ampdu)
- airtime /= 16;
-
- return airtime;
+ if (duration > 400) /* <= VHT20 MCS2 1S */
+ agg_shift = 1;
+ else if (duration > 250) /* <= VHT20 MCS3 1S or MCS1 2S */
+ agg_shift = 2;
+ else if (duration > 150) /* <= VHT20 MCS5 1S or MCS3 2S */
+ agg_shift = 3;
+ else
+ agg_shift = 4;
+
+ duration *= len;
+ duration /= AVG_PKT_SIZE;
+ duration /= 1024;
+
+ return duration + (overhead >> agg_shift);
}
if (!conf)
--
2.28.0
Felix Fietkau <[email protected]> writes:
> Links with low data rates use much smaller aggregates and are much more
> sensitive to latency added by bufferbloat.
> Tune the assumed aggregation length based on the tx rate duration.
>
> Signed-off-by: Felix Fietkau <[email protected]>
Acked-by: Toke Høiland-Jørgensen <[email protected]>
in my tests the patch series decreases the performance of ath10k
chipsets by 50%, even if feature is not enabled.
so sorry. no ack from me.
so far i was able to reproduce it on linksys ea8500 and asrock-G10 both
with qca99xx chipset
Am 13.08.2020 um 17:52 schrieb Felix Fietkau:
> Links with low data rates use much smaller aggregates and are much more
> sensitive to latency added by bufferbloat.
> Tune the assumed aggregation length based on the tx rate duration.
>
> Signed-off-by: Felix Fietkau <[email protected]>
> ---
> net/mac80211/airtime.c | 37 +++++++++++++++++++++++++------------
> 1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/net/mac80211/airtime.c b/net/mac80211/airtime.c
> index 2d959b22b141..6a46771f9d54 100644
> --- a/net/mac80211/airtime.c
> +++ b/net/mac80211/airtime.c
> @@ -646,27 +646,40 @@ u32 ieee80211_calc_expected_tx_airtime(struct ieee80211_hw *hw,
> if (pubsta) {
> struct sta_info *sta = container_of(pubsta, struct sta_info,
> sta);
> + struct ieee80211_rx_status stat;
> struct ieee80211_tx_rate *rate = &sta->tx_stats.last_rate;
> struct rate_info *ri = &sta->tx_stats.last_rate_info;
> - u32 airtime;
> + u32 duration, overhead;
> + u8 agg_shift;
>
> - if (!(rate->flags & (IEEE80211_TX_RC_VHT_MCS |
> - IEEE80211_TX_RC_MCS)))
> - ampdu = false;
> + ieee80211_fill_rx_status(&stat, hw, rate, ri, band, len);
>
> + if (stat.encoding == RX_ENC_LEGACY || !ampdu)
> + return ieee80211_calc_rx_airtime(hw, &stat, len);
> +
> + duration = ieee80211_get_rate_duration(hw, &stat, &overhead);
> /*
> * Assume that HT/VHT transmission on any AC except VO will
> * use aggregation. Since we don't have reliable reporting
> - * of aggregation length, assume an average of 16.
> + * of aggregation length, assume an average size based on the
> + * tx rate.
> * This will not be very accurate, but much better than simply
> - * assuming un-aggregated tx.
> + * assuming un-aggregated tx in all cases.
> */
> - airtime = ieee80211_calc_tx_airtime_rate(hw, rate, ri, band,
> - ampdu ? len * 16 : len);
> - if (ampdu)
> - airtime /= 16;
> -
> - return airtime;
> + if (duration > 400) /* <= VHT20 MCS2 1S */
> + agg_shift = 1;
> + else if (duration > 250) /* <= VHT20 MCS3 1S or MCS1 2S */
> + agg_shift = 2;
> + else if (duration > 150) /* <= VHT20 MCS5 1S or MCS3 2S */
> + agg_shift = 3;
> + else
> + agg_shift = 4;
> +
> + duration *= len;
> + duration /= AVG_PKT_SIZE;
> + duration /= 1024;
> +
> + return duration + (overhead >> agg_shift);
> }
>
> if (!conf)
Sebastian Gottschall <[email protected]> writes:
> in my tests the patch series decreases the performance of ath10k
> chipsets by 50%, even if feature is not enabled.
"even if feature is not enabled" - you mean without the AQL feature bit
set in the driver? That makes no sense...
-Toke
i mean airtime fairness policy was not configured
Am 21.08.2020 um 16:58 schrieb Toke Høiland-Jørgensen:
> Sebastian Gottschall <[email protected]> writes:
>
>> in my tests the patch series decreases the performance of ath10k
>> chipsets by 50%, even if feature is not enabled.
> "even if feature is not enabled" - you mean without the AQL feature bit
> set in the driver? That makes no sense...
>
> -Toke
>
>
Sebastian Gottschall <[email protected]> writes:
> i mean airtime fairness policy was not configured
Ah, right, that makes a lot more sense :)
Still not sure why this would affect throughput, though; maybe the
change in calculation makes AQL overestimate the airtime and throttle
too much? Or does the saving of last_rate_info on tx_status_ext() work
badly for ath10k?
-Toke
On 2020-08-21 17:33, Toke Høiland-Jørgensen wrote:
> Sebastian Gottschall <[email protected]> writes:
>
>> i mean airtime fairness policy was not configured
>
> Ah, right, that makes a lot more sense :)
>
> Still not sure why this would affect throughput, though; maybe the
> change in calculation makes AQL overestimate the airtime and throttle
> too much? Or does the saving of last_rate_info on tx_status_ext() work
> badly for ath10k?
The issue turned out to be missing return code checks on converting the
tx rate to struct ieee80211_rx_status.
I'll send v2 of this series with fixes.
- Felix
Felix Fietkau <[email protected]> writes:
> On 2020-08-21 17:33, Toke Høiland-Jørgensen wrote:
>> Sebastian Gottschall <[email protected]> writes:
>>
>>> i mean airtime fairness policy was not configured
>>
>> Ah, right, that makes a lot more sense :)
>>
>> Still not sure why this would affect throughput, though; maybe the
>> change in calculation makes AQL overestimate the airtime and throttle
>> too much? Or does the saving of last_rate_info on tx_status_ext() work
>> badly for ath10k?
> The issue turned out to be missing return code checks on converting the
> tx rate to struct ieee80211_rx_status.
> I'll send v2 of this series with fixes.
Ah, great! :)
-Toke