[RFC/T][PATCH 0/3] rc80211-pid: PID controller enhancements

[RFC/T][PATCH 0/3] rc80211-pid: PID controller enhancements

[Stefano Brivio]

This series of patches introduces enhancements to the PID controller and
allows tuning parameters to be set. Same as Mattias' patches applies here:
this is WIP, sending it for comments and testing and to stay in sync with
him.


--
Ciao
Stefano

[RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Stefano Brivio]

This patch introduces a learning algorithm in order for the PID controller
to learn how to map adjustment values to rates. This is better described in
code comments.


Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>

---

Index: wireless-2.6/net/mac80211/rc80211_pid.c
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid.c
+++ wireless-2.6/net/mac80211/rc80211_pid.c
@@ -2,6 +2,7 @@
  * Copyright 2002-2005, Instant802 Networks, Inc.
  * Copyright 2005, Devicescape Software, Inc.
  * Copyright 2007, Mattias Nissler <mattias.nissler@gmx.de>
+ * Copyright 2007, Stefano Brivio <stefano.brivio@polimi.it>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -39,12 +40,18 @@
  * an actual sliding window. Advantage is that we don't need to keep an array of
  * the last N error values and computation is easier.
  *
- * Once we have the adj value, we need to map it to a TX rate to be selected.
- * For now, we depend on the rates to be ordered in a way such that more robust
- * rates (i.e. such that exhibit a lower framed failed percentage) come first.
- * E.g. for the 802.11b/g case, we first have the b rates in ascending order,
- * then the g rates. The adj simply decides the index of the TX rate in the list
- * to switch to (relative to the current TX rate entry).
+ * Once we have the adj value, we map it to a rate by means of a learning
+ * algorithm. This algorithm keeps the state of the percentual failed frames
+ * difference between rates. The behaviour of the lowest available rate is kept
+ * as a reference value, and every time we switch between two rates, we compute
+ * the difference between the failed frames each rate exhibited. By doing so,
+ * we compare behaviours which different rates exhibited in adjacent timeslices,
+ * thus the comparison is minimally affected by external conditions. This
+ * difference gets propagated to the whole set of measurements, so that the
+ * reference is always the same. Periodically, we normalize this set so that
+ * recent events weigh the most. By comparing the adj value with this set, we
+ * avoid pejorative switches to lower rates and allow for switches to higher
+ * rates if they behaved well.
  *
  * Note that for the computations we use a fixed-point representation to avoid
  * floating point arithmetic. Hence, all values are shifted left by
@@ -71,6 +78,12 @@
 /* Derivative PID component coefficient. */
 #define RC_PID_COEFF_D 15
 
+/* Rate behaviour normalization quantity over time. */
+#define RC_PID_NORM_FACTOR 3
+
+/* Push high rates right after loading. */
+#define RC_PID_FAST_START 0
+
 /* Target failed frames rate for the PID controller. NB: This effectively gives
  * maximum failed frames percentage we're willing to accept. If communication is
  * good, the controller will fail to adjust failed frames percentage to the
@@ -121,6 +134,18 @@ struct rc_pid_sta_info {
 /* Algorithm parameters. We keep them on a per-algorithm approach, so they can
  * be tuned individually for each interface.
  */
+struct rc_pid_rateinfo {
+
+	/* The rate index in ieee80211_hw_mode. */
+	int index;
+
+	/* Did we do any measurement on this rate? */
+	bool valid;
+
+	/* Comparison with the lowest rate. */
+	int diff;
+};
+
 struct rc_pid_info {
 
 	/* The failed frames percentage target. */
@@ -130,15 +155,59 @@ struct rc_pid_info {
 	s32 coeff_p;
 	s32 coeff_i;
 	s32 coeff_d;
+
+	/* Rates information. */
+	struct rc_pid_rateinfo *rinfo;
+
+	/* Index of the last used rate. */
+	int oldrate;
 };
 
+/* Shift the adjustment so that we won't switch to a lower rate if it exhibited
+ * a worse failed frames behaviour and we'll choose the highest rate whose
+ * failed frames behaviour is not worse than the one of the original rate
+ * target. While at it, check that the adjustment is within the ranges. Then,
+ * provide the new rate index. */
+static int rate_control_pid_shift_adjust(struct rc_pid_rateinfo *r,
+					 int adj, int cur, int l)
+{
+	int i, j, k, tmp;
+
+	if (cur + adj < 0)
+		return 0;
+	if (cur + adj >= l)
+		return l - 1;
+
+	for (i = 0; i < l; i++)
+		if (r[i].index == cur + adj)
+			break;
+	if (unlikely(!r[i].valid))
+		return cur + adj;
+	for (j = 0; j < l; j++)
+		if (r[j].index == cur)
+			break;
+
+	if (adj < 0) {
+		if (r[i].diff > r[j].diff)
+			return cur;
+	} else {
+		tmp = r[i].diff;
+		for (k = i + 1; k + cur < l; k++)
+			if (r[k].valid && r[k].diff <= tmp) {
+				tmp = r[k].diff;
+				adj = k;
+			}
+	}
+	return cur + adj;
+}
 
 static void rate_control_pid_adjust_rate(struct ieee80211_local *local,
-					 struct sta_info *sta, int adj)
+					 struct sta_info *sta, int adj,
+					 struct rc_pid_rateinfo *rinfo)
 {
 	struct ieee80211_sub_if_data *sdata;
 	struct ieee80211_hw_mode *mode;
-	int newidx = sta->txrate + adj;
+	int newidx;
 	int maxrate;
 	int back = (adj > 0) ? 1 : -1;
 
@@ -151,10 +220,8 @@ static void rate_control_pid_adjust_rate
 	mode = local->oper_hw_mode;
 	maxrate = sdata->bss ? sdata->bss->max_ratectrl_rateidx : -1;
 
-	if (newidx < 0)
-		newidx = 0;
-	else if (newidx >= mode->num_rates)
-		newidx = mode->num_rates - 1;
+	newidx = rate_control_pid_shift_adjust(rinfo, adj, sta->txrate,
+					       mode->num_rates);
 
 	while (newidx != sta->txrate) {
 		if (rate_supported(sta, mode, newidx) &&
@@ -167,18 +234,39 @@ static void rate_control_pid_adjust_rate
 	}
 }
 
+/* Normalize the failed frames per-rate differences. */
+static void rate_control_pid_normalize(struct rc_pid_rateinfo *r, int l)
+{
+	int i;
+
+	if (r[0].diff > RC_PID_NORM_FACTOR)
+		r[0].diff -= RC_PID_NORM_FACTOR;
+	else if (r[0].diff < -RC_PID_NORM_FACTOR)
+		r[0].diff += RC_PID_NORM_FACTOR;
+	for (i = 0; i < l - 1; i++)
+		if (likely(r[i + 1].valid)) {
+			if (r[i + 1].diff > r[i].diff + RC_PID_NORM_FACTOR)
+				r[i + 1].diff -= RC_PID_NORM_FACTOR;
+			else if (r[i + 1].diff <= r[i].diff)
+				r[i + 1].diff += RC_PID_NORM_FACTOR;
+		}
+}
+
 static void rate_control_pid_sample(struct rc_pid_info *pinfo,
 				    struct ieee80211_local *local,
 				    struct sta_info *sta)
 {
 	struct rc_pid_sta_info *spinfo = sta->rate_ctrl_priv;
+	struct rc_pid_rateinfo *rinfo = pinfo->rinfo;
+	struct ieee80211_hw_mode *mode;
 	u32 pf;
 	s32 err_avg;
 	s32 err_prop;
 	s32 err_int;
 	s32 err_der;
-	int adj;
+	int adj, i, j, tmp;
 
+	mode = local->oper_hw_mode;
 	spinfo = sta->rate_ctrl_priv;
 	spinfo->last_sample = jiffies;
 
@@ -194,6 +282,31 @@ static void rate_control_pid_sample(stru
 		spinfo->tx_num_failed = 0;
 	}
 
+	/* If we just switched rate, update the rate behaviour info. */
+	if (pinfo->oldrate != sta->txrate) {
+		for (i = 0; i < mode->num_rates; i++)
+			if (rinfo[i].index == pinfo->oldrate)
+				break;
+		for (j = 0; j < mode->num_rates; j++)
+			if (rinfo[j].index == sta->txrate) {
+				rinfo[j].valid = 1;
+				break;
+			}
+
+		tmp = (pf - spinfo->last_pf);
+
+		/* We need to do an arithmetic right shift. ISO C says this is
+		 * implementation defined for negative left operands. Hence, be
+		 * careful to get it right, also for negative values. */
+		tmp = (tmp < 0) ? -((-tmp) >> (RC_PID_ARITH_SHIFT)) :
+			  tmp >> (RC_PID_ARITH_SHIFT);
+
+		rinfo[j].diff = rinfo[i].diff + tmp;
+		rinfo[j].valid = 1;
+		pinfo->oldrate = sta->txrate;
+	}
+	rate_control_pid_normalize(rinfo, mode->num_rates);
+
 	/* Compute the proportional, integral and derivative errors. */
 	err_prop = RC_PID_TARGET_PF - pf;
 
@@ -208,15 +321,13 @@ static void rate_control_pid_sample(stru
 	adj = (err_prop * pinfo->coeff_p + err_int * pinfo->coeff_i
 	      + err_der * pinfo->coeff_d);
 
-	/* We need to do an arithmetic right shift. ISO C says this is
-	 * implementation defined for negative left operands. Hence, be
-	 * careful to get it right, also for negative values. */
+	/* Arithmetic right shift, see above. */
 	adj = (adj < 0) ? -((-adj) >> (2 * RC_PID_ARITH_SHIFT)) :
 			  adj >> (2 * RC_PID_ARITH_SHIFT);
 
 	/* Change rate. */
 	if (adj)
-		rate_control_pid_adjust_rate(local, sta, adj);
+		rate_control_pid_adjust_rate(local, sta, adj, rinfo);
 }
 
 static void rate_control_pid_tx_status(void *priv, struct net_device *dev,
@@ -315,13 +426,55 @@ static void rate_control_pid_rate_init(v
 static void *rate_control_pid_alloc(struct ieee80211_local *local)
 {
 	struct rc_pid_info *pinfo;
+	struct rc_pid_rateinfo *rinfo;
+	struct ieee80211_hw_mode *mode;
+	int i, j, tmp;
+	bool s;
 
 	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
+	if (!pinfo)
+		return NULL;
+
+	mode = local->oper_hw_mode;
+	rinfo = kmalloc(sizeof(*rinfo) * mode->num_rates, GFP_ATOMIC);
+	if (!rinfo) {
+		kfree(pinfo);
+		return NULL;
+	}
+
+	/* Sort the rates. This is optimized for the most common case (i.e.
+	 * almost-sorted CCK+OFDM rates). */
+	for (i = 0; i < mode->num_rates; i++) {
+		rinfo[i].index = i;
+		if (RC_PID_FAST_START) {
+			rinfo[i].valid = 1;
+			rinfo[i].diff = 0;
+		} else
+			rinfo[i].valid = 0;
+	}
+	for (i = 1; i < mode->num_rates; i++) {
+		s = 0;
+		for (j = 0; j < mode->num_rates - i; j++)
+			if (unlikely(mode->rates[rinfo[j].index].rate >
+				     mode->rates[rinfo[j + 1].index].rate)) {
+				tmp = rinfo[j].index;
+				rinfo[j].index = rinfo[j + 1].index;
+				rinfo[j + 1].index = tmp;
+				s = 1;
+			}
+		if (!s)
+			break;
+	}
+
+	rinfo[0].diff = 0;
+	rinfo[0].valid = 1;
 
 	pinfo->target = RC_PID_TARGET_PF;
 	pinfo->coeff_p = RC_PID_COEFF_P;
 	pinfo->coeff_i = RC_PID_COEFF_I;
 	pinfo->coeff_d = RC_PID_COEFF_D;
+	pinfo->rinfo = rinfo;
+	pinfo->oldrate = 0;
 
 	return pinfo;
 }
@@ -330,6 +483,7 @@ static void *rate_control_pid_alloc(stru
 static void rate_control_pid_free(void *priv)
 {
 	struct rc_pid_info *pinfo = priv;
+	kfree(pinfo->rinfo);
 	kfree(pinfo);
 }
 


--
Ciao
Stefano

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Mattias Nissler]

Hi Stefano,

haven't tried it yet, but reviewed your code. Comments inline.

On Sun, 2007-12-09 at 21:19 +0100, Stefano Brivio wrote: 
> This patch introduces a learning algorithm in order for the PID controller
> to learn how to map adjustment values to rates. This is better described in
> code comments.
> 
> 
> Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
> 
> ---
> 
> Index: wireless-2.6/net/mac80211/rc80211_pid.c
> ===================================================================
> --- wireless-2.6.orig/net/mac80211/rc80211_pid.c
> +++ wireless-2.6/net/mac80211/rc80211_pid.c
> @@ -2,6 +2,7 @@
>   * Copyright 2002-2005, Instant802 Networks, Inc.
>   * Copyright 2005, Devicescape Software, Inc.
>   * Copyright 2007, Mattias Nissler <mattias.nissler@gmx.de>
> + * Copyright 2007, Stefano Brivio <stefano.brivio@polimi.it>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -39,12 +40,18 @@
>   * an actual sliding window. Advantage is that we don't need to keep an array of
>   * the last N error values and computation is easier.
>   *
> - * Once we have the adj value, we need to map it to a TX rate to be selected.
> - * For now, we depend on the rates to be ordered in a way such that more robust
> - * rates (i.e. such that exhibit a lower framed failed percentage) come first.
> - * E.g. for the 802.11b/g case, we first have the b rates in ascending order,
> - * then the g rates. The adj simply decides the index of the TX rate in the list
> - * to switch to (relative to the current TX rate entry).
> + * Once we have the adj value, we map it to a rate by means of a learning
> + * algorithm. This algorithm keeps the state of the percentual failed frames
> + * difference between rates. The behaviour of the lowest available rate is kept
> + * as a reference value, and every time we switch between two rates, we compute
> + * the difference between the failed frames each rate exhibited. By doing so,
> + * we compare behaviours which different rates exhibited in adjacent timeslices,
> + * thus the comparison is minimally affected by external conditions. This
> + * difference gets propagated to the whole set of measurements, so that the
> + * reference is always the same. Periodically, we normalize this set so that
> + * recent events weigh the most. By comparing the adj value with this set, we
> + * avoid pejorative switches to lower rates and allow for switches to higher
> + * rates if they behaved well.
>   *
>   * Note that for the computations we use a fixed-point representation to avoid
>   * floating point arithmetic. Hence, all values are shifted left by
> @@ -71,6 +78,12 @@
>  /* Derivative PID component coefficient. */
>  #define RC_PID_COEFF_D 15
>  
> +/* Rate behaviour normalization quantity over time. */
> +#define RC_PID_NORM_FACTOR 3
> +
> +/* Push high rates right after loading. */
> +#define RC_PID_FAST_START 0
> +
>  /* Target failed frames rate for the PID controller. NB: This effectively gives
>   * maximum failed frames percentage we're willing to accept. If communication is
>   * good, the controller will fail to adjust failed frames percentage to the
> @@ -121,6 +134,18 @@ struct rc_pid_sta_info {
>  /* Algorithm parameters. We keep them on a per-algorithm approach, so they can
>   * be tuned individually for each interface.
>   */
> +struct rc_pid_rateinfo {
> +
> +	/* The rate index in ieee80211_hw_mode. */
> +	int index;
> +
> +	/* Did we do any measurement on this rate? */
> +	bool valid;
> +
> +	/* Comparison with the lowest rate. */
> +	int diff;
> +};
> +
>  struct rc_pid_info {
>  
>  	/* The failed frames percentage target. */
> @@ -130,15 +155,59 @@ struct rc_pid_info {
>  	s32 coeff_p;
>  	s32 coeff_i;
>  	s32 coeff_d;
> +
> +	/* Rates information. */
> +	struct rc_pid_rateinfo *rinfo;
> +
> +	/* Index of the last used rate. */
> +	int oldrate;
>  };
>  
> +/* Shift the adjustment so that we won't switch to a lower rate if it exhibited
> + * a worse failed frames behaviour and we'll choose the highest rate whose
> + * failed frames behaviour is not worse than the one of the original rate
> + * target. While at it, check that the adjustment is within the ranges. Then,
> + * provide the new rate index. */
> +static int rate_control_pid_shift_adjust(struct rc_pid_rateinfo *r,
> +					 int adj, int cur, int l)
> +{
> +	int i, j, k, tmp;
> +
> +	if (cur + adj < 0)
> +		return 0;
> +	if (cur + adj >= l)
> +		return l - 1;
> +
> +	for (i = 0; i < l; i++)
> +		if (r[i].index == cur + adj)
> +			break;
> +	if (unlikely(!r[i].valid))
> +		return cur + adj;
> +	for (j = 0; j < l; j++)
> +		if (r[j].index == cur)
> +			break;
> +
> +	if (adj < 0) {
> +		if (r[i].diff > r[j].diff)
> +			return cur;

This prevents switching down if the rate we're switching to is worse
than the current. But if |adj| > 1, we could still find an acceptable
rate in between i and j, couldn't we?

> +	} else {
> +		tmp = r[i].diff;
> +		for (k = i + 1; k + cur < l; k++)
> +			if (r[k].valid && r[k].diff <= tmp) {
> +				tmp = r[k].diff;
> +				adj = k;
> +			}

This increases the rate even more if following rates are better than the
one actually selected by the PID controller. I'm not too happy with
this, because it defeats the idea that the PID controller can decide how
far it wants to switch by the absolute value of adj.

> +	}
> +	return cur + adj;
> +}
> 
>  static void rate_control_pid_adjust_rate(struct ieee80211_local *local,
> -					 struct sta_info *sta, int adj)
> +					 struct sta_info *sta, int adj,
> +					 struct rc_pid_rateinfo *rinfo)
>  {
>  	struct ieee80211_sub_if_data *sdata;
>  	struct ieee80211_hw_mode *mode;
> -	int newidx = sta->txrate + adj;
> +	int newidx;
>  	int maxrate;
>  	int back = (adj > 0) ? 1 : -1;
>  
> @@ -151,10 +220,8 @@ static void rate_control_pid_adjust_rate
>  	mode = local->oper_hw_mode;
>  	maxrate = sdata->bss ? sdata->bss->max_ratectrl_rateidx : -1;
>  
> -	if (newidx < 0)
> -		newidx = 0;
> -	else if (newidx >= mode->num_rates)
> -		newidx = mode->num_rates - 1;
> +	newidx = rate_control_pid_shift_adjust(rinfo, adj, sta->txrate,
> +					       mode->num_rates);
>  
>  	while (newidx != sta->txrate) {
>  		if (rate_supported(sta, mode, newidx) &&
> @@ -167,18 +234,39 @@ static void rate_control_pid_adjust_rate
>  	}
>  }
>  
> +/* Normalize the failed frames per-rate differences. */
> +static void rate_control_pid_normalize(struct rc_pid_rateinfo *r, int l)
> +{
> +	int i;
> +
> +	if (r[0].diff > RC_PID_NORM_FACTOR)
> +		r[0].diff -= RC_PID_NORM_FACTOR;
> +	else if (r[0].diff < -RC_PID_NORM_FACTOR)
> +		r[0].diff += RC_PID_NORM_FACTOR;
> +	for (i = 0; i < l - 1; i++)
> +		if (likely(r[i + 1].valid)) {
> +			if (r[i + 1].diff > r[i].diff + RC_PID_NORM_FACTOR)
> +				r[i + 1].diff -= RC_PID_NORM_FACTOR;
> +			else if (r[i + 1].diff <= r[i].diff)
> +				r[i + 1].diff += RC_PID_NORM_FACTOR;
> +		}

If I understand correctly, you apply an offset to all valid entries. So
I'd rather call it RC_PID_NORM_OFFSET. Furthermore, what if the diff
values are larger than k * RC_PID_NORM_OFFSET (with k an integer)? If
you first compute the offset, you can just loop over the array and apply
the offset without checking for the condition.

> +}
> +
>  static void rate_control_pid_sample(struct rc_pid_info *pinfo,
>  				    struct ieee80211_local *local,
>  				    struct sta_info *sta)
>  {
>  	struct rc_pid_sta_info *spinfo = sta->rate_ctrl_priv;
> +	struct rc_pid_rateinfo *rinfo = pinfo->rinfo;
> +	struct ieee80211_hw_mode *mode;
>  	u32 pf;
>  	s32 err_avg;
>  	s32 err_prop;
>  	s32 err_int;
>  	s32 err_der;
> -	int adj;
> +	int adj, i, j, tmp;
>  
> +	mode = local->oper_hw_mode;
>  	spinfo = sta->rate_ctrl_priv;
>  	spinfo->last_sample = jiffies;
>  
> @@ -194,6 +282,31 @@ static void rate_control_pid_sample(stru
>  		spinfo->tx_num_failed = 0;
>  	}
>  
> +	/* If we just switched rate, update the rate behaviour info. */
> +	if (pinfo->oldrate != sta->txrate) {
> +		for (i = 0; i < mode->num_rates; i++)
> +			if (rinfo[i].index == pinfo->oldrate)
> +				break;
> +		for (j = 0; j < mode->num_rates; j++)
> +			if (rinfo[j].index == sta->txrate) {
> +				rinfo[j].valid = 1;
> +				break;
> +			}

This is no 3 and 4 of the call sites to the to be written
find_index_for_rate() function ;-)

Or what about this idea: Store direct indices into the rinfo table, so
you needn't convert them back from the mode->rates indexing everywhere.

> +
> +		tmp = (pf - spinfo->last_pf);
> +
> +		/* We need to do an arithmetic right shift. ISO C says this is
> +		 * implementation defined for negative left operands. Hence, be
> +		 * careful to get it right, also for negative values. */
> +		tmp = (tmp < 0) ? -((-tmp) >> (RC_PID_ARITH_SHIFT)) :
> +			  tmp >> (RC_PID_ARITH_SHIFT);
> +
> +		rinfo[j].diff = rinfo[i].diff + tmp;
> +		rinfo[j].valid = 1;
> +		pinfo->oldrate = sta->txrate;
> +	}
> +	rate_control_pid_normalize(rinfo, mode->num_rates);
> +
>  	/* Compute the proportional, integral and derivative errors. */
>  	err_prop = RC_PID_TARGET_PF - pf;
>  
> @@ -208,15 +321,13 @@ static void rate_control_pid_sample(stru
>  	adj = (err_prop * pinfo->coeff_p + err_int * pinfo->coeff_i
>  	      + err_der * pinfo->coeff_d);
>  
> -	/* We need to do an arithmetic right shift. ISO C says this is
> -	 * implementation defined for negative left operands. Hence, be
> -	 * careful to get it right, also for negative values. */
> +	/* Arithmetic right shift, see above. */
>  	adj = (adj < 0) ? -((-adj) >> (2 * RC_PID_ARITH_SHIFT)) :
>  			  adj >> (2 * RC_PID_ARITH_SHIFT);

If we use it twice, IMHO it's worth factoring it into a macro.

> 
>  	/* Change rate. */
>  	if (adj)
> -		rate_control_pid_adjust_rate(local, sta, adj);
> +		rate_control_pid_adjust_rate(local, sta, adj, rinfo);
>  }
>  
>  static void rate_control_pid_tx_status(void *priv, struct net_device *dev,
> @@ -315,13 +426,55 @@ static void rate_control_pid_rate_init(v
>  static void *rate_control_pid_alloc(struct ieee80211_local *local)
>  {
>  	struct rc_pid_info *pinfo;
> +	struct rc_pid_rateinfo *rinfo;
> +	struct ieee80211_hw_mode *mode;
> +	int i, j, tmp;
> +	bool s;
>  
>  	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
> +	if (!pinfo)
> +		return NULL;
> +
> +	mode = local->oper_hw_mode;
> +	rinfo = kmalloc(sizeof(*rinfo) * mode->num_rates, GFP_ATOMIC);
> +	if (!rinfo) {
> +		kfree(pinfo);
> +		return NULL;
> +	}

What if the mode is changed? Have you checked the rate control algorithm
gets reinitialized? If not, we're scheduling a crash here, when
mode->num_rates changes.

> +
> +	/* Sort the rates. This is optimized for the most common case (i.e.
> +	 * almost-sorted CCK+OFDM rates). */
> +	for (i = 0; i < mode->num_rates; i++) {
> +		rinfo[i].index = i;
> +		if (RC_PID_FAST_START) {
> +			rinfo[i].valid = 1;
> +			rinfo[i].diff = 0;
> +		} else
> +			rinfo[i].valid = 0;

Maybe use an #ifdef RC_PID_FAST_START_HERE?

> +	}
> +	for (i = 1; i < mode->num_rates; i++) {
> +		s = 0;
> +		for (j = 0; j < mode->num_rates - i; j++)
> +			if (unlikely(mode->rates[rinfo[j].index].rate >
> +				     mode->rates[rinfo[j + 1].index].rate)) {
> +				tmp = rinfo[j].index;
> +				rinfo[j].index = rinfo[j + 1].index;
> +				rinfo[j + 1].index = tmp;
> +				s = 1;
> +			}
> +		if (!s)
> +			break;
> +	}

There is include/linux/sort.h. I guess using this would make the code
clearer. It took me a moment to recognize the code as bubble sort.

What just came to my mind: Maybe it's not a good idea to actually break
up the 80211.b then 80211.g sorting of the rates: If you have an AP,
it'll compute separate tx rates for all stations it knows. But there
might also be 80211.b only stations. If we have 6Mbit/s and 9Mbit/s OFDM
rates before the 11Mbit/s OFDM rate, will we ever be able to switch to
11Mbit/s?

> +
> +	rinfo[0].diff = 0;
> +	rinfo[0].valid = 1;
>  
>  	pinfo->target = RC_PID_TARGET_PF;
>  	pinfo->coeff_p = RC_PID_COEFF_P;
>  	pinfo->coeff_i = RC_PID_COEFF_I;
>  	pinfo->coeff_d = RC_PID_COEFF_D;
> +	pinfo->rinfo = rinfo;
> +	pinfo->oldrate = 0;
>  
>  	return pinfo;
>  }
> @@ -330,6 +483,7 @@ static void *rate_control_pid_alloc(stru
>  static void rate_control_pid_free(void *priv)
>  {
>  	struct rc_pid_info *pinfo = priv;
> +	kfree(pinfo->rinfo);
>  	kfree(pinfo);
>  }
>  

Overall, I'm still not too convinced whether this learning approach is
worth the effort, but this is something to be figured out by testing.

Mattias

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Stefano Brivio]

On Sun, 09 Dec 2007 23:25:50 +0100
Mattias Nissler <mattias.nissler@gmx.de> wrote:

> > +static int rate_control_pid_shift_adjust(struct rc_pid_rateinfo *r,
> > +					 int adj, int cur, int l)
> > +{
> > +	int i, j, k, tmp;
> > +
> > +	if (cur + adj < 0)
> > +		return 0;
> > +	if (cur + adj >= l)
> > +		return l - 1;
> > +
> > +	for (i = 0; i < l; i++)
> > +		if (r[i].index == cur + adj)
> > +			break;
> > +	if (unlikely(!r[i].valid))
> > +		return cur + adj;
> > +	for (j = 0; j < l; j++)
> > +		if (r[j].index == cur)
> > +			break;
> > +
> > +	if (adj < 0) {
> > +		if (r[i].diff > r[j].diff)
> > +			return cur;
> 
> This prevents switching down if the rate we're switching to is worse
> than the current. But if |adj| > 1, we could still find an acceptable
> rate in between i and j, couldn't we?

Right. I even thought about this, but then forgot to implement it. I'll fix
it.
 
> > +	} else {
> > +		tmp = r[i].diff;
> > +		for (k = i + 1; k + cur < l; k++)
> > +			if (r[k].valid && r[k].diff <= tmp) {
> > +				tmp = r[k].diff;
> > +				adj = k;
> > +			}
> 
> This increases the rate even more if following rates are better than the
> one actually selected by the PID controller.

This is intentional (see fast_start, e.g.).

> I'm not too happy with
> this, because it defeats the idea that the PID controller can decide how
> far it wants to switch by the absolute value of adj.

Please remember that normalization is periodically performed. So, this will
happen only if we noticed that in very recent events that it doesn't make
sense not to switch to an higher rate. We can tune this behaviour with the
norm_factor parameter. I tested this briefly and turned out to yield some
performance advantage in most cases (norm_factor set to 3 seems to be
optimal).

> > +/* Normalize the failed frames per-rate differences. */
> > +static void rate_control_pid_normalize(struct rc_pid_rateinfo *r, int l)
> > +{
> > +	int i;
> > +
> > +	if (r[0].diff > RC_PID_NORM_FACTOR)
> > +		r[0].diff -= RC_PID_NORM_FACTOR;
> > +	else if (r[0].diff < -RC_PID_NORM_FACTOR)
> > +		r[0].diff += RC_PID_NORM_FACTOR;
> > +	for (i = 0; i < l - 1; i++)
> > +		if (likely(r[i + 1].valid)) {
> > +			if (r[i + 1].diff > r[i].diff + RC_PID_NORM_FACTOR)
> > +				r[i + 1].diff -= RC_PID_NORM_FACTOR;
> > +			else if (r[i + 1].diff <= r[i].diff)
> > +				r[i + 1].diff += RC_PID_NORM_FACTOR;
> > +		}
> 
> If I understand correctly, you apply an offset to all valid entries. So
> I'd rather call it RC_PID_NORM_OFFSET.

Well, I don't care that much. I was referring to this definition of factor:

n., 1. One that actively contributes to an accomplishment, result, or process

Anyway "offset" may be more clear. Will fix it.

> Furthermore, what if the diff
> values are larger than k * RC_PID_NORM_OFFSET (with k an integer)? If
> you first compute the offset, you can just loop over the array and apply
> the offset without checking for the condition.

I frankly don't get your point here. No offsets are computed...

> > +}
> > +
> >  static void rate_control_pid_sample(struct rc_pid_info *pinfo,
> >  				    struct ieee80211_local *local,
> >  				    struct sta_info *sta)
> >  {
> >  	struct rc_pid_sta_info *spinfo = sta->rate_ctrl_priv;
> > +	struct rc_pid_rateinfo *rinfo = pinfo->rinfo;
> > +	struct ieee80211_hw_mode *mode;
> >  	u32 pf;
> >  	s32 err_avg;
> >  	s32 err_prop;
> >  	s32 err_int;
> >  	s32 err_der;
> > -	int adj;
> > +	int adj, i, j, tmp;
> >  
> > +	mode = local->oper_hw_mode;
> >  	spinfo = sta->rate_ctrl_priv;
> >  	spinfo->last_sample = jiffies;
> >  
> > @@ -194,6 +282,31 @@ static void rate_control_pid_sample(stru
> >  		spinfo->tx_num_failed = 0;
> >  	}
> >  
> > +	/* If we just switched rate, update the rate behaviour info. */
> > +	if (pinfo->oldrate != sta->txrate) {
> > +		for (i = 0; i < mode->num_rates; i++)
> > +			if (rinfo[i].index == pinfo->oldrate)
> > +				break;
> > +		for (j = 0; j < mode->num_rates; j++)
> > +			if (rinfo[j].index == sta->txrate) {
> > +				rinfo[j].valid = 1;
> > +				break;
> > +			}
> 
> This is no 3 and 4 of the call sites to the to be written
> find_index_for_rate() function ;-)
>
> Or what about this idea: Store direct indices into the rinfo table, so
> you needn't convert them back from the mode->rates indexing everywhere.

This makes sense. Unless you are working to the find_index_for_rate()
function, which would make more sense.

> > -	/* We need to do an arithmetic right shift. ISO C says this is
> > -	 * implementation defined for negative left operands. Hence, be
> > -	 * careful to get it right, also for negative values. */
> > +	/* Arithmetic right shift, see above. */
> >  	adj = (adj < 0) ? -((-adj) >> (2 * RC_PID_ARITH_SHIFT)) :
> >  			  adj >> (2 * RC_PID_ARITH_SHIFT);
> 
> If we use it twice, IMHO it's worth factoring it into a macro.

Will do.

> >  static void *rate_control_pid_alloc(struct ieee80211_local *local)
> >  {
> >  	struct rc_pid_info *pinfo;
> > +	struct rc_pid_rateinfo *rinfo;
> > +	struct ieee80211_hw_mode *mode;
> > +	int i, j, tmp;
> > +	bool s;
> >  
> >  	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
> > +	if (!pinfo)
> > +		return NULL;
> > +
> > +	mode = local->oper_hw_mode;
> > +	rinfo = kmalloc(sizeof(*rinfo) * mode->num_rates, GFP_ATOMIC);
> > +	if (!rinfo) {
> > +		kfree(pinfo);
> > +		return NULL;
> > +	}
> 
> What if the mode is changed? Have you checked the rate control algorithm
> gets reinitialized? If not, we're scheduling a crash here, when
> mode->num_rates changes.

No, I didn't because I thought that was obvious. But no, it seems that it
doesn't get reinitialized. This has to be addressed, IMHO.

> > +
> > +	/* Sort the rates. This is optimized for the most common case (i.e.
> > +	 * almost-sorted CCK+OFDM rates). */
> > +	for (i = 0; i < mode->num_rates; i++) {
> > +		rinfo[i].index = i;
> > +		if (RC_PID_FAST_START) {
> > +			rinfo[i].valid = 1;
> > +			rinfo[i].diff = 0;
> > +		} else
> > +			rinfo[i].valid = 0;
> 
> Maybe use an #ifdef RC_PID_FAST_START_HERE?

So that it can't be turned into a parameter? Nah.

> > +	}
> > +	for (i = 1; i < mode->num_rates; i++) {
> > +		s = 0;
> > +		for (j = 0; j < mode->num_rates - i; j++)
> > +			if (unlikely(mode->rates[rinfo[j].index].rate >
> > +				     mode->rates[rinfo[j + 1].index].rate)) {
> > +				tmp = rinfo[j].index;
> > +				rinfo[j].index = rinfo[j + 1].index;
> > +				rinfo[j + 1].index = tmp;
> > +				s = 1;
> > +			}
> > +		if (!s)
> > +			break;
> > +	}
> 
> There is include/linux/sort.h. I guess using this would make the code
> clearer. It took me a moment to recognize the code as bubble sort.

Heapsort (please see lib/sort.c) is suboptimal here. I think that
bubblesort is the right compromise between insertion sort (performance) and
readability here.

> What just came to my mind: Maybe it's not a good idea to actually break
> up the 80211.b then 80211.g sorting of the rates: If you have an AP,
> it'll compute separate tx rates for all stations it knows. But there
> might also be 80211.b only stations. If we have 6Mbit/s and 9Mbit/s OFDM
> rates before the 11Mbit/s OFDM rate, will we ever be able to switch to
> 11Mbit/s?

(11Mbit/s is a CCK rate)

We should be able to switch to 11Mbit/s:
[in rate_control_pid_adjust_rate()]:

        while (newidx != sta->txrate) {
                if (rate_supported(sta, mode, newidx) &&
                    (maxrate < 0 || newidx <= maxrate)) {
                        sta->txrate = newidx;
                        break;
                }

                newidx += back;
        }

> Overall, I'm still not too convinced whether this learning approach is
> worth the effort, but this is something to be figured out by testing.

Well, I didn't test this extensively. But at least I got rid of the
11-6-9-11M loop and this yields best performance (~1.5mbit/s more with
iperf) for scenarios with good signal and varying noise (tested with b43).
I have to fix a bug in b43 before further testing, though.


--
Ciao
Stefano

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Stefano Brivio]

On Mon, 10 Dec 2007 00:21:58 +0100
Stefano Brivio <stefano.brivio@polimi.it> wrote:

> > > +	/* If we just switched rate, update the rate behaviour info. */
> > > +	if (pinfo->oldrate != sta->txrate) {
> > > +		for (i = 0; i < mode->num_rates; i++)
> > > +			if (rinfo[i].index == pinfo->oldrate)
> > > +				break;
> > > +		for (j = 0; j < mode->num_rates; j++)
> > > +			if (rinfo[j].index == sta->txrate) {
> > > +				rinfo[j].valid = 1;
> > > +				break;
> > > +			}
> > 
> > This is no 3 and 4 of the call sites to the to be written
> > find_index_for_rate() function ;-)
> >
> > Or what about this idea: Store direct indices into the rinfo table, so
> > you needn't convert them back from the mode->rates indexing everywhere.
> 
> This makes sense. Unless you are working to the find_index_for_rate()
> function, which would make more sense.

Eek, sorry, bullshit. I didn't get your point here, I do now. Will write
that function.


--
Ciao
Stefano

[RFC/T][PATCH v2 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Stefano Brivio]

This patch introduces a learning algorithm in order for the PID controller
to learn how to map adjustment values to rates. This is better described in
code comments.


Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
Cc: Mattias Nissler <mattias.nissler@gmx.de>

---

Mattias,
this series addresses some of your concerns and some stupid bugs I just
noticed about. Thank you for your comments.

---

Index: wireless-2.6/net/mac80211/rc80211_pid.c
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid.c
+++ wireless-2.6/net/mac80211/rc80211_pid.c
@@ -2,6 +2,7 @@
  * Copyright 2002-2005, Instant802 Networks, Inc.
  * Copyright 2005, Devicescape Software, Inc.
  * Copyright 2007, Mattias Nissler <mattias.nissler@gmx.de>
+ * Copyright 2007, Stefano Brivio <stefano.brivio@polimi.it>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -39,12 +40,18 @@
  * an actual sliding window. Advantage is that we don't need to keep an array of
  * the last N error values and computation is easier.
  *
- * Once we have the adj value, we need to map it to a TX rate to be selected.
- * For now, we depend on the rates to be ordered in a way such that more robust
- * rates (i.e. such that exhibit a lower framed failed percentage) come first.
- * E.g. for the 802.11b/g case, we first have the b rates in ascending order,
- * then the g rates. The adj simply decides the index of the TX rate in the list
- * to switch to (relative to the current TX rate entry).
+ * Once we have the adj value, we map it to a rate by means of a learning
+ * algorithm. This algorithm keeps the state of the percentual failed frames
+ * difference between rates. The behaviour of the lowest available rate is kept
+ * as a reference value, and every time we switch between two rates, we compute
+ * the difference between the failed frames each rate exhibited. By doing so,
+ * we compare behaviours which different rates exhibited in adjacent timeslices,
+ * thus the comparison is minimally affected by external conditions. This
+ * difference gets propagated to the whole set of measurements, so that the
+ * reference is always the same. Periodically, we normalize this set so that
+ * recent events weigh the most. By comparing the adj value with this set, we
+ * avoid pejorative switches to lower rates and allow for switches to higher
+ * rates if they behaved well.
  *
  * Note that for the computations we use a fixed-point representation to avoid
  * floating point arithmetic. Hence, all values are shifted left by
@@ -78,6 +85,16 @@
  */
 #define RC_PID_TARGET_PF (20 << RC_PID_ARITH_SHIFT)
 
+/* Rate behaviour normalization quantity over time. */
+#define RC_PID_NORM_OFFSET 3
+
+/* Push high rates right after loading. */
+#define RC_PID_FAST_START 0
+
+/* Arithmetic right shift for positive and negative values for ISO C. */
+#define RC_PID_DO_ARITH_RIGHT_SHIFT(x, y) \
+	(x) < 0 ? -((-(x))) >> (y) : (x) >> (y)
+
 struct rc_pid_sta_info {
 	unsigned long last_change;
 	unsigned long last_sample;
@@ -121,6 +138,18 @@ struct rc_pid_sta_info {
 /* Algorithm parameters. We keep them on a per-algorithm approach, so they can
  * be tuned individually for each interface.
  */
+struct rc_pid_rateinfo {
+
+	/* The rate index in ieee80211_hw_mode. */
+	int index;
+
+	/* Did we do any measurement on this rate? */
+	bool valid;
+
+	/* Comparison with the lowest rate. */
+	int diff;
+};
+
 struct rc_pid_info {
 
 	/* The failed frames percentage target. */
@@ -130,15 +159,69 @@ struct rc_pid_info {
 	s32 coeff_p;
 	s32 coeff_i;
 	s32 coeff_d;
+
+	/* Rates information. */
+	struct rc_pid_rateinfo *rinfo;
+
+	/* Index of the last used rate. */
+	int oldrate;
 };
 
+static inline int rate_control_pid_r_to_i(struct rc_pid_rateinfo *r, int rate,
+					  int l)
+{
+	int i;
+
+	for (i = 0; i < l; i++)
+		if (r[i].index == rate)
+			break;
+
+	return i;
+}
+
+/* Shift the adjustment so that we won't switch to a lower rate if it exhibited
+ * a worse failed frames behaviour and we'll choose the highest rate whose
+ * failed frames behaviour is not worse than the one of the original rate
+ * target. While at it, check that the adjustment is within the ranges. Then,
+ * provide the new rate index. */
+static int rate_control_pid_shift_adjust(struct rc_pid_rateinfo *r,
+					 int adj, int cur, int l)
+{
+	int i, j, k, tmp;
+
+	if (cur + adj < 0)
+		return 0;
+	if (cur + adj >= l)
+		return l - 1;
+
+	i = rate_control_pid_r_to_i(r, cur + adj, l);
+	if (unlikely(!r[i].valid))
+		return cur + adj;
+	j = rate_control_pid_r_to_i(r, cur, l);
+
+	if (adj < 0 && r[j].valid) {
+			tmp = i;
+			for (k = j; k >= i; k--)
+				if (r[k].valid && r[k].diff <= r[j].diff)
+					tmp = k;
+			return r[tmp].index;
+	} else if (adj > 0) {
+			tmp = i;
+			for (k = i + 1; k + i < l; k++)
+				if (r[k].valid && r[k].diff <= r[i].diff)
+					tmp = k;
+			return r[tmp].index;
+	}
+	return cur + adj;
+}
 
 static void rate_control_pid_adjust_rate(struct ieee80211_local *local,
-					 struct sta_info *sta, int adj)
+					 struct sta_info *sta, int adj,
+					 struct rc_pid_rateinfo *rinfo)
 {
 	struct ieee80211_sub_if_data *sdata;
 	struct ieee80211_hw_mode *mode;
-	int newidx = sta->txrate + adj;
+	int newidx;
 	int maxrate;
 	int back = (adj > 0) ? 1 : -1;
 
@@ -151,10 +234,8 @@ static void rate_control_pid_adjust_rate
 	mode = local->oper_hw_mode;
 	maxrate = sdata->bss ? sdata->bss->max_ratectrl_rateidx : -1;
 
-	if (newidx < 0)
-		newidx = 0;
-	else if (newidx >= mode->num_rates)
-		newidx = mode->num_rates - 1;
+	newidx = rate_control_pid_shift_adjust(rinfo, adj, sta->txrate,
+					       mode->num_rates);
 
 	while (newidx != sta->txrate) {
 		if (rate_supported(sta, mode, newidx) &&
@@ -167,18 +248,39 @@ static void rate_control_pid_adjust_rate
 	}
 }
 
+/* Normalize the failed frames per-rate differences. */
+static void rate_control_pid_normalize(struct rc_pid_rateinfo *r, int l)
+{
+	int i;
+
+	if (r[0].diff > RC_PID_NORM_OFFSET)
+		r[0].diff -= RC_PID_NORM_OFFSET;
+	else if (r[0].diff < -RC_PID_NORM_OFFSET)
+		r[0].diff += RC_PID_NORM_OFFSET;
+	for (i = 0; i < l - 1; i++)
+		if (likely(r[i + 1].valid)) {
+			if (r[i + 1].diff > r[i].diff + RC_PID_NORM_OFFSET)
+				r[i + 1].diff -= RC_PID_NORM_OFFSET;
+			else if (r[i + 1].diff <= r[i].diff)
+				r[i + 1].diff += RC_PID_NORM_OFFSET;
+		}
+}
+
 static void rate_control_pid_sample(struct rc_pid_info *pinfo,
 				    struct ieee80211_local *local,
 				    struct sta_info *sta)
 {
 	struct rc_pid_sta_info *spinfo = sta->rate_ctrl_priv;
+	struct rc_pid_rateinfo *rinfo = pinfo->rinfo;
+	struct ieee80211_hw_mode *mode;
 	u32 pf;
 	s32 err_avg;
 	s32 err_prop;
 	s32 err_int;
 	s32 err_der;
-	int adj;
+	int adj, i, j, tmp;
 
+	mode = local->oper_hw_mode;
 	spinfo = sta->rate_ctrl_priv;
 	spinfo->last_sample = jiffies;
 
@@ -194,6 +296,24 @@ static void rate_control_pid_sample(stru
 		spinfo->tx_num_failed = 0;
 	}
 
+	/* If we just switched rate, update the rate behaviour info. */
+	if (pinfo->oldrate != sta->txrate) {
+
+		i = rate_control_pid_r_to_i(rinfo, pinfo->oldrate,
+					    mode->num_rates);
+		j = rate_control_pid_r_to_i(rinfo, sta->txrate,
+					    mode->num_rates);
+		rinfo[j].valid = 1;
+
+		tmp = (pf - spinfo->last_pf);
+		tmp = RC_PID_DO_ARITH_RIGHT_SHIFT(tmp, RC_PID_ARITH_SHIFT);
+
+		rinfo[j].diff = rinfo[i].diff + tmp;
+		rinfo[j].valid = 1;
+		pinfo->oldrate = sta->txrate;
+	}
+	rate_control_pid_normalize(rinfo, mode->num_rates);
+
 	/* Compute the proportional, integral and derivative errors. */
 	err_prop = RC_PID_TARGET_PF - pf;
 
@@ -207,16 +327,11 @@ static void rate_control_pid_sample(stru
 	/* Compute the controller output. */
 	adj = (err_prop * pinfo->coeff_p + err_int * pinfo->coeff_i
 	      + err_der * pinfo->coeff_d);
-
-	/* We need to do an arithmetic right shift. ISO C says this is
-	 * implementation defined for negative left operands. Hence, be
-	 * careful to get it right, also for negative values. */
-	adj = (adj < 0) ? -((-adj) >> (2 * RC_PID_ARITH_SHIFT)) :
-			  adj >> (2 * RC_PID_ARITH_SHIFT);
+	adj = RC_PID_DO_ARITH_RIGHT_SHIFT(adj, 2 * RC_PID_ARITH_SHIFT);
 
 	/* Change rate. */
 	if (adj)
-		rate_control_pid_adjust_rate(local, sta, adj);
+		rate_control_pid_adjust_rate(local, sta, adj, rinfo);
 }
 
 static void rate_control_pid_tx_status(void *priv, struct net_device *dev,
@@ -315,13 +430,55 @@ static void rate_control_pid_rate_init(v
 static void *rate_control_pid_alloc(struct ieee80211_local *local)
 {
 	struct rc_pid_info *pinfo;
+	struct rc_pid_rateinfo *rinfo;
+	struct ieee80211_hw_mode *mode;
+	int i, j, tmp;
+	bool s;
 
 	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
+	if (!pinfo)
+		return NULL;
+
+	mode = local->oper_hw_mode;
+	rinfo = kmalloc(sizeof(*rinfo) * mode->num_rates, GFP_ATOMIC);
+	if (!rinfo) {
+		kfree(pinfo);
+		return NULL;
+	}
+
+	/* Sort the rates. This is optimized for the most common case (i.e.
+	 * almost-sorted CCK+OFDM rates). */
+	for (i = 0; i < mode->num_rates; i++) {
+		rinfo[i].index = i;
+		if (RC_PID_FAST_START) {
+			rinfo[i].valid = 1;
+			rinfo[i].diff = 0;
+		} else
+			rinfo[i].valid = 0;
+	}
+	for (i = 1; i < mode->num_rates; i++) {
+		s = 0;
+		for (j = 0; j < mode->num_rates - i; j++)
+			if (unlikely(mode->rates[rinfo[j].index].rate >
+				     mode->rates[rinfo[j + 1].index].rate)) {
+				tmp = rinfo[j].index;
+				rinfo[j].index = rinfo[j + 1].index;
+				rinfo[j + 1].index = tmp;
+				s = 1;
+			}
+		if (!s)
+			break;
+	}
+
+	rinfo[0].diff = 0;
+	rinfo[0].valid = 1;
 
 	pinfo->target = RC_PID_TARGET_PF;
 	pinfo->coeff_p = RC_PID_COEFF_P;
 	pinfo->coeff_i = RC_PID_COEFF_I;
 	pinfo->coeff_d = RC_PID_COEFF_D;
+	pinfo->rinfo = rinfo;
+	pinfo->oldrate = 0;
 
 	return pinfo;
 }
@@ -330,6 +487,7 @@ static void *rate_control_pid_alloc(stru
 static void rate_control_pid_free(void *priv)
 {
 	struct rc_pid_info *pinfo = priv;
+	kfree(pinfo->rinfo);
 	kfree(pinfo);
 }
 


-- 
Ciao
Stefano

Re: [RFC/T][PATCH v2 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Mattias Nissler]

On Mon, 2007-12-10 at 03:24 +0100, Stefano Brivio wrote:
> This patch introduces a learning algorithm in order for the PID controller
> to learn how to map adjustment values to rates. This is better described in
> code comments.
> 
> 
> Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
> Cc: Mattias Nissler <mattias.nissler@gmx.de>
> 
> ---
> 
> Mattias,
> this series addresses some of your concerns and some stupid bugs I just
> noticed about. Thank you for your comments.
> 
> ---
> 
> Index: wireless-2.6/net/mac80211/rc80211_pid.c
> ===================================================================
> --- wireless-2.6.orig/net/mac80211/rc80211_pid.c
> +++ wireless-2.6/net/mac80211/rc80211_pid.c
> @@ -2,6 +2,7 @@
>   * Copyright 2002-2005, Instant802 Networks, Inc.
>   * Copyright 2005, Devicescape Software, Inc.
>   * Copyright 2007, Mattias Nissler <mattias.nissler@gmx.de>
> + * Copyright 2007, Stefano Brivio <stefano.brivio@polimi.it>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -39,12 +40,18 @@
>   * an actual sliding window. Advantage is that we don't need to keep an array of
>   * the last N error values and computation is easier.
>   *
> - * Once we have the adj value, we need to map it to a TX rate to be selected.
> - * For now, we depend on the rates to be ordered in a way such that more robust
> - * rates (i.e. such that exhibit a lower framed failed percentage) come first.
> - * E.g. for the 802.11b/g case, we first have the b rates in ascending order,
> - * then the g rates. The adj simply decides the index of the TX rate in the list
> - * to switch to (relative to the current TX rate entry).
> + * Once we have the adj value, we map it to a rate by means of a learning
> + * algorithm. This algorithm keeps the state of the percentual failed frames
> + * difference between rates. The behaviour of the lowest available rate is kept
> + * as a reference value, and every time we switch between two rates, we compute
> + * the difference between the failed frames each rate exhibited. By doing so,
> + * we compare behaviours which different rates exhibited in adjacent timeslices,
> + * thus the comparison is minimally affected by external conditions. This
> + * difference gets propagated to the whole set of measurements, so that the
> + * reference is always the same. Periodically, we normalize this set so that
> + * recent events weigh the most. By comparing the adj value with this set, we
> + * avoid pejorative switches to lower rates and allow for switches to higher
> + * rates if they behaved well.
>   *
>   * Note that for the computations we use a fixed-point representation to avoid
>   * floating point arithmetic. Hence, all values are shifted left by
> @@ -78,6 +85,16 @@
>   */
>  #define RC_PID_TARGET_PF (20 << RC_PID_ARITH_SHIFT)
>  
> +/* Rate behaviour normalization quantity over time. */
> +#define RC_PID_NORM_OFFSET 3
> +
> +/* Push high rates right after loading. */
> +#define RC_PID_FAST_START 0
> +
> +/* Arithmetic right shift for positive and negative values for ISO C. */
> +#define RC_PID_DO_ARITH_RIGHT_SHIFT(x, y) \
> +	(x) < 0 ? -((-(x))) >> (y) : (x) >> (y)
> +
>  struct rc_pid_sta_info {
>  	unsigned long last_change;
>  	unsigned long last_sample;
> @@ -121,6 +138,18 @@ struct rc_pid_sta_info {
>  /* Algorithm parameters. We keep them on a per-algorithm approach, so they can
>   * be tuned individually for each interface.
>   */
> +struct rc_pid_rateinfo {
> +
> +	/* The rate index in ieee80211_hw_mode. */
> +	int index;
> +
> +	/* Did we do any measurement on this rate? */
> +	bool valid;
> +
> +	/* Comparison with the lowest rate. */
> +	int diff;
> +};
> +
>  struct rc_pid_info {
>  
>  	/* The failed frames percentage target. */
> @@ -130,15 +159,69 @@ struct rc_pid_info {
>  	s32 coeff_p;
>  	s32 coeff_i;
>  	s32 coeff_d;
> +
> +	/* Rates information. */
> +	struct rc_pid_rateinfo *rinfo;
> +
> +	/* Index of the last used rate. */
> +	int oldrate;
>  };
>  
> +static inline int rate_control_pid_r_to_i(struct rc_pid_rateinfo *r, int rate,
> +					  int l)
> +{
> +	int i;
> +
> +	for (i = 0; i < l; i++)
> +		if (r[i].index == rate)
> +			break;
> +
> +	return i;
> +}

Isn't the store-direct-rinfo-indices approach nicer? Why did you decide
for the index conversion function in favour of storing direct indices?

> +
> +/* Shift the adjustment so that we won't switch to a lower rate if it exhibited
> + * a worse failed frames behaviour and we'll choose the highest rate whose
> + * failed frames behaviour is not worse than the one of the original rate
> + * target. While at it, check that the adjustment is within the ranges. Then,
> + * provide the new rate index. */
> +static int rate_control_pid_shift_adjust(struct rc_pid_rateinfo *r,
> +					 int adj, int cur, int l)
> +{
> +	int i, j, k, tmp;
> +
> +	if (cur + adj < 0)
> +		return 0;
> +	if (cur + adj >= l)
> +		return l - 1;
> +
> +	i = rate_control_pid_r_to_i(r, cur + adj, l);
> +	if (unlikely(!r[i].valid))
> +		return cur + adj;
> +	j = rate_control_pid_r_to_i(r, cur, l);
> +
> +	if (adj < 0 && r[j].valid) {
> +			tmp = i;
> +			for (k = j; k >= i; k--)
> +				if (r[k].valid && r[k].diff <= r[j].diff)
> +					tmp = k;
> +			return r[tmp].index;
> +	} else if (adj > 0) {
> +			tmp = i;
> +			for (k = i + 1; k + i < l; k++)
> +				if (r[k].valid && r[k].diff <= r[i].diff)
> +					tmp = k;
> +			return r[tmp].index;
> +	}
> +	return cur + adj;
> +}
>  
>  static void rate_control_pid_adjust_rate(struct ieee80211_local *local,
> -					 struct sta_info *sta, int adj)
> +					 struct sta_info *sta, int adj,
> +					 struct rc_pid_rateinfo *rinfo)
>  {
>  	struct ieee80211_sub_if_data *sdata;
>  	struct ieee80211_hw_mode *mode;
> -	int newidx = sta->txrate + adj;
> +	int newidx;
>  	int maxrate;
>  	int back = (adj > 0) ? 1 : -1;
>  
> @@ -151,10 +234,8 @@ static void rate_control_pid_adjust_rate
>  	mode = local->oper_hw_mode;
>  	maxrate = sdata->bss ? sdata->bss->max_ratectrl_rateidx : -1;
>  
> -	if (newidx < 0)
> -		newidx = 0;
> -	else if (newidx >= mode->num_rates)
> -		newidx = mode->num_rates - 1;
> +	newidx = rate_control_pid_shift_adjust(rinfo, adj, sta->txrate,
> +					       mode->num_rates);
>  
>  	while (newidx != sta->txrate) {
>  		if (rate_supported(sta, mode, newidx) &&
> @@ -167,18 +248,39 @@ static void rate_control_pid_adjust_rate
>  	}
>  }
>  
> +/* Normalize the failed frames per-rate differences. */
> +static void rate_control_pid_normalize(struct rc_pid_rateinfo *r, int l)
> +{
> +	int i;
> +
> +	if (r[0].diff > RC_PID_NORM_OFFSET)
> +		r[0].diff -= RC_PID_NORM_OFFSET;
> +	else if (r[0].diff < -RC_PID_NORM_OFFSET)
> +		r[0].diff += RC_PID_NORM_OFFSET;
> +	for (i = 0; i < l - 1; i++)
> +		if (likely(r[i + 1].valid)) {
> +			if (r[i + 1].diff > r[i].diff + RC_PID_NORM_OFFSET)
> +				r[i + 1].diff -= RC_PID_NORM_OFFSET;
> +			else if (r[i + 1].diff <= r[i].diff)
> +				r[i + 1].diff += RC_PID_NORM_OFFSET;
> +		}
> +}
> +
>  static void rate_control_pid_sample(struct rc_pid_info *pinfo,
>  				    struct ieee80211_local *local,
>  				    struct sta_info *sta)
>  {
>  	struct rc_pid_sta_info *spinfo = sta->rate_ctrl_priv;
> +	struct rc_pid_rateinfo *rinfo = pinfo->rinfo;
> +	struct ieee80211_hw_mode *mode;
>  	u32 pf;
>  	s32 err_avg;
>  	s32 err_prop;
>  	s32 err_int;
>  	s32 err_der;
> -	int adj;
> +	int adj, i, j, tmp;
>  
> +	mode = local->oper_hw_mode;
>  	spinfo = sta->rate_ctrl_priv;
>  	spinfo->last_sample = jiffies;
>  
> @@ -194,6 +296,24 @@ static void rate_control_pid_sample(stru
>  		spinfo->tx_num_failed = 0;
>  	}
>  
> +	/* If we just switched rate, update the rate behaviour info. */
> +	if (pinfo->oldrate != sta->txrate) {
> +
> +		i = rate_control_pid_r_to_i(rinfo, pinfo->oldrate,
> +					    mode->num_rates);
> +		j = rate_control_pid_r_to_i(rinfo, sta->txrate,
> +					    mode->num_rates);
> +		rinfo[j].valid = 1;
> +
> +		tmp = (pf - spinfo->last_pf);
> +		tmp = RC_PID_DO_ARITH_RIGHT_SHIFT(tmp, RC_PID_ARITH_SHIFT);
> +
> +		rinfo[j].diff = rinfo[i].diff + tmp;
> +		rinfo[j].valid = 1;
> +		pinfo->oldrate = sta->txrate;
> +	}
> +	rate_control_pid_normalize(rinfo, mode->num_rates);
> +
>  	/* Compute the proportional, integral and derivative errors. */
>  	err_prop = RC_PID_TARGET_PF - pf;
>  
> @@ -207,16 +327,11 @@ static void rate_control_pid_sample(stru
>  	/* Compute the controller output. */
>  	adj = (err_prop * pinfo->coeff_p + err_int * pinfo->coeff_i
>  	      + err_der * pinfo->coeff_d);
> -
> -	/* We need to do an arithmetic right shift. ISO C says this is
> -	 * implementation defined for negative left operands. Hence, be
> -	 * careful to get it right, also for negative values. */
> -	adj = (adj < 0) ? -((-adj) >> (2 * RC_PID_ARITH_SHIFT)) :
> -			  adj >> (2 * RC_PID_ARITH_SHIFT);
> +	adj = RC_PID_DO_ARITH_RIGHT_SHIFT(adj, 2 * RC_PID_ARITH_SHIFT);
>  
>  	/* Change rate. */
>  	if (adj)
> -		rate_control_pid_adjust_rate(local, sta, adj);
> +		rate_control_pid_adjust_rate(local, sta, adj, rinfo);
>  }
>  
>  static void rate_control_pid_tx_status(void *priv, struct net_device *dev,
> @@ -315,13 +430,55 @@ static void rate_control_pid_rate_init(v
>  static void *rate_control_pid_alloc(struct ieee80211_local *local)
>  {
>  	struct rc_pid_info *pinfo;
> +	struct rc_pid_rateinfo *rinfo;
> +	struct ieee80211_hw_mode *mode;
> +	int i, j, tmp;
> +	bool s;
>  
>  	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
> +	if (!pinfo)
> +		return NULL;
> +
> +	mode = local->oper_hw_mode;
> +	rinfo = kmalloc(sizeof(*rinfo) * mode->num_rates, GFP_ATOMIC);
> +	if (!rinfo) {
> +		kfree(pinfo);
> +		return NULL;
> +	}
> +
> +	/* Sort the rates. This is optimized for the most common case (i.e.
> +	 * almost-sorted CCK+OFDM rates). */
> +	for (i = 0; i < mode->num_rates; i++) {
> +		rinfo[i].index = i;
> +		if (RC_PID_FAST_START) {
> +			rinfo[i].valid = 1;
> +			rinfo[i].diff = 0;
> +		} else
> +			rinfo[i].valid = 0;
> +	}
> +	for (i = 1; i < mode->num_rates; i++) {
> +		s = 0;
> +		for (j = 0; j < mode->num_rates - i; j++)
> +			if (unlikely(mode->rates[rinfo[j].index].rate >
> +				     mode->rates[rinfo[j + 1].index].rate)) {
> +				tmp = rinfo[j].index;
> +				rinfo[j].index = rinfo[j + 1].index;
> +				rinfo[j + 1].index = tmp;
> +				s = 1;
> +			}
> +		if (!s)
> +			break;
> +	}
> +
> +	rinfo[0].diff = 0;
> +	rinfo[0].valid = 1;
>  
>  	pinfo->target = RC_PID_TARGET_PF;
>  	pinfo->coeff_p = RC_PID_COEFF_P;
>  	pinfo->coeff_i = RC_PID_COEFF_I;
>  	pinfo->coeff_d = RC_PID_COEFF_D;
> +	pinfo->rinfo = rinfo;
> +	pinfo->oldrate = 0;
>  
>  	return pinfo;
>  }
> @@ -330,6 +487,7 @@ static void *rate_control_pid_alloc(stru
>  static void rate_control_pid_free(void *priv)
>  {
>  	struct rc_pid_info *pinfo = priv;
> +	kfree(pinfo->rinfo);
>  	kfree(pinfo);
>  }
>  
> 
> 

Re: [RFC/T][PATCH v2 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Stefano Brivio]

On Mon, 10 Dec 2007 07:51:20 +0100
Mattias Nissler <mattias.nissler@gmx.de> wrote:

> > +static inline int rate_control_pid_r_to_i(struct rc_pid_rateinfo *r, int rate,
> > +					  int l)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < l; i++)
> > +		if (r[i].index == rate)
> > +			break;
> > +
> > +	return i;
> > +}
> 
> Isn't the store-direct-rinfo-indices approach nicer? Why did you decide
> for the index conversion function in favour of storing direct indices?

Yesterday there was a reason, now I just forgot it. If I can't recall about
it within tomorrow, I'll switch to the other approach. ;)


-- 
Ciao
Stefano

[RFC/T][PATCH v3 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Stefano Brivio]

This patch introduces a learning algorithm in order for the PID controller
to learn how to map adjustment values to rates. This is better described in
code comments.


Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
Cc: Mattias Nissler <mattias.nissler@gmx.de>

---

This version addresses other concerns by Mattias. This should be almost in
shape for inclusion into the tree.

---

Index: wireless-2.6/net/mac80211/rc80211_pid.c
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid.c
+++ wireless-2.6/net/mac80211/rc80211_pid.c
@@ -2,6 +2,7 @@
  * Copyright 2002-2005, Instant802 Networks, Inc.
  * Copyright 2005, Devicescape Software, Inc.
  * Copyright 2007, Mattias Nissler <mattias.nissler@gmx.de>
+ * Copyright 2007, Stefano Brivio <stefano.brivio@polimi.it>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -39,12 +40,18 @@
  * an actual sliding window. Advantage is that we don't need to keep an array of
  * the last N error values and computation is easier.
  *
- * Once we have the adj value, we need to map it to a TX rate to be selected.
- * For now, we depend on the rates to be ordered in a way such that more robust
- * rates (i.e. such that exhibit a lower framed failed percentage) come first.
- * E.g. for the 802.11b/g case, we first have the b rates in ascending order,
- * then the g rates. The adj simply decides the index of the TX rate in the list
- * to switch to (relative to the current TX rate entry).
+ * Once we have the adj value, we map it to a rate by means of a learning
+ * algorithm. This algorithm keeps the state of the percentual failed frames
+ * difference between rates. The behaviour of the lowest available rate is kept
+ * as a reference value, and every time we switch between two rates, we compute
+ * the difference between the failed frames each rate exhibited. By doing so,
+ * we compare behaviours which different rates exhibited in adjacent timeslices,
+ * thus the comparison is minimally affected by external conditions. This
+ * difference gets propagated to the whole set of measurements, so that the
+ * reference is always the same. Periodically, we normalize this set so that
+ * recent events weigh the most. By comparing the adj value with this set, we
+ * avoid pejorative switches to lower rates and allow for switches to higher
+ * rates if they behaved well.
  *
  * Note that for the computations we use a fixed-point representation to avoid
  * floating point arithmetic. Hence, all values are shifted left by
@@ -78,6 +85,16 @@
  */
 #define RC_PID_TARGET_PF (20 << RC_PID_ARITH_SHIFT)
 
+/* Rate behaviour normalization quantity over time. */
+#define RC_PID_NORM_OFFSET 3
+
+/* Push high rates right after loading. */
+#define RC_PID_FAST_START 0
+
+/* Arithmetic right shift for positive and negative values for ISO C. */
+#define RC_PID_DO_ARITH_RIGHT_SHIFT(x, y) \
+	(x) < 0 ? -((-(x))) >> (y) : (x) >> (y)
+
 struct rc_pid_sta_info {
 	unsigned long last_change;
 	unsigned long last_sample;
@@ -121,6 +138,21 @@ struct rc_pid_sta_info {
 /* Algorithm parameters. We keep them on a per-algorithm approach, so they can
  * be tuned individually for each interface.
  */
+struct rc_pid_rateinfo {
+
+	/* Map sorted rates to rates in ieee80211_hw_mode. */
+	int index;
+
+	/* Map rates in ieee80211_hw_mode to sorted rates. */
+	int rev_index;
+
+	/* Did we do any measurement on this rate? */
+	bool valid;
+
+	/* Comparison with the lowest rate. */
+	int diff;
+};
+
 struct rc_pid_info {
 
 	/* The failed frames percentage target. */
@@ -130,15 +162,59 @@ struct rc_pid_info {
 	s32 coeff_p;
 	s32 coeff_i;
 	s32 coeff_d;
+
+	/* Rates information. */
+	struct rc_pid_rateinfo *rinfo;
+
+	/* Index of the last used rate. */
+	int oldrate;
 };
 
+/* Shift the adjustment so that we won't switch to a lower rate if it exhibited
+ * a worse failed frames behaviour and we'll choose the highest rate whose
+ * failed frames behaviour is not worse than the one of the original rate
+ * target. While at it, check that the adjustment is within the ranges. Then,
+ * provide the new rate index. */
+static int rate_control_pid_shift_adjust(struct rc_pid_rateinfo *r,
+					 int adj, int cur, int l)
+{
+	int i, j, k, tmp;
+
+	if (cur + adj < 0)
+		return 0;
+	if (cur + adj >= l)
+		return l - 1;
+
+	i = r[cur + adj].rev_index;
+
+	if (unlikely(!r[i].valid))
+		return cur + adj;
+
+	j = r[cur].rev_index;
+
+	if (adj < 0 && r[j].valid) {
+			tmp = i;
+			for (k = j; k >= i; k--)
+				if (r[k].valid && r[k].diff <= r[j].diff)
+					tmp = k;
+			return r[tmp].index;
+	} else if (adj > 0) {
+			tmp = i;
+			for (k = i + 1; k + i < l; k++)
+				if (r[k].valid && r[k].diff <= r[i].diff)
+					tmp = k;
+			return r[tmp].index;
+	}
+	return cur + adj;
+}
 
 static void rate_control_pid_adjust_rate(struct ieee80211_local *local,
-					 struct sta_info *sta, int adj)
+					 struct sta_info *sta, int adj,
+					 struct rc_pid_rateinfo *rinfo)
 {
 	struct ieee80211_sub_if_data *sdata;
 	struct ieee80211_hw_mode *mode;
-	int newidx = sta->txrate + adj;
+	int newidx;
 	int maxrate;
 	int back = (adj > 0) ? 1 : -1;
 
@@ -151,10 +227,8 @@ static void rate_control_pid_adjust_rate
 	mode = local->oper_hw_mode;
 	maxrate = sdata->bss ? sdata->bss->max_ratectrl_rateidx : -1;
 
-	if (newidx < 0)
-		newidx = 0;
-	else if (newidx >= mode->num_rates)
-		newidx = mode->num_rates - 1;
+	newidx = rate_control_pid_shift_adjust(rinfo, adj, sta->txrate,
+					       mode->num_rates);
 
 	while (newidx != sta->txrate) {
 		if (rate_supported(sta, mode, newidx) &&
@@ -167,18 +241,39 @@ static void rate_control_pid_adjust_rate
 	}
 }
 
+/* Normalize the failed frames per-rate differences. */
+static void rate_control_pid_normalize(struct rc_pid_rateinfo *r, int l)
+{
+	int i;
+
+	if (r[0].diff > RC_PID_NORM_OFFSET)
+		r[0].diff -= RC_PID_NORM_OFFSET;
+	else if (r[0].diff < -RC_PID_NORM_OFFSET)
+		r[0].diff += RC_PID_NORM_OFFSET;
+	for (i = 0; i < l - 1; i++)
+		if (likely(r[i + 1].valid)) {
+			if (r[i + 1].diff > r[i].diff + RC_PID_NORM_OFFSET)
+				r[i + 1].diff -= RC_PID_NORM_OFFSET;
+			else if (r[i + 1].diff <= r[i].diff)
+				r[i + 1].diff += RC_PID_NORM_OFFSET;
+		}
+}
+
 static void rate_control_pid_sample(struct rc_pid_info *pinfo,
 				    struct ieee80211_local *local,
 				    struct sta_info *sta)
 {
 	struct rc_pid_sta_info *spinfo = sta->rate_ctrl_priv;
+	struct rc_pid_rateinfo *rinfo = pinfo->rinfo;
+	struct ieee80211_hw_mode *mode;
 	u32 pf;
 	s32 err_avg;
 	s32 err_prop;
 	s32 err_int;
 	s32 err_der;
-	int adj;
+	int adj, i, j, tmp;
 
+	mode = local->oper_hw_mode;
 	spinfo = sta->rate_ctrl_priv;
 	spinfo->last_sample = jiffies;
 
@@ -194,6 +289,23 @@ static void rate_control_pid_sample(stru
 		spinfo->tx_num_failed = 0;
 	}
 
+	/* If we just switched rate, update the rate behaviour info. */
+	if (pinfo->oldrate != sta->txrate) {
+
+		i = rinfo[pinfo->oldrate].rev_index;
+		j = rinfo[sta->txrate].rev_index;
+
+		rinfo[j].valid = 1;
+
+		tmp = (pf - spinfo->last_pf);
+		tmp = RC_PID_DO_ARITH_RIGHT_SHIFT(tmp, RC_PID_ARITH_SHIFT);
+
+		rinfo[j].diff = rinfo[i].diff + tmp;
+		rinfo[j].valid = 1;
+		pinfo->oldrate = sta->txrate;
+	}
+	rate_control_pid_normalize(rinfo, mode->num_rates);
+
 	/* Compute the proportional, integral and derivative errors. */
 	err_prop = RC_PID_TARGET_PF - pf;
 
@@ -207,16 +319,11 @@ static void rate_control_pid_sample(stru
 	/* Compute the controller output. */
 	adj = (err_prop * pinfo->coeff_p + err_int * pinfo->coeff_i
 	      + err_der * pinfo->coeff_d);
-
-	/* We need to do an arithmetic right shift. ISO C says this is
-	 * implementation defined for negative left operands. Hence, be
-	 * careful to get it right, also for negative values. */
-	adj = (adj < 0) ? -((-adj) >> (2 * RC_PID_ARITH_SHIFT)) :
-			  adj >> (2 * RC_PID_ARITH_SHIFT);
+	adj = RC_PID_DO_ARITH_RIGHT_SHIFT(adj, 2 * RC_PID_ARITH_SHIFT);
 
 	/* Change rate. */
 	if (adj)
-		rate_control_pid_adjust_rate(local, sta, adj);
+		rate_control_pid_adjust_rate(local, sta, adj, rinfo);
 }
 
 static void rate_control_pid_tx_status(void *priv, struct net_device *dev,
@@ -315,13 +422,61 @@ static void rate_control_pid_rate_init(v
 static void *rate_control_pid_alloc(struct ieee80211_local *local)
 {
 	struct rc_pid_info *pinfo;
+	struct rc_pid_rateinfo *rinfo;
+	struct ieee80211_hw_mode *mode;
+	int i, j, tmp;
+	bool s;
 
 	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
+	if (!pinfo)
+		return NULL;
+
+	/* We can safely assume that oper_hw_mode won't change unless we get
+	 * reinitialized. */
+	mode = local->oper_hw_mode;
+	rinfo = kmalloc(sizeof(*rinfo) * mode->num_rates, GFP_ATOMIC);
+	if (!rinfo) {
+		kfree(pinfo);
+		return NULL;
+	}
+
+	/* Sort the rates. This is optimized for the most common case (i.e.
+	 * almost-sorted CCK+OFDM rates). Kind of bubble-sort with reversed
+	 * mapping too. */
+	for (i = 0; i < mode->num_rates; i++) {
+		rinfo[i].index = i;
+		rinfo[i].rev_index = i;
+		if (RC_PID_FAST_START) {
+			rinfo[i].valid = 1;
+			rinfo[i].diff = 0;
+		} else
+			rinfo[i].valid = 0;
+	}
+	for (i = 1; i < mode->num_rates; i++) {
+		s = 0;
+		for (j = 0; j < mode->num_rates - i; j++)
+			if (unlikely(mode->rates[rinfo[j].index].rate >
+				     mode->rates[rinfo[j + 1].index].rate)) {
+				tmp = rinfo[j].index;
+				rinfo[j].index = rinfo[j + 1].index;
+				rinfo[j + 1].index = tmp;
+				rinfo[rinfo[j].index].rev_index = j;
+				rinfo[rinfo[j + 1].index].rev_index = j + 1;
+				s = 1;
+			}
+		if (!s)
+			break;
+	}
+
+	rinfo[0].diff = 0;
+	rinfo[0].valid = 1;
 
 	pinfo->target = RC_PID_TARGET_PF;
 	pinfo->coeff_p = RC_PID_COEFF_P;
 	pinfo->coeff_i = RC_PID_COEFF_I;
 	pinfo->coeff_d = RC_PID_COEFF_D;
+	pinfo->rinfo = rinfo;
+	pinfo->oldrate = 0;
 
 	return pinfo;
 }
@@ -330,6 +485,7 @@ static void *rate_control_pid_alloc(stru
 static void rate_control_pid_free(void *priv)
 {
 	struct rc_pid_info *pinfo = priv;
+	kfree(pinfo->rinfo);
 	kfree(pinfo);
 }
 


-- 
Ciao
Stefano

[RFC/T][PATCH v4 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Stefano Brivio]

This patch introduces a learning algorithm in order for the PID controller
to learn how to map adjustment values to rates. This is better described in
code comments.


Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
Cc: Mattias Nissler <mattias.nissler@gmx.de>

---

This fixes a HUGE typo in RC_PID_DO_ARITH_RIGHT_SHIFT. ;)

---

Index: wireless-2.6/net/mac80211/rc80211_pid.c
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid.c
+++ wireless-2.6/net/mac80211/rc80211_pid.c
@@ -2,6 +2,7 @@
  * Copyright 2002-2005, Instant802 Networks, Inc.
  * Copyright 2005, Devicescape Software, Inc.
  * Copyright 2007, Mattias Nissler <mattias.nissler@gmx.de>
+ * Copyright 2007, Stefano Brivio <stefano.brivio@polimi.it>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -39,12 +40,18 @@
  * an actual sliding window. Advantage is that we don't need to keep an array of
  * the last N error values and computation is easier.
  *
- * Once we have the adj value, we need to map it to a TX rate to be selected.
- * For now, we depend on the rates to be ordered in a way such that more robust
- * rates (i.e. such that exhibit a lower framed failed percentage) come first.
- * E.g. for the 802.11b/g case, we first have the b rates in ascending order,
- * then the g rates. The adj simply decides the index of the TX rate in the list
- * to switch to (relative to the current TX rate entry).
+ * Once we have the adj value, we map it to a rate by means of a learning
+ * algorithm. This algorithm keeps the state of the percentual failed frames
+ * difference between rates. The behaviour of the lowest available rate is kept
+ * as a reference value, and every time we switch between two rates, we compute
+ * the difference between the failed frames each rate exhibited. By doing so,
+ * we compare behaviours which different rates exhibited in adjacent timeslices,
+ * thus the comparison is minimally affected by external conditions. This
+ * difference gets propagated to the whole set of measurements, so that the
+ * reference is always the same. Periodically, we normalize this set so that
+ * recent events weigh the most. By comparing the adj value with this set, we
+ * avoid pejorative switches to lower rates and allow for switches to higher
+ * rates if they behaved well.
  *
  * Note that for the computations we use a fixed-point representation to avoid
  * floating point arithmetic. Hence, all values are shifted left by
@@ -78,6 +85,16 @@
  */
 #define RC_PID_TARGET_PF (20 << RC_PID_ARITH_SHIFT)
 
+/* Rate behaviour normalization quantity over time. */
+#define RC_PID_NORM_OFFSET 3
+
+/* Push high rates right after loading. */
+#define RC_PID_FAST_START 0
+
+/* Arithmetic right shift for positive and negative values for ISO C. */
+#define RC_PID_DO_ARITH_RIGHT_SHIFT(x, y) \
+	(x) < 0 ? -((-(x)) >> (y)) : (x) >> (y)
+
 struct rc_pid_sta_info {
 	unsigned long last_change;
 	unsigned long last_sample;
@@ -121,6 +138,21 @@ struct rc_pid_sta_info {
 /* Algorithm parameters. We keep them on a per-algorithm approach, so they can
  * be tuned individually for each interface.
  */
+struct rc_pid_rateinfo {
+
+	/* Map sorted rates to rates in ieee80211_hw_mode. */
+	int index;
+
+	/* Map rates in ieee80211_hw_mode to sorted rates. */
+	int rev_index;
+
+	/* Did we do any measurement on this rate? */
+	bool valid;
+
+	/* Comparison with the lowest rate. */
+	int diff;
+};
+
 struct rc_pid_info {
 
 	/* The failed frames percentage target. */
@@ -130,15 +162,59 @@ struct rc_pid_info {
 	s32 coeff_p;
 	s32 coeff_i;
 	s32 coeff_d;
+
+	/* Rates information. */
+	struct rc_pid_rateinfo *rinfo;
+
+	/* Index of the last used rate. */
+	int oldrate;
 };
 
+/* Shift the adjustment so that we won't switch to a lower rate if it exhibited
+ * a worse failed frames behaviour and we'll choose the highest rate whose
+ * failed frames behaviour is not worse than the one of the original rate
+ * target. While at it, check that the adjustment is within the ranges. Then,
+ * provide the new rate index. */
+static int rate_control_pid_shift_adjust(struct rc_pid_rateinfo *r,
+					 int adj, int cur, int l)
+{
+	int i, j, k, tmp;
+
+	if (cur + adj < 0)
+		return 0;
+	if (cur + adj >= l)
+		return l - 1;
+
+	i = r[cur + adj].rev_index;
+
+	if (unlikely(!r[i].valid))
+		return cur + adj;
+
+	j = r[cur].rev_index;
+
+	if (adj < 0 && r[j].valid) {
+			tmp = i;
+			for (k = j; k >= i; k--)
+				if (r[k].valid && r[k].diff <= r[j].diff)
+					tmp = k;
+			return r[tmp].index;
+	} else if (adj > 0) {
+			tmp = i;
+			for (k = i + 1; k + i < l; k++)
+				if (r[k].valid && r[k].diff <= r[i].diff)
+					tmp = k;
+			return r[tmp].index;
+	}
+	return cur + adj;
+}
 
 static void rate_control_pid_adjust_rate(struct ieee80211_local *local,
-					 struct sta_info *sta, int adj)
+					 struct sta_info *sta, int adj,
+					 struct rc_pid_rateinfo *rinfo)
 {
 	struct ieee80211_sub_if_data *sdata;
 	struct ieee80211_hw_mode *mode;
-	int newidx = sta->txrate + adj;
+	int newidx;
 	int maxrate;
 	int back = (adj > 0) ? 1 : -1;
 
@@ -151,10 +227,8 @@ static void rate_control_pid_adjust_rate
 	mode = local->oper_hw_mode;
 	maxrate = sdata->bss ? sdata->bss->max_ratectrl_rateidx : -1;
 
-	if (newidx < 0)
-		newidx = 0;
-	else if (newidx >= mode->num_rates)
-		newidx = mode->num_rates - 1;
+	newidx = rate_control_pid_shift_adjust(rinfo, adj, sta->txrate,
+					       mode->num_rates);
 
 	while (newidx != sta->txrate) {
 		if (rate_supported(sta, mode, newidx) &&
@@ -167,18 +241,39 @@ static void rate_control_pid_adjust_rate
 	}
 }
 
+/* Normalize the failed frames per-rate differences. */
+static void rate_control_pid_normalize(struct rc_pid_rateinfo *r, int l)
+{
+	int i;
+
+	if (r[0].diff > RC_PID_NORM_OFFSET)
+		r[0].diff -= RC_PID_NORM_OFFSET;
+	else if (r[0].diff < -RC_PID_NORM_OFFSET)
+		r[0].diff += RC_PID_NORM_OFFSET;
+	for (i = 0; i < l - 1; i++)
+		if (likely(r[i + 1].valid)) {
+			if (r[i + 1].diff > r[i].diff + RC_PID_NORM_OFFSET)
+				r[i + 1].diff -= RC_PID_NORM_OFFSET;
+			else if (r[i + 1].diff <= r[i].diff)
+				r[i + 1].diff += RC_PID_NORM_OFFSET;
+		}
+}
+
 static void rate_control_pid_sample(struct rc_pid_info *pinfo,
 				    struct ieee80211_local *local,
 				    struct sta_info *sta)
 {
 	struct rc_pid_sta_info *spinfo = sta->rate_ctrl_priv;
+	struct rc_pid_rateinfo *rinfo = pinfo->rinfo;
+	struct ieee80211_hw_mode *mode;
 	u32 pf;
 	s32 err_avg;
 	s32 err_prop;
 	s32 err_int;
 	s32 err_der;
-	int adj;
+	int adj, i, j, tmp;
 
+	mode = local->oper_hw_mode;
 	spinfo = sta->rate_ctrl_priv;
 	spinfo->last_sample = jiffies;
 
@@ -194,6 +289,23 @@ static void rate_control_pid_sample(stru
 		spinfo->tx_num_failed = 0;
 	}
 
+	/* If we just switched rate, update the rate behaviour info. */
+	if (pinfo->oldrate != sta->txrate) {
+
+		i = rinfo[pinfo->oldrate].rev_index;
+		j = rinfo[sta->txrate].rev_index;
+
+		rinfo[j].valid = 1;
+
+		tmp = (pf - spinfo->last_pf);
+		tmp = RC_PID_DO_ARITH_RIGHT_SHIFT(tmp, RC_PID_ARITH_SHIFT);
+
+		rinfo[j].diff = rinfo[i].diff + tmp;
+		rinfo[j].valid = 1;
+		pinfo->oldrate = sta->txrate;
+	}
+	rate_control_pid_normalize(rinfo, mode->num_rates);
+
 	/* Compute the proportional, integral and derivative errors. */
 	err_prop = RC_PID_TARGET_PF - pf;
 
@@ -207,16 +319,11 @@ static void rate_control_pid_sample(stru
 	/* Compute the controller output. */
 	adj = (err_prop * pinfo->coeff_p + err_int * pinfo->coeff_i
 	      + err_der * pinfo->coeff_d);
-
-	/* We need to do an arithmetic right shift. ISO C says this is
-	 * implementation defined for negative left operands. Hence, be
-	 * careful to get it right, also for negative values. */
-	adj = (adj < 0) ? -((-adj) >> (2 * RC_PID_ARITH_SHIFT)) :
-			  adj >> (2 * RC_PID_ARITH_SHIFT);
+	adj = RC_PID_DO_ARITH_RIGHT_SHIFT(adj, 2 * RC_PID_ARITH_SHIFT);
 
 	/* Change rate. */
 	if (adj)
-		rate_control_pid_adjust_rate(local, sta, adj);
+		rate_control_pid_adjust_rate(local, sta, adj, rinfo);
 }
 
 static void rate_control_pid_tx_status(void *priv, struct net_device *dev,
@@ -315,13 +422,61 @@ static void rate_control_pid_rate_init(v
 static void *rate_control_pid_alloc(struct ieee80211_local *local)
 {
 	struct rc_pid_info *pinfo;
+	struct rc_pid_rateinfo *rinfo;
+	struct ieee80211_hw_mode *mode;
+	int i, j, tmp;
+	bool s;
 
 	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
+	if (!pinfo)
+		return NULL;
+
+	/* We can safely assume that oper_hw_mode won't change unless we get
+	 * reinitialized. */
+	mode = local->oper_hw_mode;
+	rinfo = kmalloc(sizeof(*rinfo) * mode->num_rates, GFP_ATOMIC);
+	if (!rinfo) {
+		kfree(pinfo);
+		return NULL;
+	}
+
+	/* Sort the rates. This is optimized for the most common case (i.e.
+	 * almost-sorted CCK+OFDM rates). Kind of bubble-sort with reversed
+	 * mapping too. */
+	for (i = 0; i < mode->num_rates; i++) {
+		rinfo[i].index = i;
+		rinfo[i].rev_index = i;
+		if (RC_PID_FAST_START) {
+			rinfo[i].valid = 1;
+			rinfo[i].diff = 0;
+		} else
+			rinfo[i].valid = 0;
+	}
+	for (i = 1; i < mode->num_rates; i++) {
+		s = 0;
+		for (j = 0; j < mode->num_rates - i; j++)
+			if (unlikely(mode->rates[rinfo[j].index].rate >
+				     mode->rates[rinfo[j + 1].index].rate)) {
+				tmp = rinfo[j].index;
+				rinfo[j].index = rinfo[j + 1].index;
+				rinfo[j + 1].index = tmp;
+				rinfo[rinfo[j].index].rev_index = j;
+				rinfo[rinfo[j + 1].index].rev_index = j + 1;
+				s = 1;
+			}
+		if (!s)
+			break;
+	}
+
+	rinfo[0].diff = 0;
+	rinfo[0].valid = 1;
 
 	pinfo->target = RC_PID_TARGET_PF;
 	pinfo->coeff_p = RC_PID_COEFF_P;
 	pinfo->coeff_i = RC_PID_COEFF_I;
 	pinfo->coeff_d = RC_PID_COEFF_D;
+	pinfo->rinfo = rinfo;
+	pinfo->oldrate = 0;
 
 	return pinfo;
 }
@@ -330,6 +485,7 @@ static void *rate_control_pid_alloc(stru
 static void rate_control_pid_free(void *priv)
 {
 	struct rc_pid_info *pinfo = priv;
+	kfree(pinfo->rinfo);
 	kfree(pinfo);
 }
 


-- 
Ciao
Stefano

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Mattias Nissler]

On Mon, 2007-12-10 at 00:21 +0100, Stefano Brivio wrote:
> On Sun, 09 Dec 2007 23:25:50 +0100
> Mattias Nissler <mattias.nissler@gmx.de> wrote:
> 
> > > +static int rate_control_pid_shift_adjust(struct rc_pid_rateinfo *r,
> > > +					 int adj, int cur, int l)
> > > +{
> > > +	int i, j, k, tmp;
> > > +
> > > +	if (cur + adj < 0)
> > > +		return 0;
> > > +	if (cur + adj >= l)
> > > +		return l - 1;
> > > +
> > > +	for (i = 0; i < l; i++)
> > > +		if (r[i].index == cur + adj)
> > > +			break;
> > > +	if (unlikely(!r[i].valid))
> > > +		return cur + adj;
> > > +	for (j = 0; j < l; j++)
> > > +		if (r[j].index == cur)
> > > +			break;
> > > +
> > > +	if (adj < 0) {
> > > +		if (r[i].diff > r[j].diff)
> > > +			return cur;
> > 
> > This prevents switching down if the rate we're switching to is worse
> > than the current. But if |adj| > 1, we could still find an acceptable
> > rate in between i and j, couldn't we?
> 
> Right. I even thought about this, but then forgot to implement it. I'll fix
> it.
>  
> > > +	} else {
> > > +		tmp = r[i].diff;
> > > +		for (k = i + 1; k + cur < l; k++)
> > > +			if (r[k].valid && r[k].diff <= tmp) {
> > > +				tmp = r[k].diff;
> > > +				adj = k;
> > > +			}
> > 
> > This increases the rate even more if following rates are better than the
> > one actually selected by the PID controller.
> 
> This is intentional (see fast_start, e.g.).
> 
> > I'm not too happy with
> > this, because it defeats the idea that the PID controller can decide how
> > far it wants to switch by the absolute value of adj.
> 
> Please remember that normalization is periodically performed. So, this will
> happen only if we noticed that in very recent events that it doesn't make
> sense not to switch to an higher rate. We can tune this behaviour with the
> norm_factor parameter. I tested this briefly and turned out to yield some
> performance advantage in most cases (norm_factor set to 3 seems to be
> optimal).
> 
> > > +/* Normalize the failed frames per-rate differences. */
> > > +static void rate_control_pid_normalize(struct rc_pid_rateinfo *r, int l)
> > > +{
> > > +	int i;
> > > +
> > > +	if (r[0].diff > RC_PID_NORM_FACTOR)
> > > +		r[0].diff -= RC_PID_NORM_FACTOR;
> > > +	else if (r[0].diff < -RC_PID_NORM_FACTOR)
> > > +		r[0].diff += RC_PID_NORM_FACTOR;
> > > +	for (i = 0; i < l - 1; i++)
> > > +		if (likely(r[i + 1].valid)) {
> > > +			if (r[i + 1].diff > r[i].diff + RC_PID_NORM_FACTOR)
> > > +				r[i + 1].diff -= RC_PID_NORM_FACTOR;
> > > +			else if (r[i + 1].diff <= r[i].diff)
> > > +				r[i + 1].diff += RC_PID_NORM_FACTOR;
> > > +		}
> > 
> > If I understand correctly, you apply an offset to all valid entries. So
> > I'd rather call it RC_PID_NORM_OFFSET.
> 
> Well, I don't care that much. I was referring to this definition of factor:
> 
> n., 1. One that actively contributes to an accomplishment, result, or process
> 
> Anyway "offset" may be more clear. Will fix it.
> 
> > Furthermore, what if the diff
> > values are larger than k * RC_PID_NORM_OFFSET (with k an integer)? If
> > you first compute the offset, you can just loop over the array and apply
> > the offset without checking for the condition.
> 
> I frankly don't get your point here. No offsets are computed...

Well, let's see, maybe I misunderstood the code. I thought the idea was:
If the diff values grow too large (too small), we substract (add)
RC_PID_NORM_FACTOR from (to) all of them. But now it seems to me you do
this for every diff value individually to achieve something like an
aging effect. Is that correct? The term "normalize" probably confused
me.

> 
> > > +}
> > > +
> > >  static void rate_control_pid_sample(struct rc_pid_info *pinfo,
> > >  				    struct ieee80211_local *local,
> > >  				    struct sta_info *sta)
> > >  {
> > >  	struct rc_pid_sta_info *spinfo = sta->rate_ctrl_priv;
> > > +	struct rc_pid_rateinfo *rinfo = pinfo->rinfo;
> > > +	struct ieee80211_hw_mode *mode;
> > >  	u32 pf;
> > >  	s32 err_avg;
> > >  	s32 err_prop;
> > >  	s32 err_int;
> > >  	s32 err_der;
> > > -	int adj;
> > > +	int adj, i, j, tmp;
> > >  
> > > +	mode = local->oper_hw_mode;
> > >  	spinfo = sta->rate_ctrl_priv;
> > >  	spinfo->last_sample = jiffies;
> > >  
> > > @@ -194,6 +282,31 @@ static void rate_control_pid_sample(stru
> > >  		spinfo->tx_num_failed = 0;
> > >  	}
> > >  
> > > +	/* If we just switched rate, update the rate behaviour info. */
> > > +	if (pinfo->oldrate != sta->txrate) {
> > > +		for (i = 0; i < mode->num_rates; i++)
> > > +			if (rinfo[i].index == pinfo->oldrate)
> > > +				break;
> > > +		for (j = 0; j < mode->num_rates; j++)
> > > +			if (rinfo[j].index == sta->txrate) {
> > > +				rinfo[j].valid = 1;
> > > +				break;
> > > +			}
> > 
> > This is no 3 and 4 of the call sites to the to be written
> > find_index_for_rate() function ;-)
> >
> > Or what about this idea: Store direct indices into the rinfo table, so
> > you needn't convert them back from the mode->rates indexing everywhere.
> 
> This makes sense. Unless you are working to the find_index_for_rate()
> function, which would make more sense.
> 
> > > -	/* We need to do an arithmetic right shift. ISO C says this is
> > > -	 * implementation defined for negative left operands. Hence, be
> > > -	 * careful to get it right, also for negative values. */
> > > +	/* Arithmetic right shift, see above. */
> > >  	adj = (adj < 0) ? -((-adj) >> (2 * RC_PID_ARITH_SHIFT)) :
> > >  			  adj >> (2 * RC_PID_ARITH_SHIFT);
> > 
> > If we use it twice, IMHO it's worth factoring it into a macro.
> 
> Will do.
> 
> > >  static void *rate_control_pid_alloc(struct ieee80211_local *local)
> > >  {
> > >  	struct rc_pid_info *pinfo;
> > > +	struct rc_pid_rateinfo *rinfo;
> > > +	struct ieee80211_hw_mode *mode;
> > > +	int i, j, tmp;
> > > +	bool s;
> > >  
> > >  	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
> > > +	if (!pinfo)
> > > +		return NULL;
> > > +
> > > +	mode = local->oper_hw_mode;
> > > +	rinfo = kmalloc(sizeof(*rinfo) * mode->num_rates, GFP_ATOMIC);
> > > +	if (!rinfo) {
> > > +		kfree(pinfo);
> > > +		return NULL;
> > > +	}
> > 
> > What if the mode is changed? Have you checked the rate control algorithm
> > gets reinitialized? If not, we're scheduling a crash here, when
> > mode->num_rates changes.
> 
> No, I didn't because I thought that was obvious. But no, it seems that it
> doesn't get reinitialized. This has to be addressed, IMHO.

Yes. Maybe we can use rate_control_clear() here (haven't checked the
code, but I can remember having seen such a function).

> 
> > > +
> > > +	/* Sort the rates. This is optimized for the most common case (i.e.
> > > +	 * almost-sorted CCK+OFDM rates). */
> > > +	for (i = 0; i < mode->num_rates; i++) {
> > > +		rinfo[i].index = i;
> > > +		if (RC_PID_FAST_START) {
> > > +			rinfo[i].valid = 1;
> > > +			rinfo[i].diff = 0;
> > > +		} else
> > > +			rinfo[i].valid = 0;
> > 
> > Maybe use an #ifdef RC_PID_FAST_START_HERE?
> 
> So that it can't be turned into a parameter? Nah.

Yes, you're right.

> 
> > > +	}
> > > +	for (i = 1; i < mode->num_rates; i++) {
> > > +		s = 0;
> > > +		for (j = 0; j < mode->num_rates - i; j++)
> > > +			if (unlikely(mode->rates[rinfo[j].index].rate >
> > > +				     mode->rates[rinfo[j + 1].index].rate)) {
> > > +				tmp = rinfo[j].index;
> > > +				rinfo[j].index = rinfo[j + 1].index;
> > > +				rinfo[j + 1].index = tmp;
> > > +				s = 1;
> > > +			}
> > > +		if (!s)
> > > +			break;
> > > +	}
> > 
> > There is include/linux/sort.h. I guess using this would make the code
> > clearer. It took me a moment to recognize the code as bubble sort.
> 
> Heapsort (please see lib/sort.c) is suboptimal here. I think that
> bubblesort is the right compromise between insertion sort (performance) and
> readability here.

This is one of the places where you don't have performance issues at
all. This only called once when initializing the rate control algorithm,
so you will never need to optimize this code :-)

If you don't want the sort() call, please at least add a comment that
you're doing a bubble sort here, just to aid people reading this.

> 
> > What just came to my mind: Maybe it's not a good idea to actually break
> > up the 80211.b then 80211.g sorting of the rates: If you have an AP,
> > it'll compute separate tx rates for all stations it knows. But there
> > might also be 80211.b only stations. If we have 6Mbit/s and 9Mbit/s OFDM
> > rates before the 11Mbit/s OFDM rate, will we ever be able to switch to
> > 11Mbit/s?
> 
> (11Mbit/s is a CCK rate)

Sure, this is what I had in mind :-)

> 
> We should be able to switch to 11Mbit/s:
> [in rate_control_pid_adjust_rate()]:
> 
>         while (newidx != sta->txrate) {
>                 if (rate_supported(sta, mode, newidx) &&
>                     (maxrate < 0 || newidx <= maxrate)) {
>                         sta->txrate = newidx;
>                         break;
>                 }
> 
>                 newidx += back;
>         }

I know about this, and you're right, we'll probably make it to 11Mbit/s.
However, the issue here is quite subtle: Currently, we don't know
whether the frames that we receive status about in tx_status() were
actually sent with the rate as decided by the algorithm. This is
something I have on my list, but I currently don't know how to address
it properly. Comments welcome.

> 
> > Overall, I'm still not too convinced whether this learning approach is
> > worth the effort, but this is something to be figured out by testing.
> 
> Well, I didn't test this extensively. But at least I got rid of the
> 11-6-9-11M loop and this yields best performance (~1.5mbit/s more with
> iperf) for scenarios with good signal and varying noise (tested with b43).
> I have to fix a bug in b43 before further testing, though.
> 
> 
> --
> Ciao
> Stefano

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Stefano Brivio]

On Mon, 10 Dec 2007 07:48:56 +0100
Mattias Nissler <mattias.nissler@gmx.de> wrote:

> > > > +/* Normalize the failed frames per-rate differences. */
> > > > +static void rate_control_pid_normalize(struct rc_pid_rateinfo *r, int l)
> > > > +{
> > > > +	int i;
> > > > +
> > > > +	if (r[0].diff > RC_PID_NORM_FACTOR)
> > > > +		r[0].diff -= RC_PID_NORM_FACTOR;
> > > > +	else if (r[0].diff < -RC_PID_NORM_FACTOR)
> > > > +		r[0].diff += RC_PID_NORM_FACTOR;
> > > > +	for (i = 0; i < l - 1; i++)
> > > > +		if (likely(r[i + 1].valid)) {
> > > > +			if (r[i + 1].diff > r[i].diff + RC_PID_NORM_FACTOR)
> > > > +				r[i + 1].diff -= RC_PID_NORM_FACTOR;
> > > > +			else if (r[i + 1].diff <= r[i].diff)
> > > > +				r[i + 1].diff += RC_PID_NORM_FACTOR;
> > > > +		}
> > > 
> > > If I understand correctly, you apply an offset to all valid entries. So
> > > I'd rather call it RC_PID_NORM_OFFSET.
> > 
> > Well, I don't care that much. I was referring to this definition of factor:
> > 
> > n., 1. One that actively contributes to an accomplishment, result, or process
> > 
> > Anyway "offset" may be more clear. Will fix it.
> > 
> > > Furthermore, what if the diff
> > > values are larger than k * RC_PID_NORM_OFFSET (with k an integer)? If
> > > you first compute the offset, you can just loop over the array and apply
> > > the offset without checking for the condition.
> > 
> > I frankly don't get your point here. No offsets are computed...
> 
> Well, let's see, maybe I misunderstood the code. I thought the idea was:
> If the diff values grow too large (too small), we substract (add)
> RC_PID_NORM_FACTOR from (to) all of them. But now it seems to me you do
> this for every diff value individually to achieve something like an
> aging effect. Is that correct? The term "normalize" probably confused
> me.

I used the "normalize" term as in "process that makes something more
normal, which typically means conforming to some regularity or rule, or
returning from some state of abnormality" [wikipedia].

What I do here is to assume that lower rates will perform better, in the
long term, let's make an example: my card can transmit at three rates, 1,
2, and 3M. But it performs really bad at 3M, so what I can expect to have
is:

[rate] [diff]
  1      0
  2      10
  3      105
[this means we have used rate 1, then switched to rate 2 and had 10% more
fails, then switched to rate 3 and had 95% more fails than rate 2]

But now we don't know if bad performance of rate 3 was due to external
conditions or to a somehow broken device. Let's say that we move our
laptop to a different environment, which causes the rate 3 to work
reasonably well. That 105 doesn't make sense now and we have to make it
similar to diff for rate 2. But not equal, as 1) it's unlikely to be equal;
2) if it would be equal we would never use rate 2, which we can
theoretically suppose to be a little better than rate 3 (compare with
_shift_adjust() here). So in the end, if we don't get any recent events,
we should end up with something like:

[rate] [diff]
  1      0
  2      3
  3      6
[with norm_offset set to 3]

So what really matters here are recent events. Yes, it's something like an
aging effect. Note that norm_offset is meant to tune how fast it's supposed
that we move between different environments. A little caveat here: all
values can be negative, even the one for rate 1. This doesn't matter,
because anyway, as per the definition of the algorithm, all values are
referred to the lowest rate behaviour (and not to zero).

> > > > +	for (i = 1; i < mode->num_rates; i++) {
> > > > +		s = 0;
> > > > +		for (j = 0; j < mode->num_rates - i; j++)
> > > > +			if (unlikely(mode->rates[rinfo[j].index].rate >
> > > > +				     mode->rates[rinfo[j + 1].index].rate)) {
> > > > +				tmp = rinfo[j].index;
> > > > +				rinfo[j].index = rinfo[j + 1].index;
> > > > +				rinfo[j + 1].index = tmp;
> > > > +				s = 1;
> > > > +			}
> > > > +		if (!s)
> > > > +			break;
> > > > +	}
> > > 
> > > There is include/linux/sort.h. I guess using this would make the code
> > > clearer. It took me a moment to recognize the code as bubble sort.
> > 
> > Heapsort (please see lib/sort.c) is suboptimal here. I think that
> > bubblesort is the right compromise between insertion sort (performance) and
> > readability here.
> 
> This is one of the places where you don't have performance issues at
> all. This only called once when initializing the rate control algorithm,
> so you will never need to optimize this code :-)
> 
> If you don't want the sort() call, please at least add a comment that
> you're doing a bubble sort here, just to aid people reading this.

Ok, I'll pick one. :)

> However, the issue here is quite subtle: Currently, we don't know
> whether the frames that we receive status about in tx_status() were
> actually sent with the rate as decided by the algorithm. This is
> something I have on my list, but I currently don't know how to address
> it properly. Comments welcome.

Could you elaborate here? I miss the point (i.e. what's the problem here).


--
Ciao
Stefano

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Mattias Nissler]

On Mon, 2007-12-10 at 09:03 +0100, Stefano Brivio wrote:
> On Mon, 10 Dec 2007 07:48:56 +0100
> Mattias Nissler <mattias.nissler@gmx.de> wrote:
> 
> > > > > +/* Normalize the failed frames per-rate differences. */
> > > > > +static void rate_control_pid_normalize(struct rc_pid_rateinfo *r, int l)
> > > > > +{
> > > > > +	int i;
> > > > > +
> > > > > +	if (r[0].diff > RC_PID_NORM_FACTOR)
> > > > > +		r[0].diff -= RC_PID_NORM_FACTOR;
> > > > > +	else if (r[0].diff < -RC_PID_NORM_FACTOR)
> > > > > +		r[0].diff += RC_PID_NORM_FACTOR;
> > > > > +	for (i = 0; i < l - 1; i++)
> > > > > +		if (likely(r[i + 1].valid)) {
> > > > > +			if (r[i + 1].diff > r[i].diff + RC_PID_NORM_FACTOR)
> > > > > +				r[i + 1].diff -= RC_PID_NORM_FACTOR;
> > > > > +			else if (r[i + 1].diff <= r[i].diff)
> > > > > +				r[i + 1].diff += RC_PID_NORM_FACTOR;
> > > > > +		}
> > > > 
> > > > If I understand correctly, you apply an offset to all valid entries. So
> > > > I'd rather call it RC_PID_NORM_OFFSET.
> > > 
> > > Well, I don't care that much. I was referring to this definition of factor:
> > > 
> > > n., 1. One that actively contributes to an accomplishment, result, or process
> > > 
> > > Anyway "offset" may be more clear. Will fix it.
> > > 
> > > > Furthermore, what if the diff
> > > > values are larger than k * RC_PID_NORM_OFFSET (with k an integer)? If
> > > > you first compute the offset, you can just loop over the array and apply
> > > > the offset without checking for the condition.
> > > 
> > > I frankly don't get your point here. No offsets are computed...
> > 
> > Well, let's see, maybe I misunderstood the code. I thought the idea was:
> > If the diff values grow too large (too small), we substract (add)
> > RC_PID_NORM_FACTOR from (to) all of them. But now it seems to me you do
> > this for every diff value individually to achieve something like an
> > aging effect. Is that correct? The term "normalize" probably confused
> > me.
> 
> I used the "normalize" term as in "process that makes something more
> normal, which typically means conforming to some regularity or rule, or
> returning from some state of abnormality" [wikipedia].
> 
> What I do here is to assume that lower rates will perform better, in the
> long term, let's make an example: my card can transmit at three rates, 1,
> 2, and 3M. But it performs really bad at 3M, so what I can expect to have
> is:
> 
> [rate] [diff]
>   1      0
>   2      10
>   3      105
> [this means we have used rate 1, then switched to rate 2 and had 10% more
> fails, then switched to rate 3 and had 95% more fails than rate 2]
> 
> But now we don't know if bad performance of rate 3 was due to external
> conditions or to a somehow broken device. Let's say that we move our
> laptop to a different environment, which causes the rate 3 to work
> reasonably well. That 105 doesn't make sense now and we have to make it
> similar to diff for rate 2. But not equal, as 1) it's unlikely to be equal;
> 2) if it would be equal we would never use rate 2, which we can
> theoretically suppose to be a little better than rate 3 (compare with
> _shift_adjust() here). So in the end, if we don't get any recent events,
> we should end up with something like:
> 
> [rate] [diff]
>   1      0
>   2      3
>   3      6
> [with norm_offset set to 3]
> 
> So what really matters here are recent events. Yes, it's something like an
> aging effect. Note that norm_offset is meant to tune how fast it's supposed
> that we move between different environments. A little caveat here: all
> values can be negative, even the one for rate 1. This doesn't matter,
> because anyway, as per the definition of the algorithm, all values are
> referred to the lowest rate behaviour (and not to zero).

Ok, understood now :-)

Mattias

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Mattias Nissler]

On Mon, 2007-12-10 at 09:03 +0100, Stefano Brivio wrote:
> > However, the issue here is quite subtle: Currently, we don't know
> > whether the frames that we receive status about in tx_status() were
> > actually sent with the rate as decided by the algorithm. This is
> > something I have on my list, but I currently don't know how to address
> > it properly. Comments welcome.
> 
> Could you elaborate here? I miss the point (i.e. what's the problem here).

Sometimes, the stack sends frames at different rates than what was
decided by the rate control algorithm (there are several situations in
which this can happen, e.g. an AP only allowing 802.11b rates, rts/cts
frames, maybe more). But still, the tx status is reported back to the
rate control algorithm as for normal frames. Now the rate control
algorithm just doesn't care and accounts the tx status to the wrong
rate. This is clearly suboptimal. I cannot estimate how much impact this
behaviour has. However, it shouldn't be hard to improve the situation
either by reporting back to the rate control algorithm on which rate the
frame handed to tx_status() was actually transmitted, so it can decide
itself what to do about this (this is my preferred solution). Or you
could just have the stack don't call tx_status() for frames that were
transmitted on another rate.

If you like to work on this, go ahead, I'm busy with my graphing stuff.

Mattias

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Stefano Brivio]

On Mon, 10 Dec 2007 21:56:32 +0100
Mattias Nissler <mattias.nissler@gmx.de> wrote:

> Sometimes, the stack sends frames at different rates than what was
> decided by the rate control algorithm (there are several situations in
> which this can happen, e.g. an AP only allowing 802.11b rates, rts/cts

No, wait, to consider rts/cts frames makes sense here, but I'd say that the
same doesn't apply to AP only allowing 802.11b rates, because anyway
non-CCK rates would get excluded from supported rates, and we wouldn't even
map them.

> frames, maybe more). But still, the tx status is reported back to the
> rate control algorithm as for normal frames. Now the rate control
> algorithm just doesn't care and accounts the tx status to the wrong
> rate. This is clearly suboptimal. I cannot estimate how much impact this
> behaviour has. However, it shouldn't be hard to improve the situation
> either by reporting back to the rate control algorithm on which rate the
> frame handed to tx_status() was actually transmitted, so it can decide
> itself what to do about this (this is my preferred solution). Or you
> could just have the stack don't call tx_status() for frames that were
> transmitted on another rate.

Ok, got it. But I would just discard them, I can't think of any
significant measurement on those frames. So I would follow the second
approach here. Or do you have any suggestions on how to consider those
frames?


--
Ciao
Stefano

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Mattias Nissler]

On Mon, 2007-12-10 at 22:30 +0100, Stefano Brivio wrote:
> On Mon, 10 Dec 2007 21:56:32 +0100
> Mattias Nissler <mattias.nissler@gmx.de> wrote:
> 
> > Sometimes, the stack sends frames at different rates than what was
> > decided by the rate control algorithm (there are several situations in
> > which this can happen, e.g. an AP only allowing 802.11b rates, rts/cts
> 
> No, wait, to consider rts/cts frames makes sense here, but I'd say that the
> same doesn't apply to AP only allowing 802.11b rates, because anyway
> non-CCK rates would get excluded from supported rates, and we wouldn't even
> map them.

Actually I meant we are an AP and decide we need to send b-only rates.

> 
> > frames, maybe more). But still, the tx status is reported back to the
> > rate control algorithm as for normal frames. Now the rate control
> > algorithm just doesn't care and accounts the tx status to the wrong
> > rate. This is clearly suboptimal. I cannot estimate how much impact this
> > behaviour has. However, it shouldn't be hard to improve the situation
> > either by reporting back to the rate control algorithm on which rate the
> > frame handed to tx_status() was actually transmitted, so it can decide
> > itself what to do about this (this is my preferred solution). Or you
> > could just have the stack don't call tx_status() for frames that were
> > transmitted on another rate.
> 
> Ok, got it. But I would just discard them, I can't think of any
> significant measurement on those frames. So I would follow the second
> approach here. Or do you have any suggestions on how to consider those
> frames?

As this fix is concerned with the rate control algorithm in general, we
should also consider other possible rate control algorithms. And I don't
think it's too far-fetched that this information might be valuable in
some cases. You are right, the PID algorithm should probably discard
them. But there is a gotcha: Suppose again, we're an AP and the stack
decides to transmit on b-only rates. If we just discard frames sent on
other rates, we'll soon be stuck on an OFDM rate and cannot switch back
since we don't have any measurements. I guess this whole issue needs
some more thought. Ideas welcome :-)

Mattias

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Stefano Brivio]

On Sun, 09 Dec 2007 23:25:50 +0100
Mattias Nissler <mattias.nissler@gmx.de> wrote:

> >  static void *rate_control_pid_alloc(struct ieee80211_local *local)
> >  {
> >  	struct rc_pid_info *pinfo;
> > +	struct rc_pid_rateinfo *rinfo;
> > +	struct ieee80211_hw_mode *mode;
> > +	int i, j, tmp;
> > +	bool s;
> >  
> >  	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
> > +	if (!pinfo)
> > +		return NULL;
> > +
> > +	mode = local->oper_hw_mode;
> > +	rinfo = kmalloc(sizeof(*rinfo) * mode->num_rates, GFP_ATOMIC);
> > +	if (!rinfo) {
> > +		kfree(pinfo);
> > +		return NULL;
> > +	}
> 
> What if the mode is changed? Have you checked the rate control algorithm
> gets reinitialized? If not, we're scheduling a crash here, when
> mode->num_rates changes.

After a discussion on IRC with Michael (Wu), we came to the conclusion that
it doesn't make sense for mode->num_rates to change, because:
1) if the AP drops supported rates, it'll drop the association as well,
then everything here will be destroyed and created again;
2) that can be changed in userspace, but we couldn't figure out a scenario
where it would make any sense. Johannes, any comments? Wouldn't it make
sense to just forbid to change this in userspace?


--
Ciao
Stefano

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Mattias Nissler]

On Mon, 2007-12-10 at 09:08 +0100, Stefano Brivio wrote:
> On Sun, 09 Dec 2007 23:25:50 +0100
> Mattias Nissler <mattias.nissler@gmx.de> wrote:
> 
> > >  static void *rate_control_pid_alloc(struct ieee80211_local *local)
> > >  {
> > >  	struct rc_pid_info *pinfo;
> > > +	struct rc_pid_rateinfo *rinfo;
> > > +	struct ieee80211_hw_mode *mode;
> > > +	int i, j, tmp;
> > > +	bool s;
> > >  
> > >  	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
> > > +	if (!pinfo)
> > > +		return NULL;
> > > +
> > > +	mode = local->oper_hw_mode;
> > > +	rinfo = kmalloc(sizeof(*rinfo) * mode->num_rates, GFP_ATOMIC);
> > > +	if (!rinfo) {
> > > +		kfree(pinfo);
> > > +		return NULL;
> > > +	}
> > 
> > What if the mode is changed? Have you checked the rate control algorithm
> > gets reinitialized? If not, we're scheduling a crash here, when
> > mode->num_rates changes.
> 
> After a discussion on IRC with Michael (Wu), we came to the conclusion that
> it doesn't make sense for mode->num_rates to change, because:
> 1) if the AP drops supported rates, it'll drop the association as well,
> then everything here will be destroyed and created again;
> 2) that can be changed in userspace, but we couldn't figure out a scenario
> where it would make any sense. Johannes, any comments? Wouldn't it make
> sense to just forbid to change this in userspace?

I don't agree. For example, what if you have some broken 802.11b only
hardware that you desperately need to get going, but it freaks out on
802.11g encoded frames. Now if your AP is hostapd on a Linux machine,
you'll want to change the mode. Hence, also the number of available
rates change.

Moreover, I think we can do better than just disallowing changes to the
rate set, don't you think?

Mattias

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Stefano Brivio]

On Mon, 10 Dec 2007 21:51:07 +0100
Mattias Nissler <mattias.nissler@gmx.de> wrote:

> 
> On Mon, 2007-12-10 at 09:08 +0100, Stefano Brivio wrote:
> > On Sun, 09 Dec 2007 23:25:50 +0100
> > Mattias Nissler <mattias.nissler@gmx.de> wrote:
> > 
> > > >  static void *rate_control_pid_alloc(struct ieee80211_local *local)
> > > >  {
> > > >  	struct rc_pid_info *pinfo;
> > > > +	struct rc_pid_rateinfo *rinfo;
> > > > +	struct ieee80211_hw_mode *mode;
> > > > +	int i, j, tmp;
> > > > +	bool s;
> > > >  
> > > >  	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
> > > > +	if (!pinfo)
> > > > +		return NULL;
> > > > +
> > > > +	mode = local->oper_hw_mode;
> > > > +	rinfo = kmalloc(sizeof(*rinfo) * mode->num_rates, GFP_ATOMIC);
> > > > +	if (!rinfo) {
> > > > +		kfree(pinfo);
> > > > +		return NULL;
> > > > +	}
> > > 
> > > What if the mode is changed? Have you checked the rate control algorithm
> > > gets reinitialized? If not, we're scheduling a crash here, when
> > > mode->num_rates changes.
> > 
> > After a discussion on IRC with Michael (Wu), we came to the conclusion that
> > it doesn't make sense for mode->num_rates to change, because:
> > 1) if the AP drops supported rates, it'll drop the association as well,
> > then everything here will be destroyed and created again;
> > 2) that can be changed in userspace, but we couldn't figure out a scenario
> > where it would make any sense. Johannes, any comments? Wouldn't it make
> > sense to just forbid to change this in userspace?
> 
> I don't agree. For example, what if you have some broken 802.11b only
> hardware that you desperately need to get going, but it freaks out on
> 802.11g encoded frames. Now if your AP is hostapd on a Linux machine,
> you'll want to change the mode. Hence, also the number of available
> rates change.

Yes, but the association gets dropped. I'm not sure about the hostapd
implementation (will check), but APs drop association when they change
supported rates. So the per-sta rate control would obviously get reinitialized.
I didn't find anything clear about this in the 802.11 standard, though.

> Moreover, I think we can do better than just disallowing changes to the
> rate set, don't you think?

Well, I still can't find an example of where this would be needed. Your
scenario looks like this:
1) the AP STA periodically advertises supported rates;
2) the non-AP STA supported rates are the intersection (as in set theory)
of the rates supported by the non-AP STA and the rate advertised by the AP
STA (in this case, assuming that the both the non-AP and the AP STAs
support 802.11b and g, rates supported by the non-AP STA are both CCK and
OFDM rates);
3) let's say that a thunderstorm makes the air CCK-hostile, so we want to
reconfigure our AP STA to work with 802.11b only: the association will need
to be recreated (thus the rc algorithm gets reinitialized), and now rates
supported by the non-AP STA are the CCK rates only -> everything works as
expected and wanted.

Anyway, it's not an issue at all to deal with rates changing in
rc80211-pid - I would just need to reallocate a struct and copy over some
data (but please think about this now: OFDM rates are the same for 802.11a
and 802.11g, but it really doesn't make any sense to assume that behaviour
at 2.4GHz is anywhere near to behaviour at 5GHz - so maybe I would just
reallocate the struct and that's it). But before than doing this, I wanted
to be sure that we aren't just hiding a bug in mac80211.


--
Ciao
Stefano

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[st3]

On Mon, 10 Dec 2007 22:22:13 +0100
Stefano Brivio <stefano.brivio@polimi.it> wrote:

> 3) let's say that a thunderstorm makes the air CCK-hostile, so we want to
						 ^^^ OFDM


-- 
Ciao
Stefano

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Mattias Nissler]

On Mon, 2007-12-10 at 22:22 +0100, Stefano Brivio wrote:
> On Mon, 10 Dec 2007 21:51:07 +0100
> Mattias Nissler <mattias.nissler@gmx.de> wrote:
> 
> > 
> > On Mon, 2007-12-10 at 09:08 +0100, Stefano Brivio wrote:
> > > On Sun, 09 Dec 2007 23:25:50 +0100
> > > Mattias Nissler <mattias.nissler@gmx.de> wrote:
> > > 
> > > > >  static void *rate_control_pid_alloc(struct ieee80211_local *local)
> > > > >  {
> > > > >  	struct rc_pid_info *pinfo;
> > > > > +	struct rc_pid_rateinfo *rinfo;
> > > > > +	struct ieee80211_hw_mode *mode;
> > > > > +	int i, j, tmp;
> > > > > +	bool s;
> > > > >  
> > > > >  	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
> > > > > +	if (!pinfo)
> > > > > +		return NULL;
> > > > > +
> > > > > +	mode = local->oper_hw_mode;
> > > > > +	rinfo = kmalloc(sizeof(*rinfo) * mode->num_rates, GFP_ATOMIC);
> > > > > +	if (!rinfo) {
> > > > > +		kfree(pinfo);
> > > > > +		return NULL;
> > > > > +	}
> > > > 
> > > > What if the mode is changed? Have you checked the rate control algorithm
> > > > gets reinitialized? If not, we're scheduling a crash here, when
> > > > mode->num_rates changes.
> > > 
> > > After a discussion on IRC with Michael (Wu), we came to the conclusion that
> > > it doesn't make sense for mode->num_rates to change, because:
> > > 1) if the AP drops supported rates, it'll drop the association as well,
> > > then everything here will be destroyed and created again;
> > > 2) that can be changed in userspace, but we couldn't figure out a scenario
> > > where it would make any sense. Johannes, any comments? Wouldn't it make
> > > sense to just forbid to change this in userspace?
> > 
> > I don't agree. For example, what if you have some broken 802.11b only
> > hardware that you desperately need to get going, but it freaks out on
> > 802.11g encoded frames. Now if your AP is hostapd on a Linux machine,
> > you'll want to change the mode. Hence, also the number of available
> > rates change.
> 
> Yes, but the association gets dropped. I'm not sure about the hostapd
> implementation (will check), but APs drop association when they change
> supported rates. So the per-sta rate control would obviously get reinitialized.
> I didn't find anything clear about this in the 802.11 standard, though.

Still, just disallowing mode switches seems odd to me.

> 
> > Moreover, I think we can do better than just disallowing changes to the
> > rate set, don't you think?
> 
> Well, I still can't find an example of where this would be needed. Your
> scenario looks like this:
> 1) the AP STA periodically advertises supported rates;
> 2) the non-AP STA supported rates are the intersection (as in set theory)
> of the rates supported by the non-AP STA and the rate advertised by the AP
> STA (in this case, assuming that the both the non-AP and the AP STAs
> support 802.11b and g, rates supported by the non-AP STA are both CCK and
> OFDM rates);
> 3) let's say that a thunderstorm makes the air CCK-hostile, so we want to
> reconfigure our AP STA to work with 802.11b only: the association will need
> to be recreated (thus the rc algorithm gets reinitialized), and now rates
> supported by the non-AP STA are the CCK rates only -> everything works as
> expected and wanted.
> 
> Anyway, it's not an issue at all to deal with rates changing in
> rc80211-pid - I would just need to reallocate a struct and copy over some
> data (but please think about this now: OFDM rates are the same for 802.11a
> and 802.11g, but it really doesn't make any sense to assume that behaviour
> at 2.4GHz is anywhere near to behaviour at 5GHz - so maybe I would just
> reallocate the struct and that's it). But before than doing this, I wanted
> to be sure that we aren't just hiding a bug in mac80211.

You're right, we should find out what behaviour would be correct for the
stack to implement. Who make a more educated statement? Johannes?
Michael?

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Johannes Berg]

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


> > 2) that can be changed in userspace, but we couldn't figure out a scenario
> > where it would make any sense. Johannes, any comments? Wouldn't it make
> > sense to just forbid to change this in userspace?
> 
> I don't agree. For example, what if you have some broken 802.11b only
> hardware that you desperately need to get going, but it freaks out on
> 802.11g encoded frames. Now if your AP is hostapd on a Linux machine,
> you'll want to change the mode. Hence, also the number of available
> rates change.
> 
> Moreover, I think we can do better than just disallowing changes to the
> rate set, don't you think?

All of this is going to change anyway, but disallowing mode/rateset
changes while associated is definitely the right thing to do.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Mattias Nissler]

On Tue, 2007-12-11 at 15:52 +0100, Johannes Berg wrote:
> > > 2) that can be changed in userspace, but we couldn't figure out a scenario
> > > where it would make any sense. Johannes, any comments? Wouldn't it make
> > > sense to just forbid to change this in userspace?
> > 
> > I don't agree. For example, what if you have some broken 802.11b only
> > hardware that you desperately need to get going, but it freaks out on
> > 802.11g encoded frames. Now if your AP is hostapd on a Linux machine,
> > you'll want to change the mode. Hence, also the number of available
> > rates change.
> > 
> > Moreover, I think we can do better than just disallowing changes to the
> > rate set, don't you think?
> 
> All of this is going to change anyway, but disallowing mode/rateset
> changes while associated is definitely the right thing to do.

Just to sum up the issue for Johannes: Stefano's patch adds per-rate
information to the PID algorithm state. This is fine, however we need to
make sure the rate control algorithm gets reinitialized when the number
of rates changes (i.e. due to a mode change, what about about regulatory
domain changes?), as otherwise we might be indexing array entries we
don't have allocated. Now this spawned the more general discussion about
when the stack should allow changing modes etc.

So let's go back to the original issue: Johannes, can we assume rate
control always gets reinitialized when the number of rates or even the
hardware mode is changed?

Mattias

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Johannes Berg]

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


> Just to sum up the issue for Johannes: Stefano's patch adds per-rate
> information to the PID algorithm state. This is fine, however we need to
> make sure the rate control algorithm gets reinitialized when the number
> of rates changes (i.e. due to a mode change, what about about regulatory
> domain changes?), as otherwise we might be indexing array entries we
> don't have allocated. Now this spawned the more general discussion about
> when the stack should allow changing modes etc.

Thanks.

> So let's go back to the original issue: Johannes, can we assume rate
> control always gets reinitialized when the number of rates or even the
> hardware mode is changed?

I have no idea. Doesn't it get initialised when the interface is brought
up, and as such it would because those changes can only be done when
it's down? If we want to be strict and allow changes while it's not
associated but up then I think the answer is a clear no.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Mattias Nissler]

On Wed, 2007-12-12 at 18:13 +0100, Johannes Berg wrote:
> > Just to sum up the issue for Johannes: Stefano's patch adds per-rate
> > information to the PID algorithm state. This is fine, however we need to
> > make sure the rate control algorithm gets reinitialized when the number
> > of rates changes (i.e. due to a mode change, what about about regulatory
> > domain changes?), as otherwise we might be indexing array entries we
> > don't have allocated. Now this spawned the more general discussion about
> > when the stack should allow changing modes etc.
> 
> Thanks.
> 
> > So let's go back to the original issue: Johannes, can we assume rate
> > control always gets reinitialized when the number of rates or even the
> > hardware mode is changed?
> 
> I have no idea. Doesn't it get initialised when the interface is brought
> up, and as such it would because those changes can only be done when
> it's down? If we want to be strict and allow changes while it's not
> associated but up then I think the answer is a clear no.

I'm pretty sure we currently don't support switching modulations via
WEXT. Any ideas on how this is going to be implemented with nl80211?
Should we be able to switch e.g. from 802.11g to 802.11b while the
interface is up?

Anyway, I think we don't need any special handling of this situation at
the moment. The stack needs more nl80211/cfg80211 work anyway, so let's
fix this once we've fixed the stack.

Mattias

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Stefano Brivio]

On Wed, 12 Dec 2007 21:06:26 +0100
Mattias Nissler <mattias.nissler@gmx.de> wrote:

> Anyway, I think we don't need any special handling of this situation at
> the moment. The stack needs more nl80211/cfg80211 work anyway, so let's
> fix this once we've fixed the stack.

Agreed.


--
Ciao
Stefano

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Johannes Berg]

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


> I'm pretty sure we currently don't support switching modulations via
> WEXT. Any ideas on how this is going to be implemented with nl80211?

The way I see it, I'm going to rip out the whole *mode* stuff completely
and drill it down to
 (1) pure frequencies (no channels)
 (2) supported rates (which implies modulations)

> Should we be able to switch e.g. from 802.11g to 802.11b while the
> interface is up?

Probably not.

> Anyway, I think we don't need any special handling of this situation at
> the moment. The stack needs more nl80211/cfg80211 work anyway, so let's
> fix this once we've fixed the stack.

Right.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Jouni Malinen]

On Thu, Dec 13, 2007 at 12:42:23PM +0100, Johannes Berg wrote:

> The way I see it, I'm going to rip out the whole *mode* stuff completely
> and drill it down to
>  (1) pure frequencies (no channels)
>  (2) supported rates (which implies modulations)

Are you sure this handles all cases correctly? There are some very odd
configurations that can be done with IEEE 802.11 and the rates may not
be unique (e.g., 5 and 10 MHz channels may share the same frequency with
20 MHz channel and some of the rate values are identical between these
different modes). There are good reasons for the mode option and I would
rather hope to see it remain unless someone can convince people that the
proposed alternative mechanism handles all existing and expected future
uses in amendments to IEEE 802.11 at least as well as the mechanism
including mode parameter.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Johannes Berg]

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


> Are you sure this handles all cases correctly? There are some very odd
> configurations that can be done with IEEE 802.11 and the rates may not
> be unique (e.g., 5 and 10 MHz channels may share the same frequency with
> 20 MHz channel and some of the rate values are identical between these
> different modes).

I'm not sure why you think that matters. The *supported* bitrates are
mostly orthogonal to the frquency they're used with.

I've just sent out a patch, please do look at it.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Holger Schurig]

> So let's go back to the original issue: Johannes, can we
> assume rate control always gets reinitialized when the number
> of rates or even the hardware mode is changed?

Make sure that at least you reinitialize rate control when you 
re-associate.

Re: [RFC/T][PATCH 1/3] rc80211-pid: introduce rate behaviour learning algorithm

[Johannes Berg]

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


> After a discussion on IRC with Michael (Wu), we came to the conclusion that
> it doesn't make sense for mode->num_rates to change, because:
> 1) if the AP drops supported rates, it'll drop the association as well,
> then everything here will be destroyed and created again;
> 2) that can be changed in userspace, but we couldn't figure out a scenario
> where it would make any sense. Johannes, any comments? Wouldn't it make
> sense to just forbid to change this in userspace?

I think I'm out of the loop. What are you talking about?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[RFC/T][PATCH 2/3] rc80211-pid: introduce PID sharpening factor

[Stefano Brivio]

This patch introduces a PID sharpening factor for faster response on
association and interpolation.


Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>

---

Index: wireless-2.6/net/mac80211/rc80211_pid.c
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid.c
+++ wireless-2.6/net/mac80211/rc80211_pid.c
@@ -23,13 +23,15 @@
  *
  * The controller basically computes the following:
  *
- * adj = CP * err + CI * err_avg + CD * (err - last_err)
+ * adj = CP * err + CI * err_avg + CD * (err - last_err) * (1 + sharpening)
  *
  * where
  * 	adj	adjustment value that is used to switch TX rate (see below)
  * 	err	current error: target vs. current failed frames percentage
  * 	last_err	last error
  * 	err_avg	average (i.e. poor man's integral) of recent errors
+ *	sharpening	non-zero when fast response is needed (i.e. right after
+ *			association or interpolation), heading to zero over time
  * 	CP	Proportional coefficient
  * 	CI	Integral coefficient
  * 	CD	Derivative coefficient
@@ -65,6 +67,11 @@
 #define RC_PID_SMOOTHING_SHIFT 3
 #define RC_PID_SMOOTHING (1 << RC_PID_SMOOTHING_SHIFT)
 
+/* Sharpening factor (used for D part of PID controller) */
+#define RATE_CONTROL_SHARPENING_SHIFT 2
+#define RATE_CONTROL_SHARPENING (1 << RATE_CONTROL_SHARPENING_SHIFT)
+#define RATE_CONTROL_SHARPENING_DURATION 1
+
 /* Fixed point arithmetic shifting amount. */
 #define RC_PID_ARITH_SHIFT 8
 
@@ -127,8 +134,11 @@ struct rc_pid_sta_info {
 	 */
 	s32 err_avg_sc;
 
-	/* Last framed failes percentage sample */
+	/* Last framed failes percentage sample. */
 	u32 last_pf;
+
+	/* Sharpening needed. */
+	u8 sharp_cnt;
 };
 
 /* Algorithm parameters. We keep them on a per-algorithm approach, so they can
@@ -271,10 +281,12 @@ static void rate_control_pid_sample(stru
 	spinfo->last_sample = jiffies;
 
 	/* If no frames were transmitted, we assume the old sample is
-	 * still a good measurement and copy it. */
-	if (spinfo->tx_num_xmit == 0)
+	 * still a good measurement and copy it, and turn the sharpening factor
+	 * on. */
+	if (spinfo->tx_num_xmit == 0) {
 		pf = spinfo->last_pf;
-	else {
+		spinfo->sharp_cnt = RATE_CONTROL_SHARPENING_DURATION;
+	} else {
 		pf = spinfo->tx_num_failed * 100 / spinfo->tx_num_xmit;
 		pf <<= RC_PID_ARITH_SHIFT;
 
@@ -314,8 +326,11 @@ static void rate_control_pid_sample(stru
 	spinfo->err_avg_sc = spinfo->err_avg_sc - err_avg + err_prop;
 	err_int = spinfo->err_avg_sc >> RC_PID_SMOOTHING_SHIFT;
 
-	err_der = pf - spinfo->last_pf;
+	err_der = pf - spinfo->last_pf
+		  * (1 + RATE_CONTROL_SHARPENING * spinfo->sharp_cnt);
 	spinfo->last_pf = pf;
+	if (spinfo->sharp_cnt)
+			spinfo->sharp_cnt--;
 
 	/* Compute the controller output. */
 	adj = (err_prop * pinfo->coeff_p + err_int * pinfo->coeff_i


-- 
Ciao
Stefano

Re: [RFC/T][PATCH 2/3] rc80211-pid: introduce PID sharpening factor

[Mattias Nissler]

On Sun, 2007-12-09 at 21:21 +0100, Stefano Brivio wrote:
> This patch introduces a PID sharpening factor for faster response on
> association and interpolation.
> 
> 
> Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
> 
> ---
> 
> Index: wireless-2.6/net/mac80211/rc80211_pid.c
> ===================================================================
> --- wireless-2.6.orig/net/mac80211/rc80211_pid.c
> +++ wireless-2.6/net/mac80211/rc80211_pid.c
> @@ -23,13 +23,15 @@
>   *
>   * The controller basically computes the following:
>   *
> - * adj = CP * err + CI * err_avg + CD * (err - last_err)
> + * adj = CP * err + CI * err_avg + CD * (err - last_err) * (1 + sharpening)
>   *
>   * where
>   * 	adj	adjustment value that is used to switch TX rate (see below)
>   * 	err	current error: target vs. current failed frames percentage
>   * 	last_err	last error
>   * 	err_avg	average (i.e. poor man's integral) of recent errors
> + *	sharpening	non-zero when fast response is needed (i.e. right after
> + *			association or interpolation), heading to zero over time
>   * 	CP	Proportional coefficient
>   * 	CI	Integral coefficient
>   * 	CD	Derivative coefficient
> @@ -65,6 +67,11 @@
>  #define RC_PID_SMOOTHING_SHIFT 3
>  #define RC_PID_SMOOTHING (1 << RC_PID_SMOOTHING_SHIFT)
>  
> +/* Sharpening factor (used for D part of PID controller) */
> +#define RATE_CONTROL_SHARPENING_SHIFT 2
> +#define RATE_CONTROL_SHARPENING (1 << RATE_CONTROL_SHARPENING_SHIFT)
> +#define RATE_CONTROL_SHARPENING_DURATION 1
> +
>  /* Fixed point arithmetic shifting amount. */
>  #define RC_PID_ARITH_SHIFT 8
>  
> @@ -127,8 +134,11 @@ struct rc_pid_sta_info {
>  	 */
>  	s32 err_avg_sc;
>  
> -	/* Last framed failes percentage sample */
> +	/* Last framed failes percentage sample. */
>  	u32 last_pf;
> +
> +	/* Sharpening needed. */
> +	u8 sharp_cnt;
>  };
>  
>  /* Algorithm parameters. We keep them on a per-algorithm approach, so they can
> @@ -271,10 +281,12 @@ static void rate_control_pid_sample(stru
>  	spinfo->last_sample = jiffies;
>  
>  	/* If no frames were transmitted, we assume the old sample is
> -	 * still a good measurement and copy it. */
> -	if (spinfo->tx_num_xmit == 0)
> +	 * still a good measurement and copy it, and turn the sharpening factor
> +	 * on. */
> +	if (spinfo->tx_num_xmit == 0) {
>  		pf = spinfo->last_pf;
> -	else {
> +		spinfo->sharp_cnt = RATE_CONTROL_SHARPENING_DURATION;
> +	} else {

Note that current rate_control_pid_sample() is only called from
rate_control_pid_tx_status(), which does an tx_num_xmit++ in advance. So
the tx_num_xmit branch should actually never be executed (I kept it only
to guard against any division by zero errors). I guess this makes the
whole patch basically a noop.

>  		pf = spinfo->tx_num_failed * 100 / spinfo->tx_num_xmit;
>  		pf <<= RC_PID_ARITH_SHIFT;
>  
> @@ -314,8 +326,11 @@ static void rate_control_pid_sample(stru
>  	spinfo->err_avg_sc = spinfo->err_avg_sc - err_avg + err_prop;
>  	err_int = spinfo->err_avg_sc >> RC_PID_SMOOTHING_SHIFT;
>  
> -	err_der = pf - spinfo->last_pf;
> +	err_der = pf - spinfo->last_pf
> +		  * (1 + RATE_CONTROL_SHARPENING * spinfo->sharp_cnt);
>  	spinfo->last_pf = pf;
> +	if (spinfo->sharp_cnt)
> +			spinfo->sharp_cnt--;
>  
>  	/* Compute the controller output. */
>  	adj = (err_prop * pinfo->coeff_p + err_int * pinfo->coeff_i
> 
> 

Re: [RFC/T][PATCH 2/3] rc80211-pid: introduce PID sharpening factor

[Stefano Brivio]

On Sun, 09 Dec 2007 23:29:21 +0100
Mattias Nissler <mattias.nissler@gmx.de> wrote:

> Note that current rate_control_pid_sample() is only called from
> rate_control_pid_tx_status(), which does an tx_num_xmit++ in advance. So
> the tx_num_xmit branch should actually never be executed (I kept it only
> to guard against any division by zero errors).

Eek? Removing the misleading comment would have been nice... where does
interpolation occur now? I got a bit confused by this.


--
Ciao
Stefano

Re: [RFC/T][PATCH 2/3] rc80211-pid: introduce PID sharpening factor

[Mattias Nissler]

On Mon, 2007-12-10 at 00:31 +0100, Stefano Brivio wrote:
> On Sun, 09 Dec 2007 23:29:21 +0100
> Mattias Nissler <mattias.nissler@gmx.de> wrote:
> 
> > Note that current rate_control_pid_sample() is only called from
> > rate_control_pid_tx_status(), which does an tx_num_xmit++ in advance. So
> > the tx_num_xmit branch should actually never be executed (I kept it only
> > to guard against any division by zero errors).
> 
> Eek? Removing the misleading comment would have been nice...

Indeed. I'll amend it :-)

> where does
> interpolation occur now? I got a bit confused by this.

There is no interpolation so far :-) Rate control isn't called
periodically, but only when tx status reports come in. Therefore,
currently we have fixed interval sampling only when packets are sent at
a high enough rate, i.e. 1 packet per second. I'd like to keep it that
way, so we save the timer and don't have to worry about synchronization.
I guess you rather want to base your sharpening patch on the jiffies -
last_sample difference. If it's to much, there haven't been many packets
recently. This will also work when rate control is started, cause
last_sample == 0 then.

Mattias

[RFC/T][PATCH v2 2/3] rc80211-pid: introduce PID sharpening factor

[Stefano Brivio]

This patch introduces a PID sharpening factor for faster response after
association and low activity events.


Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
Cc: Mattias Nissler <mattias.nissler@gmx.de>

---

Ok, this isn't a no-op anymore. :)

---

Index: wireless-2.6/net/mac80211/rc80211_pid.c
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid.c
+++ wireless-2.6/net/mac80211/rc80211_pid.c
@@ -23,13 +23,16 @@
  *
  * The controller basically computes the following:
  *
- * adj = CP * err + CI * err_avg + CD * (err - last_err)
+ * adj = CP * err + CI * err_avg + CD * (err - last_err) * (1 + sharpening)
  *
  * where
  * 	adj	adjustment value that is used to switch TX rate (see below)
  * 	err	current error: target vs. current failed frames percentage
  * 	last_err	last error
  * 	err_avg	average (i.e. poor man's integral) of recent errors
+ *	sharpening	non-zero when fast response is needed (i.e. right after
+ *			association or no frames sent for a long time), heading
+ * 			to zero over time
  * 	CP	Proportional coefficient
  * 	CI	Integral coefficient
  * 	CD	Derivative coefficient
@@ -65,6 +68,11 @@
 #define RC_PID_SMOOTHING_SHIFT 3
 #define RC_PID_SMOOTHING (1 << RC_PID_SMOOTHING_SHIFT)
 
+/* Sharpening factor (used for D part of PID controller) */
+#define RATE_CONTROL_SHARPENING_SHIFT 2
+#define RATE_CONTROL_SHARPENING (1 << RATE_CONTROL_SHARPENING_SHIFT)
+#define RATE_CONTROL_SHARPENING_DURATION 1
+
 /* Fixed point arithmetic shifting amount. */
 #define RC_PID_ARITH_SHIFT 8
 
@@ -131,8 +139,11 @@ struct rc_pid_sta_info {
 	 */
 	s32 err_avg_sc;
 
-	/* Last framed failes percentage sample */
+	/* Last framed failes percentage sample. */
 	u32 last_pf;
+
+	/* Sharpening needed. */
+	u8 sharp_cnt;
 };
 
 /* Algorithm parameters. We keep them on a per-algorithm approach, so they can
@@ -282,19 +293,19 @@ static void rate_control_pid_sample(stru
 
 	mode = local->oper_hw_mode;
 	spinfo = sta->rate_ctrl_priv;
+
+	/* In case nothing happened during the previous control interval, turn
+	 * on the sharpening factor. */
+	if (jiffies - spinfo->last_sample > RC_PID_INTERVAL)
+		spinfo->sharp_cnt = RATE_CONTROL_SHARPENING_DURATION;
+
 	spinfo->last_sample = jiffies;
 
-	/* If no frames were transmitted, we assume the old sample is
-	 * still a good measurement and copy it. */
-	if (spinfo->tx_num_xmit == 0)
-		pf = spinfo->last_pf;
-	else {
-		pf = spinfo->tx_num_failed * 100 / spinfo->tx_num_xmit;
-		pf <<= RC_PID_ARITH_SHIFT;
+	pf = spinfo->tx_num_failed * 100 / spinfo->tx_num_xmit;
+	pf <<= RC_PID_ARITH_SHIFT;
 
-		spinfo->tx_num_xmit = 0;
-		spinfo->tx_num_failed = 0;
-	}
+	spinfo->tx_num_xmit = 0;
+	spinfo->tx_num_failed = 0;
 
 	/* If we just switched rate, update the rate behaviour info. */
 	if (pinfo->oldrate != sta->txrate) {
@@ -321,8 +332,11 @@ static void rate_control_pid_sample(stru
 	spinfo->err_avg_sc = spinfo->err_avg_sc - err_avg + err_prop;
 	err_int = spinfo->err_avg_sc >> RC_PID_SMOOTHING_SHIFT;
 
-	err_der = pf - spinfo->last_pf;
+	err_der = pf - spinfo->last_pf
+		  * (1 + RATE_CONTROL_SHARPENING * spinfo->sharp_cnt);
 	spinfo->last_pf = pf;
+	if (spinfo->sharp_cnt)
+			spinfo->sharp_cnt--;
 
 	/* Compute the controller output. */
 	adj = (err_prop * pinfo->coeff_p + err_int * pinfo->coeff_i


-- 
Ciao
Stefano

Re: [RFC/T][PATCH v2 2/3] rc80211-pid: introduce PID sharpening factor

[Mattias Nissler]

On Mon, 2007-12-10 at 03:28 +0100, Stefano Brivio wrote:
> This patch introduces a PID sharpening factor for faster response after
> association and low activity events.
> 
> 
> Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
> Cc: Mattias Nissler <mattias.nissler@gmx.de>
> 
> ---
> 
> Ok, this isn't a no-op anymore. :)
> 
> ---
> 
> Index: wireless-2.6/net/mac80211/rc80211_pid.c
> ===================================================================
> --- wireless-2.6.orig/net/mac80211/rc80211_pid.c
> +++ wireless-2.6/net/mac80211/rc80211_pid.c
> @@ -23,13 +23,16 @@
>   *
>   * The controller basically computes the following:
>   *
> - * adj = CP * err + CI * err_avg + CD * (err - last_err)
> + * adj = CP * err + CI * err_avg + CD * (err - last_err) * (1 + sharpening)
>   *
>   * where
>   * 	adj	adjustment value that is used to switch TX rate (see below)
>   * 	err	current error: target vs. current failed frames percentage
>   * 	last_err	last error
>   * 	err_avg	average (i.e. poor man's integral) of recent errors
> + *	sharpening	non-zero when fast response is needed (i.e. right after
> + *			association or no frames sent for a long time), heading
> + * 			to zero over time
>   * 	CP	Proportional coefficient
>   * 	CI	Integral coefficient
>   * 	CD	Derivative coefficient
> @@ -65,6 +68,11 @@
>  #define RC_PID_SMOOTHING_SHIFT 3
>  #define RC_PID_SMOOTHING (1 << RC_PID_SMOOTHING_SHIFT)
>  
> +/* Sharpening factor (used for D part of PID controller) */
> +#define RATE_CONTROL_SHARPENING_SHIFT 2
> +#define RATE_CONTROL_SHARPENING (1 << RATE_CONTROL_SHARPENING_SHIFT)
> +#define RATE_CONTROL_SHARPENING_DURATION 1
> +
>  /* Fixed point arithmetic shifting amount. */
>  #define RC_PID_ARITH_SHIFT 8
>  
> @@ -131,8 +139,11 @@ struct rc_pid_sta_info {
>  	 */
>  	s32 err_avg_sc;
>  
> -	/* Last framed failes percentage sample */
> +	/* Last framed failes percentage sample. */
>  	u32 last_pf;
> +
> +	/* Sharpening needed. */
> +	u8 sharp_cnt;
>  };
>  
>  /* Algorithm parameters. We keep them on a per-algorithm approach, so they can
> @@ -282,19 +293,19 @@ static void rate_control_pid_sample(stru
>  
>  	mode = local->oper_hw_mode;
>  	spinfo = sta->rate_ctrl_priv;
> +
> +	/* In case nothing happened during the previous control interval, turn
> +	 * on the sharpening factor. */
> +	if (jiffies - spinfo->last_sample > RC_PID_INTERVAL)
> +		spinfo->sharp_cnt = RATE_CONTROL_SHARPENING_DURATION;
> +
>  	spinfo->last_sample = jiffies;
>  
> -	/* If no frames were transmitted, we assume the old sample is
> -	 * still a good measurement and copy it. */
> -	if (spinfo->tx_num_xmit == 0)
> -		pf = spinfo->last_pf;

Please don't remove this check. We can never know when anybody starts
calling rate_control_pid_sample() without packets transmitted. Then it's
good to have it, else we divide by zero in the next line.

> -	else {
> -		pf = spinfo->tx_num_failed * 100 / spinfo->tx_num_xmit;
> -		pf <<= RC_PID_ARITH_SHIFT;
> +	pf = spinfo->tx_num_failed * 100 / spinfo->tx_num_xmit;
> +	pf <<= RC_PID_ARITH_SHIFT;
>  
> -		spinfo->tx_num_xmit = 0;
> -		spinfo->tx_num_failed = 0;
> -	}
> +	spinfo->tx_num_xmit = 0;
> +	spinfo->tx_num_failed = 0;
>  
>  	/* If we just switched rate, update the rate behaviour info. */
>  	if (pinfo->oldrate != sta->txrate) {
> @@ -321,8 +332,11 @@ static void rate_control_pid_sample(stru
>  	spinfo->err_avg_sc = spinfo->err_avg_sc - err_avg + err_prop;
>  	err_int = spinfo->err_avg_sc >> RC_PID_SMOOTHING_SHIFT;
>  
> -	err_der = pf - spinfo->last_pf;
> +	err_der = pf - spinfo->last_pf
> +		  * (1 + RATE_CONTROL_SHARPENING * spinfo->sharp_cnt);
>  	spinfo->last_pf = pf;
> +	if (spinfo->sharp_cnt)
> +			spinfo->sharp_cnt--;
>  
>  	/* Compute the controller output. */
>  	adj = (err_prop * pinfo->coeff_p + err_int * pinfo->coeff_i
> 
> 

Re: [RFC/T][PATCH v2 2/3] rc80211-pid: introduce PID sharpening factor

[Stefano Brivio]

On Mon, 10 Dec 2007 07:28:01 +0100
Mattias Nissler <mattias.nissler@gmx.de> wrote:

> > -	/* If no frames were transmitted, we assume the old sample is
> > -	 * still a good measurement and copy it. */
> > -	if (spinfo->tx_num_xmit == 0)
> > -		pf = spinfo->last_pf;
> 
> Please don't remove this check. We can never know when anybody starts
> calling rate_control_pid_sample() without packets transmitted. Then it's
> good to have it, else we divide by zero in the next line.

But, as you said, rate_control_pid_sample() only gets called by
rate_control_pid_tx_status(). There, spinfo->tx_num_xmit always get
increased. The only other spot where spinfo->tx_num_xmit gets changed is in
rate_control_pid_sample(), where we set it to 0 after that that division
gets done, and we obviously change its value after having been called by
rate_control_pid_tx_status(). So, it always get increased before the
division, and it's never set to a negative value. Therefore, I assume that
it will never be zero in the division. Am I wrong?


--
Ciao
Stefano

Re: [RFC/T][PATCH v2 2/3] rc80211-pid: introduce PID sharpening factor

[Mattias Nissler]

On Mon, 2007-12-10 at 08:21 +0100, Stefano Brivio wrote:
> On Mon, 10 Dec 2007 07:28:01 +0100
> Mattias Nissler <mattias.nissler@gmx.de> wrote:
> 
> > > -	/* If no frames were transmitted, we assume the old sample is
> > > -	 * still a good measurement and copy it. */
> > > -	if (spinfo->tx_num_xmit == 0)
> > > -		pf = spinfo->last_pf;
> > 
> > Please don't remove this check. We can never know when anybody starts
> > calling rate_control_pid_sample() without packets transmitted. Then it's
> > good to have it, else we divide by zero in the next line.
> 
> But, as you said, rate_control_pid_sample() only gets called by
> rate_control_pid_tx_status(). There, spinfo->tx_num_xmit always get
> increased. The only other spot where spinfo->tx_num_xmit gets changed is in
> rate_control_pid_sample(), where we set it to 0 after that that division
> gets done, and we obviously change its value after having been called by
> rate_control_pid_tx_status(). So, it always get increased before the
> division, and it's never set to a negative value. Therefore, I assume that
> it will never be zero in the division. Am I wrong?

You're totally right. But IMHO it's nicer to be a little defensive here.
You never know what happens to your code in the future. Just imagine
somebody coming up with: Hey, let's use a timer-based approach for
sampling. Ok, that's easy to do, just call sample() every time the timer
expires. And whoops, there's the division by zero when no packets are
transmitted...

Mattias

Re: [RFC/T][PATCH v2 2/3] rc80211-pid: introduce PID sharpening factor

[Stefano Brivio]

On Mon, 10 Dec 2007 08:44:56 +0100
Mattias Nissler <mattias.nissler@gmx.de> wrote:

> You're totally right. But IMHO it's nicer to be a little defensive here.
> You never know what happens to your code in the future. Just imagine
> somebody coming up with: Hey, let's use a timer-based approach for
> sampling. Ok, that's easy to do, just call sample() every time the timer
> expires. And whoops, there's the division by zero when no packets are
> transmitted...

Ok, you are right, I'll fix this.


-- 
Ciao
Stefano

[RFC/T][PATCH v3 2/3] rc80211-pid: introduce PID sharpening factor

[Stefano Brivio]

This patch introduces a PID sharpening factor for faster response on
association and interpolation.


Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>

---

Index: wireless-2.6/net/mac80211/rc80211_pid.c
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid.c
+++ wireless-2.6/net/mac80211/rc80211_pid.c
@@ -23,13 +23,16 @@
  *
  * The controller basically computes the following:
  *
- * adj = CP * err + CI * err_avg + CD * (err - last_err)
+ * adj = CP * err + CI * err_avg + CD * (err - last_err) * (1 + sharpening)
  *
  * where
  * 	adj	adjustment value that is used to switch TX rate (see below)
  * 	err	current error: target vs. current failed frames percentage
  * 	last_err	last error
  * 	err_avg	average (i.e. poor man's integral) of recent errors
+ *	sharpening	non-zero when fast response is needed (i.e. right after
+ *			association or no frames sent for a long time), heading
+ * 			to zero over time
  * 	CP	Proportional coefficient
  * 	CI	Integral coefficient
  * 	CD	Derivative coefficient
@@ -65,6 +68,11 @@
 #define RC_PID_SMOOTHING_SHIFT 3
 #define RC_PID_SMOOTHING (1 << RC_PID_SMOOTHING_SHIFT)
 
+/* Sharpening factor (used for D part of PID controller) */
+#define RATE_CONTROL_SHARPENING_SHIFT 2
+#define RATE_CONTROL_SHARPENING (1 << RATE_CONTROL_SHARPENING_SHIFT)
+#define RATE_CONTROL_SHARPENING_DURATION 1
+
 /* Fixed point arithmetic shifting amount. */
 #define RC_PID_ARITH_SHIFT 8
 
@@ -131,8 +139,11 @@ struct rc_pid_sta_info {
 	 */
 	s32 err_avg_sc;
 
-	/* Last framed failes percentage sample */
+	/* Last framed failes percentage sample. */
 	u32 last_pf;
+
+	/* Sharpening needed. */
+	u8 sharp_cnt;
 };
 
 /* Algorithm parameters. We keep them on a per-algorithm approach, so they can
@@ -275,20 +286,26 @@ static void rate_control_pid_sample(stru
 
 	mode = local->oper_hw_mode;
 	spinfo = sta->rate_ctrl_priv;
+
+	/* In case nothing happened during the previous control interval, turn
+	 * on the sharpening factor. */
+	if (jiffies - spinfo->last_sample > RC_PID_INTERVAL)
+		spinfo->sharp_cnt = RATE_CONTROL_SHARPENING_DURATION;
+
 	spinfo->last_sample = jiffies;
 
-	/* If no frames were transmitted, we assume the old sample is
+	/* This should never happen, but in case, we assume the old sample is
 	 * still a good measurement and copy it. */
-	if (spinfo->tx_num_xmit == 0)
+	if (unlikely(spinfo->tx_num_xmit == 0))
 		pf = spinfo->last_pf;
 	else {
 		pf = spinfo->tx_num_failed * 100 / spinfo->tx_num_xmit;
 		pf <<= RC_PID_ARITH_SHIFT;
-
-		spinfo->tx_num_xmit = 0;
-		spinfo->tx_num_failed = 0;
 	}
 
+	spinfo->tx_num_xmit = 0;
+	spinfo->tx_num_failed = 0;
+
 	/* If we just switched rate, update the rate behaviour info. */
 	if (pinfo->oldrate != sta->txrate) {
 
@@ -313,8 +330,11 @@ static void rate_control_pid_sample(stru
 	spinfo->err_avg_sc = spinfo->err_avg_sc - err_avg + err_prop;
 	err_int = spinfo->err_avg_sc >> RC_PID_SMOOTHING_SHIFT;
 
-	err_der = pf - spinfo->last_pf;
+	err_der = pf - spinfo->last_pf
+		  * (1 + RATE_CONTROL_SHARPENING * spinfo->sharp_cnt);
 	spinfo->last_pf = pf;
+	if (spinfo->sharp_cnt)
+			spinfo->sharp_cnt--;
 
 	/* Compute the controller output. */
 	adj = (err_prop * pinfo->coeff_p + err_int * pinfo->coeff_i



-- 
Ciao
Stefano

[RFC/T][PATCH 3/3] rc80211-pid: allow for parameters to be set through sysfs

[Stefano Brivio]

This patch allows for tuning parameters to be set through sysfs. Note that
this lacks locking, as another copy of the whole parameters data would have
been needed and this won't be the final approach anyway, so don't bother
too much.


Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>

---

Mattias: oops. Just noticed that my previous approach to locking here was
bogus due to misconceptions about sysfs parameters. It's possible to get
locking right here, but quite troublesome.
Everybody: this lacks proper locking. Never apply this to any sane tree,
please.

---

Index: wireless-2.6/net/mac80211/rc80211_pid.c
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid.c
+++ wireless-2.6/net/mac80211/rc80211_pid.c
@@ -60,6 +60,51 @@
  * RC_PID_ARITH_SHIFT.
  */
 
+static int modparam_rc_imul = 1;
+module_param_named(rc_imul, modparam_rc_imul, int, 0644);
+MODULE_PARM_DESC(rc_imul, "PID rate control interval multiplier");
+
+static int modparam_rc_idiv = 1;
+module_param_named(rc_idiv, modparam_rc_idiv, int, 0644);
+MODULE_PARM_DESC(rc_idiv, "PID rate control interval divider");
+
+static int modparam_rc_pf = 1;
+module_param_named(rc_pf, modparam_rc_pf, int, 0644);
+MODULE_PARM_DESC(rc_pf, "PID rate control failed frames percentage target");
+
+static int modparam_rc_p = 1;
+module_param_named(rc_p, modparam_rc_p, int, 0644);
+MODULE_PARM_DESC(rc_p, "PID rate control proportional coefficient");
+
+static int modparam_rc_i = 1;
+module_param_named(rc_i, modparam_rc_i, int, 0644);
+MODULE_PARM_DESC(rc_i, "PID rate control integral coefficient");
+
+static int modparam_rc_d = 1;
+module_param_named(rc_d, modparam_rc_d, int, 0644);
+MODULE_PARM_DESC(rc_d, "PID rate control derivative coefficient");
+
+static int modparam_rc_sm_s = 3;
+module_param_named(rc_sm_s, modparam_rc_sm_s, int, 0644);
+MODULE_PARM_DESC(rc_sm_s, "PID rate control smoothing factor shift");
+
+static int modparam_rc_sh_s = 2;
+module_param_named(rc_sh_s, modparam_rc_sh_s, int, 0644);
+MODULE_PARM_DESC(rc_sh_s, "PID rate control sharpening factor shift");
+
+static int modparam_rc_sh_d = 3;
+module_param_named(rc_sh_d, modparam_rc_sh_d, int, 0644);
+MODULE_PARM_DESC(rc_sh_d, "PID rate control sharpening factor duration");
+
+static int modparam_rc_norm_factor = 3;
+module_param_named(rc_norm_factor, modparam_rc_norm_factor, int, 0644);
+MODULE_PARM_DESC(rc_norm_factor, "PID rate behaviour normalization factor");
+
+static int modparam_rc_fast_start = 0;
+module_param_named(rc_fast_start, modparam_rc_fast_start, int, 0644);
+MODULE_PARM_DESC(rc_fast_start, "PID allowance for high rates right after"
+				"loading");
+
 /* Sampling frequency for measuring percentage of failed frames. */
 #define RC_PID_INTERVAL (HZ / 1)
 
@@ -158,13 +203,28 @@ struct rc_pid_rateinfo {
 
 struct rc_pid_info {
 
+	/* Rate control interval multiplier and divider. */
+	int mul;
+	int div;
+
 	/* The failed frames percentage target. */
-	u32 target;
+	int target;
 
 	/* P, I and D coefficients. */
-	s32 coeff_p;
-	s32 coeff_i;
-	s32 coeff_d;
+	int coeff_p;
+	int coeff_i;
+	int coeff_d;
+
+	/* Smoothing and sharpening factors. */
+	int sm_s;
+	int sh_s;
+	int sh_d;
+
+	/* Rate behaviour normalization factor. */
+	int norm_factor;
+
+	/* Fast start. */
+	bool fast_start;
 
 	/* Rates information. */
 	struct rc_pid_rateinfo *rinfo;
@@ -245,20 +305,27 @@ static void rate_control_pid_adjust_rate
 }
 
 /* Normalize the failed frames per-rate differences. */
-static void rate_control_pid_normalize(struct rc_pid_rateinfo *r, int l)
+static void rate_control_pid_normalize(struct rc_pid_info *p, int l)
 {
-	int i;
+	int i, nf;
+	struct rc_pid_rateinfo *r = p->rinfo;
 
-	if (r[0].diff > RC_PID_NORM_FACTOR)
-		r[0].diff -= RC_PID_NORM_FACTOR;
-	else if (r[0].diff < -RC_PID_NORM_FACTOR)
-		r[0].diff += RC_PID_NORM_FACTOR;
+	/* TODO: RCU lock needed here when implemented properly. */
+	p->norm_factor = modparam_rc_norm_factor;
+	/* TODO: RCU unlock. */
+
+	nf = p->norm_factor;
+
+	if (r[0].diff > nf)
+		r[0].diff -= nf;
+	else if (r[0].diff < -nf)
+		r[0].diff += nf;
 	for (i = 0; i < l - 1; i++)
 		if (likely(r[i + 1].valid)) {
-			if (r[i + 1].diff > r[i].diff + RC_PID_NORM_FACTOR)
-				r[i + 1].diff -= RC_PID_NORM_FACTOR;
+			if (r[i + 1].diff > r[i].diff + nf)
+				r[i + 1].diff -= nf;
 			else if (r[i + 1].diff <= r[i].diff)
-				r[i + 1].diff += RC_PID_NORM_FACTOR;
+				r[i + 1].diff += nf;
 		}
 }
 
@@ -280,12 +347,22 @@ static void rate_control_pid_sample(stru
 	spinfo = sta->rate_ctrl_priv;
 	spinfo->last_sample = jiffies;
 
+	/* TODO: RCU lock needed here when implemented properly. */
+	pinfo->target = modparam_rc_pf;
+	pinfo->coeff_p = modparam_rc_p;
+	pinfo->coeff_i = modparam_rc_i;
+	pinfo->coeff_d = modparam_rc_d;
+	pinfo->sm_s = modparam_rc_sm_s;
+	pinfo->sh_s = modparam_rc_sh_s;
+	pinfo->sh_d = modparam_rc_sh_d;
+	/* TODO: RCU unlock. */
+
 	/* If no frames were transmitted, we assume the old sample is
 	 * still a good measurement and copy it, and turn the sharpening factor
 	 * on. */
 	if (spinfo->tx_num_xmit == 0) {
 		pf = spinfo->last_pf;
-		spinfo->sharp_cnt = RATE_CONTROL_SHARPENING_DURATION;
+		spinfo->sharp_cnt = pinfo->sh_d;
 	} else {
 		pf = spinfo->tx_num_failed * 100 / spinfo->tx_num_xmit;
 		pf <<= RC_PID_ARITH_SHIFT;
@@ -317,17 +394,17 @@ static void rate_control_pid_sample(stru
 		rinfo[j].valid = 1;
 		pinfo->oldrate = sta->txrate;
 	}
-	rate_control_pid_normalize(rinfo, mode->num_rates);
+	rate_control_pid_normalize(pinfo, mode->num_rates);
 
 	/* Compute the proportional, integral and derivative errors. */
 	err_prop = RC_PID_TARGET_PF - pf;
 
-	err_avg = spinfo->err_avg_sc >> RC_PID_SMOOTHING_SHIFT;
+	err_avg = spinfo->err_avg_sc >> pinfo->sm_s;
 	spinfo->err_avg_sc = spinfo->err_avg_sc - err_avg + err_prop;
-	err_int = spinfo->err_avg_sc >> RC_PID_SMOOTHING_SHIFT;
+	err_int = spinfo->err_avg_sc >> pinfo->sm_s;
 
 	err_der = pf - spinfo->last_pf
-		  * (1 + RATE_CONTROL_SHARPENING * spinfo->sharp_cnt);
+		  * (1 + (1 << pinfo->sh_s) * spinfo->sharp_cnt);
 	spinfo->last_pf = pf;
 	if (spinfo->sharp_cnt)
 			spinfo->sharp_cnt--;
@@ -389,7 +466,12 @@ static void rate_control_pid_tx_status(v
 	sta->tx_num_mpdu_fail += status->retry_count;
 
 	/* Update PID controller state. */
-	if (time_after(jiffies, spinfo->last_sample + RC_PID_INTERVAL))
+	/* TODO: RCU lock needed here when implemented properly. */
+	pinfo->mul = modparam_rc_imul;
+	pinfo->div = modparam_rc_idiv;
+	/* TODO: RCU unlock. */
+	if (time_after(jiffies, spinfo->last_sample
+				+ (HZ * pinfo->mul) / pinfo->div))
 		rate_control_pid_sample(pinfo, local, sta);
 
 	sta_info_put(sta);
@@ -457,11 +539,14 @@ static void *rate_control_pid_alloc(stru
 		return NULL;
 	}
 
+	/* TODO: RCU lock needed here when implemented properly. */
+	pinfo->fast_start = modparam_rc_fast_start;
+	/* TODO: RCU unlock. */
 	/* Sort the rates. This is optimized for the most common case (i.e.
 	 * almost-sorted CCK+OFDM rates). */
 	for (i = 0; i < mode->num_rates; i++) {
 		rinfo[i].index = i;
-		if (RC_PID_FAST_START) {
+		if (pinfo->fast_start) {
 			rinfo[i].valid = 1;
 			rinfo[i].diff = 0;
 		} else
@@ -484,10 +569,6 @@ static void *rate_control_pid_alloc(stru
 	rinfo[0].diff = 0;
 	rinfo[0].valid = 1;
 
-	pinfo->target = RC_PID_TARGET_PF;
-	pinfo->coeff_p = RC_PID_COEFF_P;
-	pinfo->coeff_i = RC_PID_COEFF_I;
-	pinfo->coeff_d = RC_PID_COEFF_D;
 	pinfo->rinfo = rinfo;
 	pinfo->oldrate = 0;
 

-- 
Ciao
Stefano

Re: [RFC/T][PATCH 3/3] rc80211-pid: allow for parameters to be set through sysfs

[Mattias Nissler]

On Sun, 2007-12-09 at 21:28 +0100, Stefano Brivio wrote:
> This patch allows for tuning parameters to be set through sysfs. Note that
> this lacks locking, as another copy of the whole parameters data would have
> been needed and this won't be the final approach anyway, so don't bother
> too much.
> 
> 
> Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
> 
> ---
> 
> Mattias: oops. Just noticed that my previous approach to locking here was
> bogus due to misconceptions about sysfs parameters. It's possible to get
> locking right here, but quite troublesome.

Ok, no worries, it's for testing only anyway.

Mattias

[RFC/T][PATCH v2 3/3] rc80211-pid: allow for parameters to be set through sysfs

[Stefano Brivio]

This patch allows for tuning parameters to be set through sysfs. Note that
this lacks locking, as another copy of the whole parameters data would have
been needed and this won't be the final approach anyway, so don't bother
too much.


Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>

---

This applies on top of previous v2 patches.

---

Index: wireless-2.6/net/mac80211/rc80211_pid.c
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid.c
+++ wireless-2.6/net/mac80211/rc80211_pid.c
@@ -61,17 +61,50 @@
  * RC_PID_ARITH_SHIFT.
  */
 
-/* Sampling frequency for measuring percentage of failed frames. */
-#define RC_PID_INTERVAL (HZ / 1)
-
-/* Exponential averaging smoothness (used for I part of PID controller) */
-#define RC_PID_SMOOTHING_SHIFT 3
-#define RC_PID_SMOOTHING (1 << RC_PID_SMOOTHING_SHIFT)
-
-/* Sharpening factor (used for D part of PID controller) */
-#define RATE_CONTROL_SHARPENING_SHIFT 2
-#define RATE_CONTROL_SHARPENING (1 << RATE_CONTROL_SHARPENING_SHIFT)
-#define RATE_CONTROL_SHARPENING_DURATION 1
+static int modparam_rc_imul = 1;
+module_param_named(rc_imul, modparam_rc_imul, int, 0644);
+MODULE_PARM_DESC(rc_imul, "PID rate control interval multiplier");
+
+static int modparam_rc_idiv = 1;
+module_param_named(rc_idiv, modparam_rc_idiv, int, 0644);
+MODULE_PARM_DESC(rc_idiv, "PID rate control interval divider");
+
+static int modparam_rc_pf = 1;
+module_param_named(rc_pf, modparam_rc_pf, int, 0644);
+MODULE_PARM_DESC(rc_pf, "PID rate control failed frames percentage target");
+
+static int modparam_rc_p = 1;
+module_param_named(rc_p, modparam_rc_p, int, 0644);
+MODULE_PARM_DESC(rc_p, "PID rate control proportional coefficient");
+
+static int modparam_rc_i = 1;
+module_param_named(rc_i, modparam_rc_i, int, 0644);
+MODULE_PARM_DESC(rc_i, "PID rate control integral coefficient");
+
+static int modparam_rc_d = 1;
+module_param_named(rc_d, modparam_rc_d, int, 0644);
+MODULE_PARM_DESC(rc_d, "PID rate control derivative coefficient");
+
+static int modparam_rc_sm_s = 3;
+module_param_named(rc_sm_s, modparam_rc_sm_s, int, 0644);
+MODULE_PARM_DESC(rc_sm_s, "PID rate control smoothing factor shift");
+
+static int modparam_rc_sh_s = 2;
+module_param_named(rc_sh_s, modparam_rc_sh_s, int, 0644);
+MODULE_PARM_DESC(rc_sh_s, "PID rate control sharpening factor shift");
+
+static int modparam_rc_sh_d = 3;
+module_param_named(rc_sh_d, modparam_rc_sh_d, int, 0644);
+MODULE_PARM_DESC(rc_sh_d, "PID rate control sharpening factor duration");
+
+static int modparam_rc_norm_offset = 3;
+module_param_named(rc_norm_offset, modparam_rc_norm_offset, int, 0644);
+MODULE_PARM_DESC(rc_norm_offset, "PID rate behaviour normalization offset");
+
+static int modparam_rc_fast_start = 0;
+module_param_named(rc_fast_start, modparam_rc_fast_start, int, 0644);
+MODULE_PARM_DESC(rc_fast_start, "PID allowance for high rates right after"
+				"loading");
 
 /* Fixed point arithmetic shifting amount. */
 #define RC_PID_ARITH_SHIFT 8
@@ -79,26 +112,6 @@
 /* Fixed point arithmetic factor. */
 #define RC_PID_ARITH_FACTOR (1 << RC_PID_ARITH_SHIFT)
 
-/* Proportional PID component coefficient. */
-#define RC_PID_COEFF_P 15
-/* Integral PID component coefficient. */
-#define RC_PID_COEFF_I 10
-/* Derivative PID component coefficient. */
-#define RC_PID_COEFF_D 15
-
-/* Target failed frames rate for the PID controller. NB: This effectively gives
- * maximum failed frames percentage we're willing to accept. If communication is
- * good, the controller will fail to adjust failed frames percentage to the
- * target. This is intentional.
- */
-#define RC_PID_TARGET_PF (20 << RC_PID_ARITH_SHIFT)
-
-/* Rate behaviour normalization quantity over time. */
-#define RC_PID_NORM_OFFSET 3
-
-/* Push high rates right after loading. */
-#define RC_PID_FAST_START 0
-
 /* Arithmetic right shift for positive and negative values for ISO C. */
 #define RC_PID_DO_ARITH_RIGHT_SHIFT(x, y) \
 	(x) < 0 ? -((-(x))) >> (y) : (x) >> (y)
@@ -163,13 +176,28 @@ struct rc_pid_rateinfo {
 
 struct rc_pid_info {
 
+	/* Rate control interval multiplier and divider. */
+	int imul;
+	int idiv;
+
 	/* The failed frames percentage target. */
-	u32 target;
+	int target;
 
 	/* P, I and D coefficients. */
-	s32 coeff_p;
-	s32 coeff_i;
-	s32 coeff_d;
+	int coeff_p;
+	int coeff_i;
+	int coeff_d;
+
+	/* Smoothing and sharpening factors. */
+	int sm_s;
+	int sh_s;
+	int sh_d;
+
+	/* Rate behaviour normalization factor. */
+	int norm_offset;
+
+	/* Fast start. */
+	bool fast_start;
 
 	/* Rates information. */
 	struct rc_pid_rateinfo *rinfo;
@@ -260,20 +288,27 @@ static void rate_control_pid_adjust_rate
 }
 
 /* Normalize the failed frames per-rate differences. */
-static void rate_control_pid_normalize(struct rc_pid_rateinfo *r, int l)
+static void rate_control_pid_normalize(struct rc_pid_info *p, int l)
 {
-	int i;
+	int i, no;
+	struct rc_pid_rateinfo *r = p->rinfo;
 
-	if (r[0].diff > RC_PID_NORM_OFFSET)
-		r[0].diff -= RC_PID_NORM_OFFSET;
-	else if (r[0].diff < -RC_PID_NORM_OFFSET)
-		r[0].diff += RC_PID_NORM_OFFSET;
+	/* TODO: RCU lock needed here when implemented properly. */
+	p->norm_offset = modparam_rc_norm_offset;
+	/* TODO: RCU unlock. */
+
+	no = p->norm_offset;
+
+	if (r[0].diff > no)
+		r[0].diff -= no;
+	else if (r[0].diff < -no)
+		r[0].diff += no;
 	for (i = 0; i < l - 1; i++)
 		if (likely(r[i + 1].valid)) {
-			if (r[i + 1].diff > r[i].diff + RC_PID_NORM_OFFSET)
-				r[i + 1].diff -= RC_PID_NORM_OFFSET;
+			if (r[i + 1].diff > r[i].diff + no)
+				r[i + 1].diff -= no;
 			else if (r[i + 1].diff <= r[i].diff)
-				r[i + 1].diff += RC_PID_NORM_OFFSET;
+				r[i + 1].diff += no;
 		}
 }
 
@@ -294,10 +329,22 @@ static void rate_control_pid_sample(stru
 	mode = local->oper_hw_mode;
 	spinfo = sta->rate_ctrl_priv;
 
+	/* TODO: RCU lock needed here when implemented properly. */
+	pinfo->imul = modparam_rc_imul;
+	pinfo->idiv = modparam_rc_idiv;
+	pinfo->target = modparam_rc_pf;
+	pinfo->coeff_p = modparam_rc_p;
+	pinfo->coeff_i = modparam_rc_i;
+	pinfo->coeff_d = modparam_rc_d;
+	pinfo->sm_s = modparam_rc_sm_s;
+	pinfo->sh_s = modparam_rc_sh_s;
+	pinfo->sh_d = modparam_rc_sh_d;
+	/* TODO: RCU unlock. */
+
 	/* In case nothing happened during the previous control interval, turn
 	 * on the sharpening factor. */
-	if (jiffies - spinfo->last_sample > RC_PID_INTERVAL)
-		spinfo->sharp_cnt = RATE_CONTROL_SHARPENING_DURATION;
+	if (jiffies - spinfo->last_sample > (HZ * pinfo->imul) / pinfo->idiv)
+		spinfo->sharp_cnt = pinfo->sh_d;
 
 	spinfo->last_sample = jiffies;
 
@@ -323,17 +370,17 @@ static void rate_control_pid_sample(stru
 		rinfo[j].valid = 1;
 		pinfo->oldrate = sta->txrate;
 	}
-	rate_control_pid_normalize(rinfo, mode->num_rates);
+	rate_control_pid_normalize(pinfo, mode->num_rates);
 
 	/* Compute the proportional, integral and derivative errors. */
-	err_prop = RC_PID_TARGET_PF - pf;
+	err_prop = (20 << pinfo->target) - pf;
 
-	err_avg = spinfo->err_avg_sc >> RC_PID_SMOOTHING_SHIFT;
+	err_avg = spinfo->err_avg_sc >> pinfo->sm_s;
 	spinfo->err_avg_sc = spinfo->err_avg_sc - err_avg + err_prop;
-	err_int = spinfo->err_avg_sc >> RC_PID_SMOOTHING_SHIFT;
+	err_int = spinfo->err_avg_sc >> pinfo->sm_s;
 
 	err_der = pf - spinfo->last_pf
-		  * (1 + RATE_CONTROL_SHARPENING * spinfo->sharp_cnt);
+		  * (1 + (1 << pinfo->sh_s) * spinfo->sharp_cnt);
 	spinfo->last_pf = pf;
 	if (spinfo->sharp_cnt)
 			spinfo->sharp_cnt--;
@@ -392,7 +439,12 @@ static void rate_control_pid_tx_status(v
 	sta->tx_num_mpdu_fail += status->retry_count;
 
 	/* Update PID controller state. */
-	if (time_after(jiffies, spinfo->last_sample + RC_PID_INTERVAL))
+	/* TODO: RCU lock needed here when implemented properly. */
+	pinfo->imul = modparam_rc_imul;
+	pinfo->idiv = modparam_rc_idiv;
+	/* TODO: RCU unlock. */
+	if (time_after(jiffies, spinfo->last_sample
+				+ (HZ * pinfo->imul) / pinfo->idiv))
 		rate_control_pid_sample(pinfo, local, sta);
 
 	sta_info_put(sta);
@@ -460,11 +512,14 @@ static void *rate_control_pid_alloc(stru
 		return NULL;
 	}
 
+	/* TODO: RCU lock needed here when implemented properly. */
+	pinfo->fast_start = modparam_rc_fast_start;
+	/* TODO: RCU unlock. */
 	/* Sort the rates. This is optimized for the most common case (i.e.
 	 * almost-sorted CCK+OFDM rates). */
 	for (i = 0; i < mode->num_rates; i++) {
 		rinfo[i].index = i;
-		if (RC_PID_FAST_START) {
+		if (pinfo->fast_start) {
 			rinfo[i].valid = 1;
 			rinfo[i].diff = 0;
 		} else
@@ -487,10 +542,6 @@ static void *rate_control_pid_alloc(stru
 	rinfo[0].diff = 0;
 	rinfo[0].valid = 1;
 
-	pinfo->target = RC_PID_TARGET_PF;
-	pinfo->coeff_p = RC_PID_COEFF_P;
-	pinfo->coeff_i = RC_PID_COEFF_I;
-	pinfo->coeff_d = RC_PID_COEFF_D;
 	pinfo->rinfo = rinfo;
 	pinfo->oldrate = 0;
 



-- 
Ciao
Stefano

Re: [RFC/T][PATCH 3/3] rc80211-pid: allow for parameters to be set through sysfs

[Stefano Brivio]

On Sun, 9 Dec 2007 21:28:42 +0100
Stefano Brivio <stefano.brivio@polimi.it> wrote:

> This patch allows for tuning parameters to be set through sysfs. Note that
> this lacks locking, as another copy of the whole parameters data would have
> been needed and this won't be the final approach anyway, so don't bother
> too much.
> 
> 
> Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
> 
> ---
> 
> Mattias: oops. Just noticed that my previous approach to locking here was
> bogus due to misconceptions about sysfs parameters. It's possible to get
> locking right here, but quite troublesome.
> Everybody: this lacks proper locking. Never apply this to any sane tree,
> please.

Actually, it looks like we can assume atomic operation by sysfs here. So
I'd go with this approach until we don't add the needed hooks to cfg80211.


--
Ciao
Stefano