[PATCH] mac80211: Dynamically set CoDel parameters per station.

[PATCH] mac80211: Dynamically set CoDel parameters per station.

[Toke Høiland-Jørgensen]

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=C3=B8iland-J=C3=B8rgensen <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(-)

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);
=20
 /**
+ * ieee80211_sta_set_expected_throughput - set the expected throughput f=
or a
+ * station
+ *
+ * Call this function to notify mac80211 about a change in expected thro=
ughput
+ * to a station. A driver for a device that does rate control in firmwar=
e can
+ * call this function when the expected throughput estimate towards a st=
ation
+ * changes. The information is used to tune the CoDel AQM applied to tra=
ffic
+ * 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,
=20
 	drv_sta_rate_tbl_update(hw_to_local(hw), sta->sdata, pubsta);
=20
+	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>
=20
+#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,
=20
 	sta->sta.max_rc_amsdu_len =3D IEEE80211_MAX_MPDU_LEN_HT_BA;
=20
+	sta_update_codel_params(sta, 0);
+
 	sta_dbg(sdata, "Allocated STA %pM\n", sta->sta.addr);
=20
 	return sta;
@@ -2306,6 +2309,15 @@ u32 sta_get_expected_throughput(struct sta_info *s=
ta)
 	return thr;
 }
=20
+void ieee80211_sta_set_expected_throughput(struct ieee80211_sta *pubsta,
+					   u32 thr)
+{
+	struct sta_info *sta =3D 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 =3D 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 =3D 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 =3D MS2TIME(50);
+		sta->cparams.params.interval =3D MS2TIME(300);
+		sta->cparams.params.ecn =3D false;
+	} else {
+		sta->cparams.params.target =3D MS2TIME(20);
+		sta->cparams.params.interval =3D MS2TIME(100);
+		sta->cparams.params.ecn =3D true;
+	}
+	sta->cparams.params.ce_threshold =3D CODEL_DISABLED_THRESHOLD;
+	sta->cparams.update_time =3D 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];
 };
=20
+#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_UNRESERV=
ED)
  * @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;
=20
+	struct sta_codel_params cparams;
+
 	u8 reserved_tid;
=20
 	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);
=20
 u32 sta_get_expected_throughput(struct sta_info *sta);
+void sta_update_codel_params(struct sta_info *sta, u32 thr);
=20
 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,
=20
 	local =3D container_of(fq, struct ieee80211_local, fq);
 	txqi =3D container_of(tin, struct txq_info, tin);
-	cparams =3D &local->cparams;
 	cstats =3D &local->cstats;
=20
+	if (txqi->txq.sta) {
+		struct sta_info *sta =3D container_of(txqi->txq.sta,
+						    struct sta_info, sta);
+		cparams =3D &sta->cparams.params;
+	} else {
+		cparams =3D &local->cparams;
+	}
+
 	if (flow =3D=3D &txqi->def_flow)
 		cvars =3D &txqi->def_cvars;
 	else
--=20
2.9.3

base-commit: abc3750c31320f36e230f88b235a90e0a35f7032

Re: [PATCH] mac80211: Dynamically set CoDel parameters per station.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3725 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6422 bytes --]

Re: [Make-wifi-fast] [PATCH] mac80211: Dynamically set CoDel parameters per station.

[Toke Høiland-Jørgensen]

Jim Gettys <jg@freedesktop.org> writes:

>> On Sat, Sep 10, 2016 at 3:33 PM, Toke H=C3=B8iland-J=C3=B8rgensen <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.
>
> =E2=80=8BWhere is this 2 second constant coming from? I'd expect it shoul=
d 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

Re: [Make-wifi-fast] [PATCH] mac80211: Dynamically set CoDel parameters per station.

[Loganaden Velvindron]

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=C3=B8iland-J=C3=B8rgensen <toke@t=
oke.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.

[PATCH v2] mac80211: Dynamically set CoDel parameters per station

[Toke Høiland-Jørgensen]

CoDel can be too aggressive if a station sends at a very low rate,
leading to reduced throughput. 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 (modified by the number of active stations).

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.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
Changes since v1:
 - Get rid of the hysteresis (in practice we don't go above and below
   the threshold too often).
 - Lower threshold, but scaled with the number of active (backlogged)
   stations.
 
 include/net/mac80211.h     | 17 +++++++++++++++++
 net/mac80211/debugfs_sta.c |  6 ++++++
 net/mac80211/rate.c        |  3 ++-
 net/mac80211/sta_info.c    | 31 +++++++++++++++++++++++++++++++
 net/mac80211/sta_info.h    | 12 ++++++++++++
 net/mac80211/tx.c          |  9 ++++++++-
 6 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index a3bab3c5ecfb..84ca624aa238 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -4181,6 +4181,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/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index 42601820db20..b15412c21ac9 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -154,6 +154,12 @@ static ssize_t sta_aqm_read(struct file *file, char __user *userbuf,
 
 	p += scnprintf(p,
 		       bufsz+buf-p,
+		       "target %uus interval %uus ecn %s\n",
+		       codel_time_to_us(sta->cparams.target),
+		       codel_time_to_us(sta->cparams.interval),
+		       sta->cparams.ecn ? "yes" : "no");
+	p += scnprintf(p,
+		       bufsz+buf-p,
 		       "tid ac backlog-bytes backlog-packets new-flows drops marks overlimit collisions tx-bytes tx-packets\n");
 
 	for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 206698bc93f4..233c23ba2b98 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);
 
+	ieee80211_sta_set_expected_throughput(pubsta, sta_get_expected_throughput(sta));
+
 	return 0;
 }
 EXPORT_SYMBOL(rate_control_set_rates);
@@ -938,4 +940,3 @@ void rate_control_deinitialize(struct ieee80211_local *local)
 	local->rate_ctrl = NULL;
 	rate_control_free(ref);
 }
-
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 3323a2fb289b..276ab08353b8 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"
@@ -420,6 +421,11 @@ 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->cparams.ce_threshold = CODEL_DISABLED_THRESHOLD;
+	sta->cparams.target = MS2TIME(20);
+	sta->cparams.interval = MS2TIME(100);
+	sta->cparams.ecn = true;
+
 	sta_dbg(sdata, "Allocated STA %pM\n", sta->sta.addr);
 
 	return sta;
@@ -2298,3 +2304,28 @@ unsigned long ieee80211_sta_last_active(struct sta_info *sta)
 		return stats->last_rx;
 	return sta->status_stats.last_ack;
 }
+
+static inline void sta_update_codel_params(struct sta_info *sta, u32 thr)
+{
+	if (!sta->sdata->local->ops->wake_tx_queue)
+		return;
+
+	if (thr && thr < STA_SLOW_THRESHOLD * sta->local->num_sta) {
+		sta->cparams.target = MS2TIME(50);
+		sta->cparams.interval = MS2TIME(300);
+		sta->cparams.ecn = false;
+	} else {
+		sta->cparams.target = MS2TIME(5);
+		sta->cparams.interval = MS2TIME(100);
+		sta->cparams.ecn = true;
+	}
+}
+
+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);
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index e65cda34d2bc..07e6d7c4e6b7 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -390,6 +390,14 @@ struct ieee80211_sta_rx_stats {
 };
 
 /**
+ * The bandwidth threshold below which the per-station CoDel parameters will be
+ * scaled to be more lenient (to prevent starvation of slow stations). This
+ * value will be scaled by the number of active stations when it is being
+ * applied.
+ */
+#define STA_SLOW_THRESHOLD 6000 /* 6 Mbps */
+
+/**
  * struct sta_info - STA information
  *
  * This structure collects information about a station that
@@ -442,6 +450,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
@@ -545,6 +554,8 @@ struct sta_info {
 	enum ieee80211_smps_mode known_smps_mode;
 	const struct ieee80211_cipher_scheme *cipher_scheme;
 
+	struct 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 ba8d7db0a071..6e5292b01f39 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1344,9 +1344,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 = &txqi->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
-- 
2.12.1

Re: [PATCH v2] mac80211: Dynamically set CoDel parameters per station

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2033 bytes --]

Hi Toke,

[auto build test ERROR on mac80211/master]
[also build test ERROR on v4.11-rc5 next-20170406]
[cannot apply to mac80211-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Toke-H-iland-J-rgensen/mac80211-Dynamically-set-CoDel-parameters-per-station/20170406-152423
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git master
config: i386-randconfig-x019-201714 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> net//mac80211/sta_info.c:2308:20: error: static declaration of 'sta_update_codel_params' follows non-static declaration
    static inline void sta_update_codel_params(struct sta_info *sta, u32 thr)
                       ^~~~~~~~~~~~~~~~~~~~~~~
   In file included from net//mac80211/ieee80211_i.h:35:0,
                    from net//mac80211/sta_info.c:25:
   net//mac80211/sta_info.h:727:6: note: previous declaration of 'sta_update_codel_params' was here
    void sta_update_codel_params(struct sta_info *sta, u32 thr);
         ^~~~~~~~~~~~~~~~~~~~~~~
--
   net//mac80211/tx.c: In function 'fq_tin_dequeue_func':
>> net//mac80211/tx.c:1352:26: error: 'struct codel_params' has no member named 'params'
      cparams = &sta->cparams.params;
                             ^

vim +/sta_update_codel_params +2308 net//mac80211/sta_info.c

  2302	
  2303		if (time_after(stats->last_rx, sta->status_stats.last_ack))
  2304			return stats->last_rx;
  2305		return sta->status_stats.last_ack;
  2306	}
  2307	
> 2308	static inline void sta_update_codel_params(struct sta_info *sta, u32 thr)
  2309	{
  2310		if (!sta->sdata->local->ops->wake_tx_queue)
  2311			return;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33136 bytes --]

Re: [PATCH v2] mac80211: Dynamically set CoDel parameters per station

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4003 bytes --]

Hi Toke,

[auto build test WARNING on mac80211/master]
[also build test WARNING on v4.11-rc5 next-20170406]
[cannot apply to mac80211-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Toke-H-iland-J-rgensen/mac80211-Dynamically-set-CoDel-parameters-per-station/20170406-152423
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git master
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

>> include/net/mac80211.h:4198: warning: No description found for parameter 'pubsta'
>> include/net/mac80211.h:4198: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput'
>> include/net/mac80211.h:4198: warning: No description found for parameter 'pubsta'
>> include/net/mac80211.h:4198: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput'
>> include/net/mac80211.h:4198: warning: No description found for parameter 'pubsta'
>> include/net/mac80211.h:4198: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput'
>> include/net/mac80211.h:4198: warning: No description found for parameter 'pubsta'
>> include/net/mac80211.h:4198: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput'
>> include/net/mac80211.h:4198: warning: No description found for parameter 'pubsta'
>> include/net/mac80211.h:4198: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput'
>> include/net/mac80211.h:4198: warning: No description found for parameter 'pubsta'
>> include/net/mac80211.h:4198: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput'
>> include/net/mac80211.h:4198: warning: No description found for parameter 'pubsta'
>> include/net/mac80211.h:4198: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput'
>> include/net/mac80211.h:4198: warning: No description found for parameter 'pubsta'
>> include/net/mac80211.h:4198: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput'
>> include/net/mac80211.h:4198: warning: No description found for parameter 'pubsta'
>> include/net/mac80211.h:4198: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput'
>> include/net/mac80211.h:4198: warning: No description found for parameter 'pubsta'
>> include/net/mac80211.h:4198: warning: Excess function parameter 'sta' description in 'ieee80211_sta_set_expected_throughput'

vim +/pubsta +4198 include/net/mac80211.h

  4182	
  4183	/**
  4184	 * ieee80211_sta_set_expected_throughput - set the expected throughput for a
  4185	 * station
  4186	 *
  4187	 * Call this function to notify mac80211 about a change in expected throughput
  4188	 * to a station. A driver for a device that does rate control in firmware can
  4189	 * call this function when the expected throughput estimate towards a station
  4190	 * changes. The information is used to tune the CoDel AQM applied to traffic
  4191	 * going towards that station (which can otherwise be too aggressive and cause
  4192	 * slow stations to starve).
  4193	 *
  4194	 * @sta: the station to set throughput for.
  4195	 * @thr: the current expected throughput in kbps.
  4196	 */
  4197	void ieee80211_sta_set_expected_throughput(struct ieee80211_sta *pubsta,
> 4198						   u32 thr);
  4199	
  4200	/**
  4201	 * ieee80211_tx_status - transmit status callback
  4202	 *
  4203	 * Call this function for all transmitted frames after they have been
  4204	 * transmitted. It is permissible to not call this function for
  4205	 * multicast frames but this can affect statistics.
  4206	 *

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6576 bytes --]

[PATCH v3] mac80211: Dynamically set CoDel parameters per station

[Toke Høiland-Jørgensen]

CoDel can be too aggressive if a station sends at a very low rate,
leading reduced throughput. 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 (modified by the number of active stations).

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.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
Changes since v2:
 - Messed up the rebase and squash in v2; this one actually compiles...
 
 include/net/mac80211.h     | 17 +++++++++++++++++
 net/mac80211/debugfs_sta.c |  6 ++++++
 net/mac80211/rate.c        |  3 ++-
 net/mac80211/sta_info.c    | 31 +++++++++++++++++++++++++++++++
 net/mac80211/sta_info.h    | 11 +++++++++++
 net/mac80211/tx.c          |  9 ++++++++-
 6 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index a3bab3c5ecfb..8cd546cbcabc 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -4181,6 +4181,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).
+ *
+ * @pubsta: 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/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index 42601820db20..b15412c21ac9 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -154,6 +154,12 @@ static ssize_t sta_aqm_read(struct file *file, char __user *userbuf,
 
 	p += scnprintf(p,
 		       bufsz+buf-p,
+		       "target %uus interval %uus ecn %s\n",
+		       codel_time_to_us(sta->cparams.target),
+		       codel_time_to_us(sta->cparams.interval),
+		       sta->cparams.ecn ? "yes" : "no");
+	p += scnprintf(p,
+		       bufsz+buf-p,
 		       "tid ac backlog-bytes backlog-packets new-flows drops marks overlimit collisions tx-bytes tx-packets\n");
 
 	for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 206698bc93f4..233c23ba2b98 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);
 
+	ieee80211_sta_set_expected_throughput(pubsta, sta_get_expected_throughput(sta));
+
 	return 0;
 }
 EXPORT_SYMBOL(rate_control_set_rates);
@@ -938,4 +940,3 @@ void rate_control_deinitialize(struct ieee80211_local *local)
 	local->rate_ctrl = NULL;
 	rate_control_free(ref);
 }
-
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 3323a2fb289b..d5d54e84620f 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"
@@ -420,6 +421,11 @@ 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->cparams.ce_threshold = CODEL_DISABLED_THRESHOLD;
+	sta->cparams.target = MS2TIME(20);
+	sta->cparams.interval = MS2TIME(100);
+	sta->cparams.ecn = true;
+
 	sta_dbg(sdata, "Allocated STA %pM\n", sta->sta.addr);
 
 	return sta;
@@ -2298,3 +2304,28 @@ unsigned long ieee80211_sta_last_active(struct sta_info *sta)
 		return stats->last_rx;
 	return sta->status_stats.last_ack;
 }
+
+static inline void sta_update_codel_params(struct sta_info *sta, u32 thr)
+{
+	if (!sta->sdata->local->ops->wake_tx_queue)
+		return;
+
+	if (thr && thr < STA_SLOW_THRESHOLD * sta->local->num_sta) {
+		sta->cparams.target = MS2TIME(50);
+		sta->cparams.interval = MS2TIME(300);
+		sta->cparams.ecn = false;
+	} else {
+		sta->cparams.target = MS2TIME(20);
+		sta->cparams.interval = MS2TIME(100);
+		sta->cparams.ecn = true;
+	}
+}
+
+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);
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index e65cda34d2bc..e3b07e019df1 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -390,6 +390,14 @@ struct ieee80211_sta_rx_stats {
 };
 
 /**
+ * The bandwidth threshold below which the per-station CoDel parameters will be
+ * scaled to be more lenient (to prevent starvation of slow stations). This
+ * value will be scaled by the number of active stations when it is being
+ * applied.
+ */
+#define STA_SLOW_THRESHOLD 6000 /* 6 Mbps */
+
+/**
  * struct sta_info - STA information
  *
  * This structure collects information about a station that
@@ -442,6 +450,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
@@ -545,6 +554,8 @@ struct sta_info {
 	enum ieee80211_smps_mode known_smps_mode;
 	const struct ieee80211_cipher_scheme *cipher_scheme;
 
+	struct 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 ba8d7db0a071..dab60a165059 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1344,9 +1344,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 = &txqi->cstats;
 
+	if (txqi->txq.sta) {
+		struct sta_info *sta = container_of(txqi->txq.sta,
+						    struct sta_info, sta);
+		cparams = &sta->cparams;
+	} else {
+		cparams = &local->cparams;
+	}
+
 	if (flow == &txqi->def_flow)
 		cvars = &txqi->def_cvars;
 	else
-- 
2.12.1

Re: [Make-wifi-fast] [PATCH v3] mac80211: Dynamically set CoDel parameters per station

[Eric Dumazet]

On Thu, 2017-04-06 at 11:38 +0200, Toke Høiland-Jørgensen wrote:

> +
> +	if (thr && thr < STA_SLOW_THRESHOLD * sta->local->num_sta) {
> +		sta->cparams.target = MS2TIME(50);
> +		sta->cparams.interval = MS2TIME(300);
> +		sta->cparams.ecn = false;
> +	} else {
> +		sta->cparams.target = MS2TIME(20);
> +		sta->cparams.interval = MS2TIME(100);
> +		sta->cparams.ecn = true;
> +	}
> +}

Why ECN is flipped on/off like that ? ECN really should be an admin
choice.

Also, this change in parameters looks suspect to me, adding a bimodal
behavior. I would consult Kathleen and Van on this possibility.

Re: [Make-wifi-fast] [PATCH v3] mac80211: Dynamically set CoDel parameters per station

[Toke Høiland-Jørgensen]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Thu, 2017-04-06 at 11:38 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>
>> +
>> +	if (thr && thr < STA_SLOW_THRESHOLD * sta->local->num_sta) {
>> +		sta->cparams.target =3D MS2TIME(50);
>> +		sta->cparams.interval =3D MS2TIME(300);
>> +		sta->cparams.ecn =3D false;
>> +	} else {
>> +		sta->cparams.target =3D MS2TIME(20);
>> +		sta->cparams.interval =3D MS2TIME(100);
>> +		sta->cparams.ecn =3D true;
>> +	}
>> +}
>
> Why ECN is flipped on/off like that ?

The reasoning is that at really low bandwidths we're better off dropping
the packet and getting potentially latency-sensitive data queued behind
it through (see Dave's various rants with the topic "Packets have
mass").

> ECN really should be an admin choice.

Well, the trouble is that the mac80211 queues don't really have an admin
interface currently. So it'll always use ECN (before this change).

> Also, this change in parameters looks suspect to me, adding a bimodal
> behavior. I would consult Kathleen and Van on this possibility.

Yeah, I agree that it's somewhat of a hack from a theoretical point of
view. I've also been experimenting with Kathy's recommended way of
dealing with bursty MACs (turning off CoDel when there's less than an
MTU's worth of data left), but have not had a lot of success with it.
Guess I can go back and try some variants on that, unless someone else
has better ideas?

-Toke

Re: [Make-wifi-fast] [PATCH v3] mac80211: Dynamically set CoDel parameters per station

[Dave Taht]

On Thu, Apr 6, 2017 at 8:58 AM, Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke=
.dk> wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
>
>> On Thu, 2017-04-06 at 11:38 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrot=
e:
>>
>>> +
>>> +    if (thr && thr < STA_SLOW_THRESHOLD * sta->local->num_sta) {
>>> +            sta->cparams.target =3D MS2TIME(50);
>>> +            sta->cparams.interval =3D MS2TIME(300);
>>> +            sta->cparams.ecn =3D false;
>>> +    } else {
>>> +            sta->cparams.target =3D MS2TIME(20);
>>> +            sta->cparams.interval =3D MS2TIME(100);
>>> +            sta->cparams.ecn =3D true;
>>> +    }
>>> +}
>>
>> Why ECN is flipped on/off like that ?
>
> The reasoning is that at really low bandwidths we're better off dropping
> the packet and getting potentially latency-sensitive data queued behind
> it through (see Dave's various rants with the topic "Packets have
> mass").

My general take on wifi is that if you are running at - particularly,
stuck at - a low rate (sub 6mbits in the case of this code) - you have
so many other problems like retransmits, interference, etc, in the
first place, that the presence or absence of codel here is just a
small contributor to that noise.

We could leave ecn at whatever it is set to here and not flip it on or
off. It does seem sane to twiddle the parameters enough to make sure
codel doesn't trigger at less than a MTU vs the achieved rate.

>> ECN really should be an admin choice.
>
> Well, the trouble is that the mac80211 queues don't really have an admin
> interface currently. So it'll always use ECN (before this change).

Should we add a sysfs api to this?

>> Also, this change in parameters looks suspect to me, adding a bimodal
>> behavior. I would consult Kathleen and Van on this possibility.

It's sort of trimodal, actually. I think a more effective approach
would be codel's default were the normal 5% of 100ms, bumping it up
(as per the above) when we're having bad connectivity.... and we tried
to tackle excessive retransmits harder, and addressed  the side
impacts of multicast, instead, as much bigger parts of the problem.

> Yeah, I agree that it's somewhat of a hack from a theoretical point of
> view. I've also been experimenting with Kathy's recommended way of
> dealing with bursty MACs (turning off CoDel when there's less than an
> MTU's worth of data left), but have not had a lot of success with it.

I'm not in a position to resume trying myself.

> Guess I can go back and try some variants on that, unless someone else
> has better ideas?

Just as stuck as you are!

>
> -Toke
> _______________________________________________
> Make-wifi-fast mailing list
> Make-wifi-fast@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/make-wifi-fast



--=20
Dave T=C3=A4ht
Let's go make home routers and wifi faster! With better software!
http://blog.cerowrt.org

Re: [PATCH v3] mac80211: Dynamically set CoDel parameters per station

[Johannes Berg]

On Thu, 2017-04-06 at 11:38 +0200, Toke Høiland-Jørgensen wrote:
> CoDel can be too aggressive if a station sends at a very low rate,
> leading reduced throughput. This gets worse the more stations are
> present, as each station gets more bursty the longer the round-robin
> scheduling between stations takes.
> 
[...]

I've applied this now (with some minor fixups) - the whole discussion
didn't really conclude in anything, and we can just try it.

johannes

Re: [PATCH v3] mac80211: Dynamically set CoDel parameters per station

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2017-04-06 at 11:38 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> CoDel can be too aggressive if a station sends at a very low rate,
>> leading reduced throughput. This gets worse the more stations are
>> present, as each station gets more bursty the longer the round-robin
>> scheduling between stations takes.
>>=20
> [...]
>
> I've applied this now (with some minor fixups) - the whole discussion
> didn't really conclude in anything, and we can just try it.

Okidoki. FWIW, the reason I never got any further was that my
experiments with other approaches proved somewhat inconclusive. Partly
because a reorganisation of my testbed has caused the physical
conditions to change which has caused the original problem to be less
severe.

So yeah, some wider testing would be good, and I agree that it is low
risk. I'll revisit this myself at some point after I get my testbed
rebased on a new kernel and figure out why the physical conditions
changed... :)

-Toke