[PATCH 1/7] rc80211-pid: export human-readable target_pf value to debugfs

[PATCH 1/7] rc80211-pid: export human-readable target_pf value to debugfs

[Stefano Brivio]

Export the non-shifted target_pf value to debugfs, so that it's human-readable.

Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
---
 rc80211_pid.h      |    2 +-
 rc80211_pid_algo.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Index: wireless-2.6/net/mac80211/rc80211_pid.h
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid.h
+++ wireless-2.6/net/mac80211/rc80211_pid.h
@@ -38,7 +38,7 @@
  * link quality is good, the controller will fail to adjust failed frames
  * percentage to the target. This is intentional.
  */
-#define RC_PID_TARGET_PF (11 << RC_PID_ARITH_SHIFT)
+#define RC_PID_TARGET_PF 11

 /* Rate behaviour normalization quantity over time. */
 #define RC_PID_NORM_OFFSET 3
Index: wireless-2.6/net/mac80211/rc80211_pid_algo.c
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid_algo.c
+++ wireless-2.6/net/mac80211/rc80211_pid_algo.c
@@ -210,7 +210,7 @@ static void rate_control_pid_sample(stru
 	rate_control_pid_normalize(pinfo, mode->num_rates);

 	/* Compute the proportional, integral and derivative errors. */
-	err_prop = pinfo->target - pf;
+	err_prop = (pinfo->target << RC_PID_ARITH_SHIFT) - pf;

 	err_avg = spinfo->err_avg_sc >> pinfo->smoothing_shift;
 	spinfo->err_avg_sc = spinfo->err_avg_sc - err_avg + err_prop;

-- 
Ciao
Stefano

(no subject)

[Mattias Nissler]

Hey,

I'm a little late on this and away from my development machine anyway, =
but when reading my email, I noticed the following and thought this is =
important enough to point out, even though I only have that crappy webm=
ailer and the only kernel tree I can look at is wireless-2.6/everything=
 on git.kernel.org

In rc80211_pid.h, you have the following (this from the wireless-2.6 tr=
ee):

        /* Sampling period for measuring percentage of failed frames. *=
/
        #define RC_PID_INTERVAL (HZ / 8)

But rc80211_pid_algo.c says:

        period =3D (HZ * pinfo->sampling_period + 500) / 1000;
        if (!period)
                period =3D 1;

You see this is completely bogus. My original patch had:

        /* Sampling period for measuring percentage of failed frames in=
 0.001s. */
        #define RC_PID_INTERVAL 1000

You see why what you posted and John commited is bogus?

I'm getting a little fed up with this, you keep screwing up the code wi=
th my name on it *sigh*

And again, I still don't think guarding against period =3D=3D 0 is the =
right thing to do. As I explained privately in IRC, we should make sure=
 the sample_interval is larger than HZ when setting the sample_interval=
 variable  (once nl80211 is done). It doesn't make sense to do this che=
ck every time we execute the code when we can do it once in advance.

Sigh.

Mattias
--=20
Der GMX SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen!=20
Ideal f=FCr Modem und ISDN: http://www.gmx.net/de/go/smartsurfer
-
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re:

[Stefano Brivio]

On Sun, 23 Dec 2007 13:19:51 +0100
"Mattias Nissler" <Mattias.Nissler@gmx.de> wrote:

> Hey,
> 
> I'm a little late on this and away from my development machine anyway, but
> when reading my email, I noticed the following and thought this is
> important enough to point out, even though I only have that crappy
> webmailer and the only kernel tree I can look at is wireless-2.6/everything
> on git.kernel.org
> 
> In rc80211_pid.h, you have the following (this from the wireless-2.6 tree):
> 
>         /* Sampling period for measuring percentage of failed frames. */
>         #define RC_PID_INTERVAL (HZ / 8)
> 
> But rc80211_pid_algo.c says:
> 
>         period = (HZ * pinfo->sampling_period + 500) / 1000;
>         if (!period)
>                 period = 1;
> 
> You see this is completely bogus. My original patch had:
> 
>         /* Sampling period for measuring percentage of failed frames in 0.001s. */
>         #define RC_PID_INTERVAL 1000
> 
> You see why what you posted and John commited is bogus?

Indeed. I didn't notice about this because HZ == 1000 on my laptop. But I
didn't even sign off that patch, I just added a From: on top of it,
and changed this:

+#define RC_PID_INTERVAL (HZ / 1)
[yes, this is present in your original - as sent through IRC - patch]

to:

+#define RC_PID_INTERVAL (HZ / 8)
[and IIRC we discussed about this on IRC]

I'll post a fix for this.

> I'm getting a little fed up with this, you keep screwing up the code with
> my name on it *sigh*

Please, could you show _exactly_ which patches with your name (as in,
From: you) you think I screwed up?

I don't think I screwed up anything yours. I would be very sorry otherwise.

> And again, I still don't think guarding against period == 0 is the right
> thing to do. As I explained privately in IRC, we should make sure the
> sample_interval is larger than HZ when setting the sample_interval
> variable  (once nl80211 is done). It doesn't make sense to do this check
> every time we execute the code when we can do it once in advance.

Exactly, once nl80211 is done. Now, what if the control interval gets
changed after initialization? How often should we check whether it's larger
than HZ? If you got another solution which would work together with the
current debugfs interface, please post it.

> Sigh.

Please stop this. I think that this is:
1) not correct (but I don't care that much);
2) useless;
3) confusing.

Sorry again if I actually screwed up anything.


--
Ciao
Stefano

[PATCH 2/7] rc80211-pid: add kerneldoc for tunable parameters

[Stefano Brivio]

Add a kerneldoc description for parameters which are tunable through debugfs.

Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
---
 rc80211_pid.h |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Index: wireless-2.6/net/mac80211/rc80211_pid.h
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid.h
+++ wireless-2.6/net/mac80211/rc80211_pid.h
@@ -1,5 +1,6 @@
 /*
  * 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
@@ -119,6 +120,29 @@ struct rc_pid_events_file_info {
 	unsigned int next_entry;
 };

+/**
+ * struct rc_pid_debugfs_entries - tunable parameters
+ *
+ * Algorithm parameters, tunable via debugfs.
+ * @dir: the debugfs directory for a specific phy
+ * @target: target percentage for failed frames
+ * @sampling_period: error sampling interval in milliseconds
+ * @coeff_p: absolute value of the proportional coefficient
+ * @coeff_i: absolute value of the integral coefficient
+ * @coeff_d: absolute value of the derivative coefficient
+ * @smoothing_shift: absolute value of the integral smoothing factor (i.e.
+ *	amount of smoothing introduced by the exponential moving average)
+ * @sharpen_factor: absolute value of the derivative sharpening factor (i.e.
+ *	amount of emphasis given to the derivative term after low activity
+ *	events)
+ * @sharpen_duration: duration of the sharpening effect after the detected low
+ *	activity event, relative to sampling_period
+ * @norm_offset: amount of normalization periodically performed on the learnt
+ *	rate behaviour values (lower means we should trust more what we learnt
+ *	about behaviour of rates, higher means we should trust more the natural
+ *	ordering of rates)
+ * @fast_start: if Y, push high rates right after initialization
+ */
 struct rc_pid_debugfs_entries {
 	struct dentry *dir;
 	struct dentry *target;

-- 
Ciao
Stefano

[PATCH 3/7] rc80211-pid: simplify and fix shift_adjust

[Stefano Brivio]

Simplify and fix rate_control_pid_shift_adjust(). A bug prevented correct
mapping of sorted rates, and readability was seriously flawed.

Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
---
 rc80211_pid_algo.c |   34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

Index: wireless-2.6/net/mac80211/rc80211_pid_algo.c
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid_algo.c
+++ wireless-2.6/net/mac80211/rc80211_pid_algo.c
@@ -74,29 +74,27 @@ static int rate_control_pid_shift_adjust
 {
 	int i, j, k, tmp;

-	if (cur + adj < 0)
-		return 0;
-	if (cur + adj >= l)
-		return l - 1;
+	j = r[cur].rev_index;
+	i = j + adj;

-	i = r[cur + adj].rev_index;
+	if (i < 0)
+		return r[0].index;
+	if (i >= l - 1)
+		return r[l - 1].index;

-	j = r[cur].rev_index;
+	tmp = i;

 	if (adj < 0) {
-			tmp = i;
-			for (k = j; k >= i; k--)
-				if (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].diff <= r[i].diff)
-					tmp = k;
-			return r[tmp].index;
+		for (k = j; k >= i; k--)
+			if (r[k].diff <= r[j].diff)
+				tmp = k;
+	} else {
+		for (k = i + 1; k + i < l; k++)
+			if (r[k].diff <= r[i].diff)
+				tmp = k;
 	}
-	return cur + adj;
+
+	return r[tmp].index;
 }

 static void rate_control_pid_adjust_rate(struct ieee80211_local *local,

-- 
Ciao
Stefano

[PATCH 4/7] rc80211-pid: fix sta_info refcounting

[Stefano Brivio]

Fix a bug which caused uncorrect refcounting of PHYs in mac80211. Thanks to
Johannes Berg for spotting this out.

Cc: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
---
 rc80211_pid_algo.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: wireless-2.6/net/mac80211/rc80211_pid_algo.c
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid_algo.c
+++ wireless-2.6/net/mac80211/rc80211_pid_algo.c
@@ -254,7 +254,7 @@ static void rate_control_pid_tx_status(v
 	/* Ignore all frames that were sent with a different rate than the rate
 	 * we currently advise mac80211 to use. */
 	if (status->control.rate != &local->oper_hw_mode->rates[sta->txrate])
-		return;
+		goto ignore;

 	spinfo = sta->rate_ctrl_priv;
 	spinfo->tx_num_xmit++;
@@ -295,6 +295,7 @@ static void rate_control_pid_tx_status(v
 	if (time_after(jiffies, spinfo->last_sample + period))
 		rate_control_pid_sample(pinfo, local, sta);

+ignore:
 	sta_info_put(sta);
 }


-- 
Ciao
Stefano

Re: [PATCH 4/7] rc80211-pid: fix sta_info refcounting

[Johannes Berg]

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


On Sun, 2007-12-23 at 04:43 +0100, Stefano Brivio wrote:
> Fix a bug which caused uncorrect refcounting of PHYs in mac80211. Thanks to
> Johannes Berg for spotting this out.
> 
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>

Yup, this looks like the correct fix.

Acked-by: Johannes Berg <johannes@sipsolutions.net>

> ---
>  rc80211_pid_algo.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: wireless-2.6/net/mac80211/rc80211_pid_algo.c
> ===================================================================
> --- wireless-2.6.orig/net/mac80211/rc80211_pid_algo.c
> +++ wireless-2.6/net/mac80211/rc80211_pid_algo.c
> @@ -254,7 +254,7 @@ static void rate_control_pid_tx_status(v
>  	/* Ignore all frames that were sent with a different rate than the rate
>  	 * we currently advise mac80211 to use. */
>  	if (status->control.rate != &local->oper_hw_mode->rates[sta->txrate])
> -		return;
> +		goto ignore;
> 
>  	spinfo = sta->rate_ctrl_priv;
>  	spinfo->tx_num_xmit++;
> @@ -295,6 +295,7 @@ static void rate_control_pid_tx_status(v
>  	if (time_after(jiffies, spinfo->last_sample + period))
>  		rate_control_pid_sample(pinfo, local, sta);
> 
> +ignore:
>  	sta_info_put(sta);
>  }
> 
> 

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

[PATCH 5/7] rc80211-pid: pf_target tuning

[Stefano Brivio]

Set a better value for percentage target for failed frames. The previous value
slowed down too much rate increases in case of permanently low activity. While
at it, increase readability.

Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
---
 rc80211_pid.h |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

Index: wireless-2.6/net/mac80211/rc80211_pid.h
===================================================================
--- wireless-2.6.orig/net/mac80211/rc80211_pid.h
+++ wireless-2.6/net/mac80211/rc80211_pid.h
@@ -11,41 +11,41 @@
 #define RC80211_PID_H

 /* Sampling period for measuring percentage of failed frames. */
-#define RC_PID_INTERVAL (HZ / 8)
+#define RC_PID_INTERVAL			(HZ / 8)

 /* 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)
+#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 RC_PID_SHARPENING_FACTOR 0
-#define RC_PID_SHARPENING_DURATION 0
+#define RC_PID_SHARPENING_FACTOR	0
+#define RC_PID_SHARPENING_DURATION	0

 /* Fixed point arithmetic shifting amount. */
-#define RC_PID_ARITH_SHIFT 8
+#define RC_PID_ARITH_SHIFT		8

 /* Fixed point arithmetic factor. */
-#define RC_PID_ARITH_FACTOR (1 << RC_PID_ARITH_SHIFT)
+#define RC_PID_ARITH_FACTOR		(1 << RC_PID_ARITH_SHIFT)

 /* Proportional PID component coefficient. */
-#define RC_PID_COEFF_P 15
+#define RC_PID_COEFF_P			15
 /* Integral PID component coefficient. */
-#define RC_PID_COEFF_I 9
+#define RC_PID_COEFF_I			9
 /* Derivative PID component coefficient. */
-#define RC_PID_COEFF_D 15
+#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 the wireless
  * link quality is good, the controller will fail to adjust failed frames
  * percentage to the target. This is intentional.
  */
-#define RC_PID_TARGET_PF 11
+#define RC_PID_TARGET_PF		14

 /* Rate behaviour normalization quantity over time. */
-#define RC_PID_NORM_OFFSET 3
+#define RC_PID_NORM_OFFSET		3

 /* Push high rates right after loading. */
-#define RC_PID_FAST_START 0
+#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) \

-- 
Ciao
Stefano

[PATCH 6/7] rc80211-pid: add MAINTAINERS entry

[Stefano Brivio]

Add an entry in MAINTAINERS for rc80211-pid.

Cc: Mattias Nissler <mattias.nissler@gmx.de>
Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
---
 MAINTAINERS |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: wireless-2.6/MAINTAINERS
===================================================================
--- wireless-2.6.orig/MAINTAINERS
+++ wireless-2.6/MAINTAINERS
@@ -2495,6 +2495,16 @@ W:	http://linuxwireless.org/
 T:	git kernel.org:/pub/scm/linux/kernel/git/linville/wireless-2.6.git
 S:	Maintained

+MAC80211 PID RATE CONTROL
+P:	Stefano Brivio
+M:	stefano.brivio@polimi.it
+P:	Mattias Nissler
+M:	mattias.nissler@gmx.de
+L:	linux-wireless@vger.kernel.org
+W:	http://linuxwireless.org/en/developers/Documentation/mac80211/RateControl/PID
+T:	git kernel.org:/pub/scm/linux/kernel/git/linville/wireless-2.6.git
+S:	Maintained
+
 MACVLAN DRIVER
 P:	Patrick McHardy
 M:	kaber@trash.net

-- 
Ciao
Stefano

[RFC PATCH 7/7] mac80211: fix sta_info locking

[Stefano Brivio]

While tinkering with a sta_info refcounting bug in rc80211-pid algorithm, I
discovered that calling sta_info_get() and then sta_info_put() right after
would cause a kernel panic on my uniprocessor, preemptible kernel. I
couldn't set up netconsole, however, most of the trace is reported below
(my camera did its best, as the trace wouldn't fit on the screen and I
couldn't scroll, so wasn't able to see the first part with naked eyes :).

EIP at delay_tsc+0x22/0x50
[couldn't read the EBX and such, but I guess you won't care]
panic+0xf9/0x100
die+0x1e0/0x1f0
do_page_fault+0x357/0x640
autoremove_wake_function+0x1b/0x50
__wake_up_common+0x3e/0x70
do_page_fault+0x0/0x640
error_code+0x6a/0x70
sta_info_get+0x3a/0x60 [mac80211]
__ieee80211_rx+0x290/0x1830 [mac80211]
skb_queue_tail+0x3b/0x70
ieee80211_rx_irqsafe+0x30/0x80 [mac80211]
ssb_pci_write32+0x22/0x70 [ssb]
ieee80211_tasklet_handler+0xaf/0xe0 [mac80211]
hrtimer_run_queues+0xf6/0x1a0
process_timeout+0x0/0x10
tasklet_action+0x27/0x60
__do_softirq+0x54/0xb0
do_softirq+0x7b/0xe0
handle_level_irq+0x0/0x110
irq_exit+0x30/0x40
do_IRQ+0x83/0xd0
common_interrupt+0x23/0x20
[...]

So I guessed that locking was lacking somewhere. The following patch fixes
the issue for me, but I'm not sure at all that it's the right fix. Thanks.

NOT-Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
---
Index: wireless-2.6/net/mac80211/sta_info.c
===================================================================
--- wireless-2.6.orig/net/mac80211/sta_info.c
+++ wireless-2.6/net/mac80211/sta_info.c
@@ -105,6 +105,7 @@ static void sta_info_release(struct kref
 	struct ieee80211_local *local = sta->local;
 	struct sk_buff *skb;

+	write_lock_bh(&local->sta_lock);
 	/* free sta structure; it has already been removed from
 	 * hash table etc. external structures. Make sure that all
 	 * buffered frames are release (one might have been added
@@ -118,6 +119,8 @@ static void sta_info_release(struct kref
 	}
 	rate_control_free_sta(sta->rate_ctrl, sta->rate_ctrl_priv);
 	rate_control_put(sta->rate_ctrl);
+	write_unlock_bh(&local->sta_lock);
+
 	kfree(sta);
 }

-- 
Ciao
Stefano

Re: [RFC PATCH 7/7] mac80211: fix sta_info locking

[Johannes Berg]

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


> EIP at delay_tsc+0x22/0x50
> [couldn't read the EBX and such, but I guess you won't care]

It'd be good to know actually, along with disassembly.

> So I guessed that locking was lacking somewhere. The following patch fixes
> the issue for me, but I'm not sure at all that it's the right fix. Thanks.

Definitely not. If you look at the code in question, it has nothing at
all that needs locking. It's not accessing any shared structures at all.

johannes

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

Re: [RFC PATCH 7/7] mac80211: fix sta_info locking

[Stefano Brivio]

On Sun, 23 Dec 2007 08:38:56 +0100
Johannes Berg <johannes@sipsolutions.net> wrote:

> > EIP at delay_tsc+0x22/0x50
> > [couldn't read the EBX and such, but I guess you won't care]
> 
> It'd be good to know actually, along with disassembly.
> 
> > So I guessed that locking was lacking somewhere. The following patch fixes
> > the issue for me, but I'm not sure at all that it's the right fix. Thanks.
> 
> Definitely not. If you look at the code in question, it has nothing at
> all that needs locking. It's not accessing any shared structures at all.

I now removed the lock, rebuilt my quilt tree, rebuilt mac80211 and I can't
reproduce it. I think that could have been my fault, I should have did
something wrong such as calling sta_info_put() twice or somesuch. Sorry for
the noise. Will poke you again if I happen to reproduce this.


--
Ciao
Stefano

Re: [RFC PATCH 7/7] mac80211: fix sta_info locking

[Stefano Brivio]

On Sun, 23 Dec 2007 11:18:06 +0100
Stefano Brivio <stefano.brivio@polimi.it> wrote:

> I now removed the lock, rebuilt my quilt tree, rebuilt mac80211 and I can't
> reproduce it. I think that could have been my fault, I should have did
							  ^^^^ must, I meant.


-- 
Ciao
Stefano