[PATCH 0/2] PID controller fixes

[PATCH 0/2] PID controller fixes

[Bob Copeland]

Here are two fixes to the PID rc.  The first one improves PID
performance by quite a bit on my test setup, and seems correct,
but I am not an expert on the algo so comments welcome.  The
other is just a minor cleanup.  With the change, Minstrel is
still a bit better in my tests.

Bob Copeland (2):
  mac80211: fix sign error in pid controller
  mac80211: pid: replace open-coded msecs_to_jiffies

 net/mac80211/rc80211_pid_algo.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

[PATCH 1/2] mac80211: fix sign error in pid controller

[Bob Copeland]

While testing the pid rate controller in mac80211_hwsim, I noticed
that once the controller reached 54 Mbit rates, it would fail to
lower the rate when necessary.  The debug log shows:

1945 186786 pf_sample 50 3534 3577 50

My interpretation is that the fixed point scaling of the target
error value (pf) is incorrect: the error value of 50 compared to
a target of 14 case should result in a scaling value of
(14-50) = -36 * 256 or -9216, but instead it is (14 * 256)-50, or
3534.

Correct this by doing fixed point scaling after subtraction.

Cc: stefano.brivio@polimi.it
Cc: mattias.nissler@gmx.de
Signed-off-by: Bob Copeland <me@bobcopeland.com>
---

Mattias / Stefano, please advise if I missed something.

 net/mac80211/rc80211_pid_algo.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/rc80211_pid_algo.c b/net/mac80211/rc80211_pid_algo.c
index 699d3ed..29bc4c5 100644
--- a/net/mac80211/rc80211_pid_algo.c
+++ b/net/mac80211/rc80211_pid_algo.c
@@ -190,7 +190,7 @@ static void rate_control_pid_sample(struct rc_pid_info *pinfo,
 	rate_control_pid_normalize(pinfo, sband->n_bitrates);
 
 	/* Compute the proportional, integral and derivative errors. */
-	err_prop = (pinfo->target << RC_PID_ARITH_SHIFT) - pf;
+	err_prop = (pinfo->target - pf) << RC_PID_ARITH_SHIFT;
 
 	err_avg = spinfo->err_avg_sc >> pinfo->smoothing_shift;
 	spinfo->err_avg_sc = spinfo->err_avg_sc - err_avg + err_prop;
-- 
1.6.3.3

Re: [PATCH 1/2] mac80211: fix sign error in pid controller

[Stefano Brivio]

On Sat, 16 Jan 2010 14:36:52 -0500
Bob Copeland <me@bobcopeland.com> wrote:

> While testing the pid rate controller in mac80211_hwsim, I noticed
> that once the controller reached 54 Mbit rates, it would fail to
> lower the rate when necessary.  The debug log shows:
> 
> 1945 186786 pf_sample 50 3534 3577 50
> 
> My interpretation is that the fixed point scaling of the target
> error value (pf) is incorrect: the error value of 50 compared to
> a target of 14 case should result in a scaling value of
> (14-50) = -36 * 256 or -9216, but instead it is (14 * 256)-50, or
> 3534.

Good catch! I actually wonder why nobody hit this before. One possible
explanation is that the proportional error alone, in most cases, isn't
significant "enough" with regard to the other ones.

Mattias, could you please double check this anyway?

> Cc: stefano.brivio@polimi.it
> Cc: mattias.nissler@gmx.de
> Signed-off-by: Bob Copeland <me@bobcopeland.com>

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


--
Ciao
Stefano

Re: [PATCH 1/2] mac80211: fix sign error in pid controller

[Mattias Nissler]

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

On Sat, 2010-01-16 at 14:36 -0500, Bob Copeland wrote:
> While testing the pid rate controller in mac80211_hwsim, I noticed
> that once the controller reached 54 Mbit rates, it would fail to
> lower the rate when necessary.  The debug log shows:
> 
> 1945 186786 pf_sample 50 3534 3577 50
> 
> My interpretation is that the fixed point scaling of the target
> error value (pf) is incorrect: the error value of 50 compared to
> a target of 14 case should result in a scaling value of
> (14-50) = -36 * 256 or -9216, but instead it is (14 * 256)-50, or
> 3534.
> 
> Correct this by doing fixed point scaling after subtraction.
> 
> Cc: stefano.brivio@polimi.it
> Cc: mattias.nissler@gmx.de
> Signed-off-by: Bob Copeland <me@bobcopeland.com>

I took a quick look and found that you're right. pf is an unscaled
value, so we should subtract it before scaling.

Acked-by: Mattias Nissler <mattias.nissler@gmx.de>

> ---
> 
> Mattias / Stefano, please advise if I missed something.
> 
>  net/mac80211/rc80211_pid_algo.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/mac80211/rc80211_pid_algo.c b/net/mac80211/rc80211_pid_algo.c
> index 699d3ed..29bc4c5 100644
> --- a/net/mac80211/rc80211_pid_algo.c
> +++ b/net/mac80211/rc80211_pid_algo.c
> @@ -190,7 +190,7 @@ static void rate_control_pid_sample(struct rc_pid_info *pinfo,
>  	rate_control_pid_normalize(pinfo, sband->n_bitrates);
>  
>  	/* Compute the proportional, integral and derivative errors. */
> -	err_prop = (pinfo->target << RC_PID_ARITH_SHIFT) - pf;
> +	err_prop = (pinfo->target - pf) << RC_PID_ARITH_SHIFT;
>  
>  	err_avg = spinfo->err_avg_sc >> pinfo->smoothing_shift;
>  	spinfo->err_avg_sc = spinfo->err_avg_sc - err_avg + err_prop;


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

[PATCH 2/2] mac80211: pid: replace open-coded msecs_to_jiffies

[Bob Copeland]

Code directly scaling by HZ and rounding can be more efficiently
and clearly performed with msecs_to_jiffies.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 net/mac80211/rc80211_pid_algo.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/mac80211/rc80211_pid_algo.c b/net/mac80211/rc80211_pid_algo.c
index 29bc4c5..2652a37 100644
--- a/net/mac80211/rc80211_pid_algo.c
+++ b/net/mac80211/rc80211_pid_algo.c
@@ -157,9 +157,7 @@ static void rate_control_pid_sample(struct rc_pid_info *pinfo,
 
 	/* In case nothing happened during the previous control interval, turn
 	 * the sharpening factor on. */
-	period = (HZ * pinfo->sampling_period + 500) / 1000;
-	if (!period)
-		period = 1;
+	period = msecs_to_jiffies(pinfo->sampling_period);
 	if (jiffies - spinfo->last_sample > 2 * period)
 		spinfo->sharp_cnt = pinfo->sharpen_duration;
 
@@ -252,9 +250,7 @@ static void rate_control_pid_tx_status(void *priv, struct ieee80211_supported_ba
 	}
 
 	/* Update PID controller state. */
-	period = (HZ * pinfo->sampling_period + 500) / 1000;
-	if (!period)
-		period = 1;
+	period = msecs_to_jiffies(pinfo->sampling_period);
 	if (time_after(jiffies, spinfo->last_sample + period))
 		rate_control_pid_sample(pinfo, sband, sta, spinfo);
 }
-- 
1.6.3.3