| Message ID | 20160910193315.30738-1-toke@toke.dk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
Jim Gettys <jg@freedesktop.org> writes: >> On Sat, Sep 10, 2016 at 3:33 PM, Toke Høiland-Jørgensen <toke@toke.dk> wrote: >> >> CoDel can be too aggressive if a station sends at a very low rate, >> leading to starvation. This gets worse the more stations are present, as >> each station gets more bursty the longer the round-robin scheduling >> between stations takes. >> >> This adds dynamic adjustment of CoDel parameters per station. It uses >> the rate selection information to estimate throughput and sets more >> lenient CoDel parameters if the estimated throughput is below a >> threshold. To not change parameters too often, a hysteresis of two >> seconds is added. > > Where is this 2 second constant coming from? I'd expect it should be > of order the maximum RTT (or a small constant factor of that, which > for intercontinental connections should be 200-300ms. Well, in most cases a station is either going to be squarely below or squarely above the threshold. The hysteresis is there to deal with the exception to this, where a station's rate oscillates around the threshold. I picked two seconds as something that is far enough above the CoDel interval to hopefully let it do its thing. > More interestingly, maybe the adjustment should be related to the # of > active stations. There is no doubt the algorithm can be improved. This is just a stopgap measure to avoid starving slow stations. The CoDel parameters for slow stations could be set smarter as well, or they could be scaled with the rate instead of being threshold based. But since we have FQ, being lenient can work without affecting latency too much. > Basically, I'm pushing back about an arbitrary number apparently > picked out of thin air... ;-). You're very welcome to contribute to coming up with a better solution ;) -Toke
Hi Toke, [auto build test WARNING on ] url: https://github.com/0day-ci/linux/commits/Toke-H-iland-J-rgensen/mac80211-Dynamically-set-CoDel-parameters-per-station/20160911-033626 base: reproduce: make htmldocs All warnings (new ones prefixed by >>): >> include/net/mac80211.h:4106: warning: No description found for parameter 'pubsta' >> include/net/mac80211.h:4106: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput' >> include/net/mac80211.h:4106: warning: No description found for parameter 'pubsta' >> include/net/mac80211.h:4106: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput' >> include/net/mac80211.h:4106: warning: No description found for parameter 'pubsta' >> include/net/mac80211.h:4106: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput' >> include/net/mac80211.h:4106: warning: No description found for parameter 'pubsta' >> include/net/mac80211.h:4106: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput' >> include/net/mac80211.h:4106: warning: No description found for parameter 'pubsta' >> include/net/mac80211.h:4106: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput' >> include/net/mac80211.h:4106: warning: No description found for parameter 'pubsta' >> include/net/mac80211.h:4106: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput' >> include/net/mac80211.h:4106: warning: No description found for parameter 'pubsta' >> include/net/mac80211.h:4106: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput' >> include/net/mac80211.h:4106: warning: No description found for parameter 'pubsta' >> include/net/mac80211.h:4106: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput' >> include/net/mac80211.h:4106: warning: No description found for parameter 'pubsta' >> include/net/mac80211.h:4106: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput' >> include/net/mac80211.h:4106: warning: No description found for parameter 'pubsta' >> include/net/mac80211.h:4106: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput' vim +/pubsta +4106 include/net/mac80211.h 4090 4091 /** 4092 * ieee80211_sta_set_expected_throughput - set the expected throughput for a 4093 * station 4094 * 4095 * Call this function to notify mac80211 about a change in expected throughput 4096 * to a station. A driver for a device that does rate control in firmware can 4097 * call this function when the expected throughput estimate towards a station 4098 * changes. The information is used to tune the CoDel AQM applied to traffic 4099 * going towards that station (which can otherwise be too aggressive and cause 4100 * slow stations to starve). 4101 * 4102 * @sta: the station to set throughput for. 4103 * @thr: the current expected throughput in kbps. 4104 */ 4105 void ieee80211_sta_set_expected_throughput(struct ieee80211_sta *pubsta, > 4106 u32 thr); 4107 4108 /** 4109 * ieee80211_tx_status - transmit status callback 4110 * 4111 * Call this function for all transmitted frames after they have been 4112 * transmitted. It is permissible to not call this function for 4113 * multicast frames but this can affect statistics. 4114 * --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sat, Sep 10, 2016 at 11:54 PM, Jim Gettys <jg@freedesktop.org> wrote: > > > On Sat, Sep 10, 2016 at 3:33 PM, Toke Høiland-Jørgensen <toke@toke.dk> > wrote: >> >> CoDel can be too aggressive if a station sends at a very low rate, >> leading to starvation. This gets worse the more stations are present, as >> each station gets more bursty the longer the round-robin scheduling >> between stations takes. >> >> This adds dynamic adjustment of CoDel parameters per station. It uses >> the rate selection information to estimate throughput and sets more >> lenient CoDel parameters if the estimated throughput is below a >> threshold. To not change parameters too often, a hysteresis of two >> seconds is added. > > > Where is this 2 second constant coming from? I'd expect it should be of > order the maximum RTT (or a small constant factor of that, which for > intercontinental connections should be 200-300ms. > Indeed, from Mauritius (Africa) to remote countries like Australia, or parts of the US, we see latencies of up to 500-600ms.
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index cca510a..6e0cf85 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -4089,6 +4089,23 @@ void ieee80211_get_tx_rates(struct ieee80211_vif *vif, int max_rates); /** + * ieee80211_sta_set_expected_throughput - set the expected throughput for a + * station + * + * Call this function to notify mac80211 about a change in expected throughput + * to a station. A driver for a device that does rate control in firmware can + * call this function when the expected throughput estimate towards a station + * changes. The information is used to tune the CoDel AQM applied to traffic + * going towards that station (which can otherwise be too aggressive and cause + * slow stations to starve). + * + * @sta: the station to set throughput for. + * @thr: the current expected throughput in kbps. + */ +void ieee80211_sta_set_expected_throughput(struct ieee80211_sta *pubsta, + u32 thr); + +/** * ieee80211_tx_status - transmit status callback * * Call this function for all transmitted frames after they have been diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c index 206698b..5370f5c 100644 --- a/net/mac80211/rate.c +++ b/net/mac80211/rate.c @@ -890,6 +890,8 @@ int rate_control_set_rates(struct ieee80211_hw *hw, drv_sta_rate_tbl_update(hw_to_local(hw), sta->sdata, pubsta); + sta_update_codel_params(sta, sta_get_expected_throughput(sta)); + return 0; } EXPORT_SYMBOL(rate_control_set_rates); diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 19f14c9..6deda4a 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -20,6 +20,7 @@ #include <linux/timer.h> #include <linux/rtnetlink.h> +#include <net/codel.h> #include <net/mac80211.h> #include "ieee80211_i.h" #include "driver-ops.h" @@ -419,6 +420,8 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata, sta->sta.max_rc_amsdu_len = IEEE80211_MAX_MPDU_LEN_HT_BA; + sta_update_codel_params(sta, 0); + sta_dbg(sdata, "Allocated STA %pM\n", sta->sta.addr); return sta; @@ -2306,6 +2309,15 @@ u32 sta_get_expected_throughput(struct sta_info *sta) return thr; } +void ieee80211_sta_set_expected_throughput(struct ieee80211_sta *pubsta, + u32 thr) +{ + struct sta_info *sta = container_of(pubsta, struct sta_info, sta); + + sta_update_codel_params(sta, thr); +} +EXPORT_SYMBOL(ieee80211_sta_set_expected_throughput); + unsigned long ieee80211_sta_last_active(struct sta_info *sta) { struct ieee80211_sta_rx_stats *stats = sta_get_last_rx_stats(sta); @@ -2314,3 +2326,26 @@ unsigned long ieee80211_sta_last_active(struct sta_info *sta) return stats->last_rx; return sta->status_stats.last_ack; } + +void sta_update_codel_params(struct sta_info *sta, u32 thr) +{ + u64 now = ktime_get_ns(); + + if (!sta->sdata->local->ops->wake_tx_queue) + return; + + if (now - sta->cparams.update_time < STA_CPARAMS_HYSTERESIS) + return; + + if (thr && thr < STA_SLOW_THRESHOLD) { + sta->cparams.params.target = MS2TIME(50); + sta->cparams.params.interval = MS2TIME(300); + sta->cparams.params.ecn = false; + } else { + sta->cparams.params.target = MS2TIME(20); + sta->cparams.params.interval = MS2TIME(100); + sta->cparams.params.ecn = true; + } + sta->cparams.params.ce_threshold = CODEL_DISABLED_THRESHOLD; + sta->cparams.update_time = now; +} diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h index 0556be3..ad088ff 100644 --- a/net/mac80211/sta_info.h +++ b/net/mac80211/sta_info.h @@ -384,6 +384,14 @@ struct ieee80211_sta_rx_stats { u64 msdu[IEEE80211_NUM_TIDS + 1]; }; +#define STA_CPARAMS_HYSTERESIS (2L * NSEC_PER_SEC) +#define STA_SLOW_THRESHOLD 8000 /* 8 Mbps */ + +struct sta_codel_params { + struct codel_params params; + u64 update_time; +}; + /** * struct sta_info - STA information * @@ -437,6 +445,7 @@ struct ieee80211_sta_rx_stats { * @known_smps_mode: the smps_mode the client thinks we are in. Relevant for * AP only. * @cipher_scheme: optional cipher scheme for this station + * @cparams: CoDel parameters for this station. * @reserved_tid: reserved TID (if any, otherwise IEEE80211_TID_UNRESERVED) * @fast_tx: TX fastpath information * @fast_rx: RX fastpath information @@ -540,6 +549,8 @@ struct sta_info { enum ieee80211_smps_mode known_smps_mode; const struct ieee80211_cipher_scheme *cipher_scheme; + struct sta_codel_params cparams; + u8 reserved_tid; struct cfg80211_chan_def tdls_chandef; @@ -713,6 +724,7 @@ void sta_set_rate_info_tx(struct sta_info *sta, void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo); u32 sta_get_expected_throughput(struct sta_info *sta); +void sta_update_codel_params(struct sta_info *sta, u32 thr); void ieee80211_sta_expire(struct ieee80211_sub_if_data *sdata, unsigned long exp_time); diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index efc38e7..ec60ac1 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -1342,9 +1342,16 @@ static struct sk_buff *fq_tin_dequeue_func(struct fq *fq, local = container_of(fq, struct ieee80211_local, fq); txqi = container_of(tin, struct txq_info, tin); - cparams = &local->cparams; cstats = &local->cstats; + if (txqi->txq.sta) { + struct sta_info *sta = container_of(txqi->txq.sta, + struct sta_info, sta); + cparams = &sta->cparams.params; + } else { + cparams = &local->cparams; + } + if (flow == &txqi->def_flow) cvars = &txqi->def_cvars; else
CoDel can be too aggressive if a station sends at a very low rate, leading to starvation. This gets worse the more stations are present, as each station gets more bursty the longer the round-robin scheduling between stations takes. This adds dynamic adjustment of CoDel parameters per station. It uses the rate selection information to estimate throughput and sets more lenient CoDel parameters if the estimated throughput is below a threshold. To not change parameters too often, a hysteresis of two seconds is added. A new callback is added that drivers can use to notify mac80211 about changes in expected throughput, so the same adjustment can be made for cards that implement rate control in firmware. Drivers that don't use this will just get the default parameters. The threshold used and the CoDel parameters set for slow stations are chosen to err on the side of caution. I.e. rather be too lenient than too aggressive. A better algorithm can then be added later. Cc: Michal Kazior <michal.kazior@tieto.com> Cc: Felix Fietkau <nbd@nbd.name> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> --- include/net/mac80211.h | 17 +++++++++++++++++ net/mac80211/rate.c | 2 ++ net/mac80211/sta_info.c | 35 +++++++++++++++++++++++++++++++++++ net/mac80211/sta_info.h | 12 ++++++++++++ net/mac80211/tx.c | 9 ++++++++- 5 files changed, 74 insertions(+), 1 deletion(-)