| Message ID | 20160908195915.31478-1-toke@toke.dk (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
On 2016-09-08 21:59, Toke Høiland-Jørgensen wrote: > CoDel can be too aggressive if a station sends at a very low rate. This > gets worse the more stations are present, as each station gets more > bursty the longer the round-robin scheduling between stations takes. > > This is an attempt to dynamically adjust CoDel parameters per station. > It takes a rather simple approach and uses a simple binary designation > of a station as 'slow' if it has expected throughput less than eight > Mbps (i.e. the lowest couple of rates). In this case, CoDel is set to be > more lenient by adjusting its target to 50 ms and interval to 300 ms. > There's a built-in hysteresis so a station cannot flip between slow and > fast more than once every two seconds. > > In this version the check is performed every time a packet is enqueued > to the intermediate queues; and the overhead of doing this is alleviated > a bit by caching the result and by the above-mentioned hysteresis. It > can probably be smarter about both when and how to do the scaling, but > this seems to alleviate some of the starvation I've seen with very slow > stations. Since this is not dealing properly with firmware rate control anyway, I'd suggest calling the update from rate_control_set_rates in order to avoid putting more stuff into the tx hot path. You could add a separate code path to allow firmware rate control devices to provide a hint at a convenient point in time. - Felix
Felix Fietkau <nbd@nbd.name> writes: > On 2016-09-08 21:59, Toke Høiland-Jørgensen wrote: >> CoDel can be too aggressive if a station sends at a very low rate. This >> gets worse the more stations are present, as each station gets more >> bursty the longer the round-robin scheduling between stations takes. >> >> This is an attempt to dynamically adjust CoDel parameters per station. >> It takes a rather simple approach and uses a simple binary designation >> of a station as 'slow' if it has expected throughput less than eight >> Mbps (i.e. the lowest couple of rates). In this case, CoDel is set to be >> more lenient by adjusting its target to 50 ms and interval to 300 ms. >> There's a built-in hysteresis so a station cannot flip between slow and >> fast more than once every two seconds. >> >> In this version the check is performed every time a packet is enqueued >> to the intermediate queues; and the overhead of doing this is alleviated >> a bit by caching the result and by the above-mentioned hysteresis. It >> can probably be smarter about both when and how to do the scaling, but >> this seems to alleviate some of the starvation I've seen with very slow >> stations. > Since this is not dealing properly with firmware rate control anyway, > I'd suggest calling the update from rate_control_set_rates in order to > avoid putting more stuff into the tx hot path. Yeah, I wasn't expecting that to stay in the TX path, but was not familiar enough with minstrel to know where a good place would be. Thanks for the pointer. > You could add a separate code path to allow firmware rate control > devices to provide a hint at a convenient point in time. There's already a get_expected_throughput() driver hook; but no drivers seem to be using it? But I assume you mean a hook the other way, i.e. export a function where the driver can tell mac80211 "my expected throughput changed, please update"? -Toke
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 19f14c9..8bc9eba 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -20,6 +20,8 @@ #include <linux/timer.h> #include <linux/rtnetlink.h> +#include <net/codel.h> +#include <net/codel_impl.h> #include <net/mac80211.h> #include "ieee80211_i.h" #include "driver-ops.h" @@ -325,6 +327,10 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata, INIT_WORK(&sta->drv_deliver_wk, sta_deliver_ps_frames); INIT_WORK(&sta->ampdu_mlme.work, ieee80211_ba_session_work); mutex_init(&sta->ampdu_mlme.mtx); + + codel_params_init(&sta->cparams.cparams); + sta->cparams.cparams.ecn = true; + #ifdef CONFIG_MAC80211_MESH if (ieee80211_vif_is_mesh(&sdata->vif)) { sta->mesh = kzalloc(sizeof(*sta->mesh), gfp); @@ -2314,3 +2320,34 @@ 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) +{ + u64 now = ktime_get_ns(); + u32 thr; + bool slow; + + if (likely(now - sta->cparams.update_time < STA_CPARAMS_HYSTERESIS)) + return; + + thr = sta_get_expected_throughput(sta); + slow = !!(thr && thr < STA_SLOW_THRESHOLD); + + if (likely(slow == sta->cparams.slow)) + return; + + net_info_ratelimited("%pM: updating CoDel params - throughput %d slow %s\n", + sta->addr, thr, slow ? "true" : "false"); + + if (slow) { + sta->cparams.cparams.target = MS2TIME(50); + sta->cparams.cparams.interval = MS2TIME(300); + sta->cparams.cparams.ecn = false; + } else { + sta->cparams.cparams.target = MS2TIME(5); + sta->cparams.cparams.interval = MS2TIME(100); + sta->cparams.cparams.ecn = true; + } + sta->cparams.update_time = now; + sta->cparams.slow = slow; +} diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h index 0556be3..928c52d 100644 --- a/net/mac80211/sta_info.h +++ b/net/mac80211/sta_info.h @@ -384,6 +384,16 @@ struct ieee80211_sta_rx_stats { u64 msdu[IEEE80211_NUM_TIDS + 1]; }; +#define STA_CPARAMS_HYSTERESIS 5 * NSEC_PER_SEC +#define STA_SLOW_THRESHOLD 8000 /* 8 mbps */ + +struct sta_codel_params { + struct codel_params cparams; + u64 update_time; + bool slow; +}; +void sta_update_codel_params(struct sta_info *sta); + /** * struct sta_info - STA information * @@ -437,6 +447,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 +551,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; diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index efc38e7..3973ab0 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -1339,12 +1339,19 @@ static struct sk_buff *fq_tin_dequeue_func(struct fq *fq, struct codel_vars *cvars; struct codel_params *cparams; struct codel_stats *cstats; + struct sta_info *sta; 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) { + sta = container_of(txqi->txq.sta, struct sta_info, sta); + cparams = &sta->cparams.cparams; + } else + cparams = &local->cparams; + + if (flow == &txqi->def_flow) cvars = &txqi->def_cvars; else @@ -1547,6 +1554,11 @@ static bool ieee80211_tx_frags(struct ieee80211_local *local, txqi = ieee80211_get_txq(local, vif, sta, skb); if (txqi) { + if (sta) { + struct sta_info *sti = container_of(sta, struct sta_info, sta); + sta_update_codel_params(sti); + } + info->control.vif = vif; __skb_unlink(skb, skbs);
CoDel can be too aggressive if a station sends at a very low rate. This gets worse the more stations are present, as each station gets more bursty the longer the round-robin scheduling between stations takes. This is an attempt to dynamically adjust CoDel parameters per station. It takes a rather simple approach and uses a simple binary designation of a station as 'slow' if it has expected throughput less than eight Mbps (i.e. the lowest couple of rates). In this case, CoDel is set to be more lenient by adjusting its target to 50 ms and interval to 300 ms. There's a built-in hysteresis so a station cannot flip between slow and fast more than once every two seconds. In this version the check is performed every time a packet is enqueued to the intermediate queues; and the overhead of doing this is alleviated a bit by caching the result and by the above-mentioned hysteresis. It can probably be smarter about both when and how to do the scaling, but this seems to alleviate some of the starvation I've seen with very slow stations. I'm not sure how this interacts with drivers that do rate-control in firmware (such as ath10k). I assume they will just not be able to return an expected rate (and so will be considered 'fast'). Cc: Michal Kazior <michal.kazior@tieto.com> Cc: Felix Fietkau <nbd@nbd.name> Cc: Dave Taht <dave@taht.net> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> --- net/mac80211/sta_info.c | 37 +++++++++++++++++++++++++++++++++++++ net/mac80211/sta_info.h | 13 +++++++++++++ net/mac80211/tx.c | 14 +++++++++++++- 3 files changed, 63 insertions(+), 1 deletion(-)