Re: [patch 1/9] mac80211: Clean up rate selection code

Re: [patch 1/9] mac80211: Clean up rate selection code

[Johannes Berg]

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

Looks good, minor comment below.

> +void rate_control_get_rate(struct net_device *dev,
> +			   struct ieee80211_hw_mode *mode, struct sk_buff *skb,
> +			   struct rate_selection *sel)

> +	fc = le16_to_cpu(hdr->frame_control);
> +	if ((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA ||
> +	    (hdr->addr1[0] & 0x01))

Please use is_multicast_ether_addr()

> +	/* If a forced rate is in effect, select it. */
> +	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> +	if (sdata->bss && sdata->bss->force_unicast_rateidx > -1)
> +		sel->rate = &mode->rates[sdata->bss->force_unicast_rateidx];
> +
> +	/* If we haven't found the rate yet, ask the rate control algo. */
> +	if (!sel->rate)
> +		ref->ops->get_rate(ref->priv, dev, mode, skb, sel);

Maybe after this we should insert

	if (unlikely(!sel->rate)) {
		WARN_ON(1);
		sel->rate = rate_lowest(...);
	}

Not sure though.

johannes

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

Re: [patch 1/9] mac80211: Clean up rate selection code

[Mattias Nissler]

On Mon, 2007-12-17 at 12:46 +0100, Johannes Berg wrote:
> Looks good, minor comment below.
> 
> > +void rate_control_get_rate(struct net_device *dev,
> > +			   struct ieee80211_hw_mode *mode, struct sk_buff *skb,
> > +			   struct rate_selection *sel)
> 
> > +	fc = le16_to_cpu(hdr->frame_control);
> > +	if ((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA ||
> > +	    (hdr->addr1[0] & 0x01))
> 
> Please use is_multicast_ether_addr()

Sure, we had that before. Seems Stefano gave me an old patch. Sorry
about this.

> 
> > +	/* If a forced rate is in effect, select it. */
> > +	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> > +	if (sdata->bss && sdata->bss->force_unicast_rateidx > -1)
> > +		sel->rate = &mode->rates[sdata->bss->force_unicast_rateidx];
> > +
> > +	/* If we haven't found the rate yet, ask the rate control algo. */
> > +	if (!sel->rate)
> > +		ref->ops->get_rate(ref->priv, dev, mode, skb, sel);
> 
> Maybe after this we should insert
> 
> 	if (unlikely(!sel->rate)) {
> 		WARN_ON(1);
> 		sel->rate = rate_lowest(...);
> 	}
> 
> Not sure though.

I don't think we need this. Rate control is supposed to select a rate,
if it doesn't know it can assign a fallback rate itself.

Mattas

Re: [patch 1/9] mac80211: Clean up rate selection code

[Johannes Berg]

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


> > > +	/* If a forced rate is in effect, select it. */
> > > +	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> > > +	if (sdata->bss && sdata->bss->force_unicast_rateidx > -1)
> > > +		sel->rate = &mode->rates[sdata->bss->force_unicast_rateidx];
> > > +
> > > +	/* If we haven't found the rate yet, ask the rate control algo. */
> > > +	if (!sel->rate)
> > > +		ref->ops->get_rate(ref->priv, dev, mode, skb, sel);
> > 
> > Maybe after this we should insert
> > 
> > 	if (unlikely(!sel->rate)) {
> > 		WARN_ON(1);
> > 		sel->rate = rate_lowest(...);
> > 	}
> > 
> > Not sure though.
> 
> I don't think we need this. Rate control is supposed to select a rate,
> if it doesn't know it can assign a fallback rate itself.

Obviously. I just thought we could protect against buggy rate control
algorithm code that way. Not really necessary though.

johannes

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

Re: [patch 1/9] mac80211: Clean up rate selection code

[Mattias Nissler]

On Tue, 2007-12-18 at 14:17 +0100, Johannes Berg wrote:
> > > > +	/* If a forced rate is in effect, select it. */
> > > > +	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> > > > +	if (sdata->bss && sdata->bss->force_unicast_rateidx > -1)
> > > > +		sel->rate = &mode->rates[sdata->bss->force_unicast_rateidx];
> > > > +
> > > > +	/* If we haven't found the rate yet, ask the rate control algo. */
> > > > +	if (!sel->rate)
> > > > +		ref->ops->get_rate(ref->priv, dev, mode, skb, sel);
> > > 
> > > Maybe after this we should insert
> > > 
> > > 	if (unlikely(!sel->rate)) {
> > > 		WARN_ON(1);
> > > 		sel->rate = rate_lowest(...);
> > > 	}
> > > 
> > > Not sure though.
> > 
> > I don't think we need this. Rate control is supposed to select a rate,
> > if it doesn't know it can assign a fallback rate itself.
> 
> Obviously. I just thought we could protect against buggy rate control
> algorithm code that way. Not really necessary though.

If it's buggy, it should be fixed. So it's actually better to fail
dramatically ;-)

Mattias

Re: [patch 2/9] iwlwifi: Resync rate control code with mac80211

[Johannes Berg]

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


On Mon, 2007-12-17 at 02:25 +0100, Mattias Nissler wrote:
> This brings iwlwifi rate control code back in sync with mac80211.

This, and a patch to fix the "simple" algorithm should be part of the
first patch to avoid breaking bisection.

johannes

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

Re: [patch 2/9] iwlwifi: Resync rate control code with mac80211

[Mattias Nissler]

On Mon, 2007-12-17 at 12:48 +0100, Johannes Berg wrote:
> On Mon, 2007-12-17 at 02:25 +0100, Mattias Nissler wrote:
> > This brings iwlwifi rate control code back in sync with mac80211.
> 
> This, and a patch to fix the "simple" algorithm should be part of the
> first patch to avoid breaking bisection.

Ok, I'll merge them.

Mattias

Re: [patch 3/9] mac80211: Add PID controller based rate control algorithm

[Johannes Berg]

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


> +	/* 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);

That looks... weird.

As far as I know all compilers the kernel can use will do an arithmetic
right shift if the data type is signed.

johannes

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

Re: [patch 3/9] mac80211: Add PID controller based rate control algorithm

[Mattias Nissler]

On Mon, 2007-12-17 at 12:50 +0100, Johannes Berg wrote:
> > +	/* 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);
> 
> That looks... weird.
> 
> As far as I know all compilers the kernel can use will do an arithmetic
> right shift if the data type is signed.

Where is your information from? I actually had some misbehaviour that
was fixed after I introduced this. I'll check the assembly gcc produces
to shed some light on this.

Mattias

Re: [patch 3/9] mac80211: Add PID controller based rate control algorithm

[Johannes Berg]

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


> > As far as I know all compilers the kernel can use will do an arithmetic
> > right shift if the data type is signed.
> 
> Where is your information from? 

Just empirically :)

> I actually had some misbehaviour that
> was fixed after I introduced this. I'll check the assembly gcc produces
> to shed some light on this.

Ok. I like the macro that stefano introduced better than inlining this
code and I can live with that. I know gcc can emit arithmetic right
shifts.

johannes

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

Re: [patch 4/9] mac80211: Make PID rate control algorithm the default

[Johannes Berg]

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


> +config MAC80211_RCSIMPLE
> +	bool "'simple' rate control algorithm (DEPRECATED)"
> +	default n
> +	depends on MAC80211
> +	---help---

I'm pretty sure just "help" is preferred style these days.

And I already said that I'd rather see a single patch that replaces
_SIMPLE with _PID.

I'd split up the patch series as follows:
 1) API cleanup, including all rate control algorithms
 2) PID replace SIMPLE
 3..N) PID improvements

Although since this is all new code 2..N should probably be collapsed
into a single patch.

johannes

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

Re: [patch 4/9] mac80211: Make PID rate control algorithm the default

[Mattias Nissler]

On Mon, 2007-12-17 at 12:52 +0100, Johannes Berg wrote:
> > +config MAC80211_RCSIMPLE
> > +	bool "'simple' rate control algorithm (DEPRECATED)"
> > +	default n
> > +	depends on MAC80211
> > +	---help---
> 
> I'm pretty sure just "help" is preferred style these days.
> 
> And I already said that I'd rather see a single patch that replaces
> _SIMPLE with _PID.
> 
> I'd split up the patch series as follows:
>  1) API cleanup, including all rate control algorithms
>  2) PID replace SIMPLE

These two refer to stuff I'm happy with.

>  3..N) PID improvements

These are patches that IMHO need further testing. They extend the PID
controller concept in a non-standard way. Stefano says they help in some
situations.

> 
> Although since this is all new code 2..N should probably be collapsed
> into a single patch.

See the comment above for why they're separate patches at the time of
writing this.

Mattias

Re: [patch 4/9] mac80211: Make PID rate control algorithm the default

[John W. Linville]

On Mon, Dec 17, 2007 at 02:25:21AM +0100, Mattias Nissler wrote:

>  config MAC80211_RCPID
> -	bool "'PID' rate control algorithm" if EMBEDDED
> +	bool "'PID' rate control algorithm"

Shouldn't this keep the "if EMBEDDED"?  Especially if it is going to be the default?

> --- rt2x00.orig/net/mac80211/ieee80211_rate.c
> +++ rt2x00/net/mac80211/ieee80211_rate.c
> @@ -97,7 +97,7 @@ ieee80211_rate_control_ops_get(const cha
>  	struct rate_control_ops *ops;
>  
>  	if (!name)
> -		name = "simple";
> +		name = "pid";

As pointed-out elsewhere, we have no way to select "simple" (without
hacking and rebuilding a driver).  Perhaps a module option would
be appropriate?

> --- rt2x00.orig/Documentation/feature-removal-schedule.txt
> +++ rt2x00/Documentation/feature-removal-schedule.txt
> @@ -49,8 +49,8 @@ Why:	V4L1 AP1 was replaced by V4L2 API d
>  	Decoder iocts are using internally to allow video drivers to
>  	communicate with video decoders. This should also be improved to allow
>  	V4L2 calls being translated into compatible internal ioctls.
> -	Compatibility ioctls will be provided, for a while, via 
> -	v4l1-compat module. 
> +	Compatibility ioctls will be provided, for a while, via
> +	v4l1-compat module.
>  Who:	Mauro Carvalho Chehab <mchehab@infradead.org>
>  
>  ---------------------------
> @@ -308,7 +308,7 @@ Who:	Roland Dreier <rolandd@cisco.com>
>  What:   sk98lin network driver
>  When:   Feburary 2008
>  Why:    In kernel tree version of driver is unmaintained. Sk98lin driver
> -	replaced by the skge driver. 
> +	replaced by the skge driver.
>  Who:    Stephen Hemminger <shemminger@linux-foundation.org>
>  
>  ---------------------------
> @@ -343,10 +343,19 @@ Who:	John W. Linville <linville@tuxdrive
>  
>  ---------------------------
>  
> -What:	iee80211 softmac wireless networking component
> +What:	ieee80211 softmac wireless networking component
>  When:	2.6.26 (or after removal of bcm43xx and port of zd1211rw to mac80211)
>  Files:	net/ieee80211/softmac
>  Why:	No in-kernel drivers will depend on it any longer.
>  Who:	John W. Linville <linville@tuxdriver.com>

These typo fixes seem fine, but they should be in a seperate patch.

I can manipulate the feature removal bits if you don't want to bother
with resubmitting.  Any thoughts on the other comments?

Thanks,

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [patch 4/9] mac80211: Make PID rate control algorithm the default

[Stefano Brivio]

On Mon, 17 Dec 2007 10:02:02 -0500
"John W. Linville" <linville@tuxdriver.com> wrote:

> On Mon, Dec 17, 2007 at 02:25:21AM +0100, Mattias Nissler wrote:
> 
> >  config MAC80211_RCPID
> > -	bool "'PID' rate control algorithm" if EMBEDDED
> > +	bool "'PID' rate control algorithm"
> 
> Shouldn't this keep the "if EMBEDDED"?  Especially if it is going to be the default?

How is EMBEDDED supposed to be related to this? Anyway, I'll go for the two
modules approach, forcing one module to be selected.

> > -What:	iee80211 softmac wireless networking component
> > +What:	ieee80211 softmac wireless networking component
> >  When:	2.6.26 (or after removal of bcm43xx and port of zd1211rw to mac80211)
> >  Files:	net/ieee80211/softmac
> >  Why:	No in-kernel drivers will depend on it any longer.
> >  Who:	John W. Linville <linville@tuxdriver.com>
> 
> These typo fixes seem fine, but they should be in a seperate patch.

I actually thought that this was a common and accepted practice. A separate
patch will be sent.


--
Ciao
Stefano

Re: [patch 4/9] mac80211: Make PID rate control algorithm the default

[John W. Linville]

On Mon, Dec 17, 2007 at 09:06:49PM +0100, Stefano Brivio wrote:
> On Mon, 17 Dec 2007 10:02:02 -0500
> "John W. Linville" <linville@tuxdriver.com> wrote:
> 
> > On Mon, Dec 17, 2007 at 02:25:21AM +0100, Mattias Nissler wrote:
> > 
> > >  config MAC80211_RCPID
> > > -	bool "'PID' rate control algorithm" if EMBEDDED
> > > +	bool "'PID' rate control algorithm"
> > 
> > Shouldn't this keep the "if EMBEDDED"?  Especially if it is going to be the default?
> 
> How is EMBEDDED supposed to be related to this? Anyway, I'll go for the two
> modules approach, forcing one module to be selected.

As I recall, the "if EMBEDDED" was there basically to say "it should
always be built-in, but if you are trying to totally minimize your
footprint you can turn it off".  Of course, that still would only
work if you had another rate control algorithm available e.g. the
iwlwifi drivers.

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [patch 4/9] mac80211: Make PID rate control algorithm the default

[Stefano Brivio]

On Mon, 17 Dec 2007 15:41:12 -0500
"John W. Linville" <linville@tuxdriver.com> wrote:

> On Mon, Dec 17, 2007 at 09:06:49PM +0100, Stefano Brivio wrote:
> > On Mon, 17 Dec 2007 10:02:02 -0500
> > "John W. Linville" <linville@tuxdriver.com> wrote:
> > 
> > > On Mon, Dec 17, 2007 at 02:25:21AM +0100, Mattias Nissler wrote:
> > > 
> > > >  config MAC80211_RCPID
> > > > -	bool "'PID' rate control algorithm" if EMBEDDED
> > > > +	bool "'PID' rate control algorithm"
> > > 
> > > Shouldn't this keep the "if EMBEDDED"?  Especially if it is going to be the default?
> > 
> > How is EMBEDDED supposed to be related to this? Anyway, I'll go for the two
> > modules approach, forcing one module to be selected.
> 
> As I recall, the "if EMBEDDED" was there basically to say "it should
> always be built-in, but if you are trying to totally minimize your
> footprint you can turn it off".  Of course, that still would only
> work if you had another rate control algorithm available e.g. the
> iwlwifi drivers.

Ok, this would make sense. But think about users which doesn't want rate
control on non-embedded systems (laptop and AP always in the same room, or
AP which only acts as a bridge between one client and a dial-up modem).
Rate control is useless here.

I would take this approach: make them selectable (as two modules), and put
a huge warning in the help message (something like "you should need to
select at least one rate control algorithm unless you really don't want
it").


--
Ciao
Stefano

Re: [patch 4/9] mac80211: Make PID rate control algorithm the default

[Johannes Berg]

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


> I would take this approach: make them selectable (as two modules), and put
> a huge warning in the help message (something like "you should need to
> select at least one rate control algorithm unless you really don't want
> it").

Please always build one into mac80211 to avoid having the issue again
where people suck and blame us.

johannes

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

Re: [patch 4/9] mac80211: Make PID rate control algorithm the default

[Mattias Nissler]

Hi,

below is a new patch that I'd be ok to go with. It builds both pid and
simple as modules. A Kconfig choice makes sure one of them is always
built. Furthermore, there is a modparam that can be used to override the
default algo to use.

The only issue I know about is that if people disable module auto
loading, they'll end up without rate control algorithm. If you don't
like this, please comment on how to fix it.

Mattias



This makes the new PID TX rate control algorithm the default instead of the
rc80211_simple rate control algorithm. The simple algorithm was flawed in
several ways: It wasn't responsive at all and didn't age the information it was
relying on properly. The PID algorithm allows us to tune characteristics such
as responsiveness by adjusting parameters and was found to generally behave
better.

Signed-off-by: Mattias Nissler <mattias.nissler@gmx.de>
Signed-off-by: Stefano Brivio <stefano.brivio@polimi.it>
---
 net/mac80211/Kconfig          |   12 --
 net/mac80211/Makefile         |    1 -
 net/mac80211/ieee80211.c      |   12 --
 net/mac80211/ieee80211_rate.c |    2 +-
 net/mac80211/ieee80211_rate.h |    3 -
 net/mac80211/rc80211_simple.c |  366 -----------------------------------------
 6 files changed, 1 insertions(+), 395 deletions(-)
 delete mode 100644 net/mac80211/rc80211_simple.c

Index: rt2x00/net/mac80211/Kconfig
===================================================================
--- rt2x00.orig/net/mac80211/Kconfig
+++ rt2x00/net/mac80211/Kconfig
@@ -13,20 +13,40 @@ config MAC80211
 	This option enables the hardware independent IEEE 802.11
 	networking stack.
 
-config MAC80211_RCSIMPLE
-	bool "'simple' rate control algorithm" if EMBEDDED
-	default y
+choice
+	prompt "Default rate control algorithm"
+	default MAC80211_RC_DEFAULT_PID
+	depends on MAC80211
+	help
+	This option selects the default rate control algorithm mac80211 will
+	use. Note that this default can still be overriden by the
+	ieee80211_default_rc_algo module parameter.
+
+config MAC80211_RC_DEFAULT_PID
+	bool "PID controller based rate control algorithm"
+	depends on MAC80211
+	select MAC80211_RC_PID
+	help
+	Select the PID controller based rate control as default rate control
+	algorithm.
+
+config MAC80211_RC_DEFAULT_SIMPLE
+	bool "Simple rate control algorithm"
 	depends on MAC80211
+	select MAC80211_RC_SIMPLE
 	help
-	  This option allows you to turn off the 'simple' rate
-	  control algorithm in mac80211. If you do turn it off,
-	  you absolutely need another rate control algorithm.
+	Select the simple rate control as default rate control algorithm.
 
-	  Say Y unless you know you will have another algorithm
-	  available.
+endchoice
 
-config MAC80211_RCPID
-	bool "'PID' rate control algorithm" if EMBEDDED
+config MAC80211_RC_DEFAULT
+	string
+	depends on MAC80211
+	default "pid" if MAC80211_RC_DEFAULT_PID
+	default "simple" if MAC80211_RC_DEFAULT_SIMPLE
+
+config MAC80211_RC_PID
+	tristate "PID controller based rate control algorithm"
 	default y
 	depends on MAC80211
 	help
@@ -37,6 +57,18 @@ config MAC80211_RCPID
 	  Say Y unless you're sure you want to use a different
 	  rate control algorithm.
 
+config MAC80211_RC_SIMPLE
+	tristate "Simple rate control algorithm (DEPRECATED)"
+	default n
+	depends on MAC80211
+	help
+	  This option enables a very simple, non-responsive TX
+	  rate control algorithm. This algorithm is deprecated
+	  and will be removed from the kernel in near future.
+	  It has been replaced by the PID algorithm.
+
+	  Say N unless you know what you are doing.
+
 config MAC80211_LEDS
 	bool "Enable LED triggers"
 	depends on MAC80211 && LEDS_TRIGGERS
Index: rt2x00/net/mac80211/Makefile
===================================================================
--- rt2x00.orig/net/mac80211/Makefile
+++ rt2x00/net/mac80211/Makefile
@@ -1,10 +1,10 @@
 obj-$(CONFIG_MAC80211) += mac80211.o
+obj-$(CONFIG_MAC80211_RC_PID) += rc80211_pid.o
+obj-$(CONFIG_MAC80211_RC_SIMPLE) += rc80211_simple.o
 
 mac80211-objs-$(CONFIG_MAC80211_LEDS) += ieee80211_led.o
 mac80211-objs-$(CONFIG_MAC80211_DEBUGFS) += debugfs.o debugfs_sta.o debugfs_netdev.o debugfs_key.o
 mac80211-objs-$(CONFIG_NET_SCHED) += wme.o
-mac80211-objs-$(CONFIG_MAC80211_RCSIMPLE) += rc80211_simple.o
-mac80211-objs-$(CONFIG_MAC80211_RCPID) += rc80211_pid.o
 
 mac80211-objs := \
 	ieee80211.o \
Index: rt2x00/net/mac80211/ieee80211.c
===================================================================
--- rt2x00.orig/net/mac80211/ieee80211.c
+++ rt2x00/net/mac80211/ieee80211.c
@@ -1313,51 +1313,21 @@ static int __init ieee80211_init(void)
 
 	BUILD_BUG_ON(sizeof(struct ieee80211_tx_packet_data) > sizeof(skb->cb));
 
-#ifdef CONFIG_MAC80211_RCSIMPLE
-	ret = ieee80211_rate_control_register(&mac80211_rcsimple);
-	if (ret)
-		goto fail;
-#endif
-
-#ifdef CONFIG_MAC80211_RCPID
-	ret = ieee80211_rate_control_register(&mac80211_rcpid);
-	if (ret)
-		goto fail;
-#endif
-
 	ret = ieee80211_wme_register();
 	if (ret) {
 		printk(KERN_DEBUG "ieee80211_init: failed to "
 		       "initialize WME (err=%d)\n", ret);
-		goto fail;
+		return ret;
 	}
 
 	ieee80211_debugfs_netdev_init();
 	ieee80211_regdomain_init();
 
 	return 0;
-
-fail:
-
-#ifdef CONFIG_MAC80211_RCSIMPLE
-	ieee80211_rate_control_unregister(&mac80211_rcsimple);
-#endif
-#ifdef CONFIG_MAC80211_RCPID
-	ieee80211_rate_control_unregister(&mac80211_rcpid);
-#endif
-
-	return ret;
 }
 
 static void __exit ieee80211_exit(void)
 {
-#ifdef CONFIG_MAC80211_RCSIMPLE
-	ieee80211_rate_control_unregister(&mac80211_rcsimple);
-#endif
-#ifdef CONFIG_MAC80211_RCPID
-	ieee80211_rate_control_unregister(&mac80211_rcpid);
-#endif
-
 	ieee80211_wme_unregister();
 	ieee80211_debugfs_netdev_exit();
 }
Index: rt2x00/net/mac80211/ieee80211_rate.c
===================================================================
--- rt2x00.orig/net/mac80211/ieee80211_rate.c
+++ rt2x00/net/mac80211/ieee80211_rate.c
@@ -21,6 +21,11 @@ struct rate_control_alg {
 static LIST_HEAD(rate_ctrl_algs);
 static DEFINE_MUTEX(rate_ctrl_mutex);
 
+static char *ieee80211_default_rc_algo = CONFIG_MAC80211_RC_DEFAULT;
+module_param(ieee80211_default_rc_algo, charp, 0444);
+MODULE_PARM_DESC(ieee80211_default_rc_algo,
+		 "Default rate control algorithm for mac80211 to use");
+
 int ieee80211_rate_control_register(struct rate_control_ops *ops)
 {
 	struct rate_control_alg *alg;
@@ -97,7 +102,7 @@ ieee80211_rate_control_ops_get(const cha
 	struct rate_control_ops *ops;
 
 	if (!name)
-		name = "simple";
+		name = ieee80211_default_rc_algo;
 
 	ops = ieee80211_try_rate_control_ops_get(name);
 	if (!ops) {
@@ -244,3 +249,4 @@ void rate_control_deinitialize(struct ie
 	local->rate_ctrl = NULL;
 	rate_control_put(ref);
 }
+
Index: rt2x00/net/mac80211/ieee80211_rate.h
===================================================================
--- rt2x00.orig/net/mac80211/ieee80211_rate.h
+++ rt2x00/net/mac80211/ieee80211_rate.h
@@ -58,12 +58,6 @@ struct rate_control_ref {
 	struct kref kref;
 };
 
-/* default 'simple' algorithm */
-extern struct rate_control_ops mac80211_rcsimple;
-
-/* 'PID' algorithm */
-extern struct rate_control_ops mac80211_rcpid;
-
 int ieee80211_rate_control_register(struct rate_control_ops *ops);
 void ieee80211_rate_control_unregister(struct rate_control_ops *ops);
 
Index: rt2x00/Documentation/feature-removal-schedule.txt
===================================================================
--- rt2x00.orig/Documentation/feature-removal-schedule.txt
+++ rt2x00/Documentation/feature-removal-schedule.txt
@@ -350,3 +350,12 @@ Why:	No in-kernel drivers will depend on
 Who:	John W. Linville <linville@tuxdriver.com>
 
 ---------------------------
+
+What:	rc80211-simple rate control algorithm for mac80211
+When:	2.6.26
+Files:	net/mac80211/rc80211-simple.c
+Why:	This algorithm was provided for reference but always exhibited bad
+	responsiveness and performance and has some serious flaws. It has been
+	replaced by rc80211-pid.
+Who:	Stefano Brivio <stefano.brivio@polimi.it>
+

Re: [patch 4/9] mac80211: Make PID rate control algorithm the default

[Johannes Berg]

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


> The only issue I know about is that if people disable module auto
> loading, they'll end up without rate control algorithm. If you don't
> like this, please comment on how to fix it.

That's exactly the issue we had before hence we started building one
into mac80211. Please build the default one into mac80211 and the other
one as a module *and* fall back to the built-in one when the default was
changed via debugfs but that one is not available.

johannes

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

Re: [patch 7/9] mac80211: Dump rc80211_pid events to debugfs

[Johannes Berg]

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

 
> +#ifdef CONFIG_MAC80211_DEBUGFS
> +#include "rc80211_pid_debugfs.h"
> +#endif
> +#include "rc80211_pid.h"

Don't ifdef includes. Also, I think it's overkill to have two header
files for a single rate control algorithm, just use a single combined
one.

johannes

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

Re: [patch 7/9] mac80211: Dump rc80211_pid events to debugfs

[Mattias Nissler]

On Mon, 2007-12-17 at 12:57 +0100, Johannes Berg wrote:
> > +#ifdef CONFIG_MAC80211_DEBUGFS
> > +#include "rc80211_pid_debugfs.h"
> > +#endif
> > +#include "rc80211_pid.h"
> 
> Don't ifdef includes. Also, I think it's overkill to have two header
> files for a single rate control algorithm, just use a single combined
> one.

Will do.

Mattias

Re: [patch 9/9] mac80211: Publish rc80211_pid parameters in debugfs

[Johannes Berg]

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


> +	period = (HZ / 1000) * pinfo->sampling_period;

That isn't going to work correctly with HZ < 1000 because the
parenthesised expression is going to be zero.

johannes

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

Re: [patch 9/9] mac80211: Publish rc80211_pid parameters in debugfs

[Mattias Nissler]

On Mon, 2007-12-17 at 13:02 +0100, Johannes Berg wrote:
> > +	period = (HZ / 1000) * pinfo->sampling_period;
> 
> That isn't going to work correctly with HZ < 1000 because the
> parenthesised expression is going to be zero.

Right. There is another line in the patches that gets this wrong.

Mattias

[patch 0/9] Rate control rework

[mattias.nissler]

Hi,

this patchset is the result of my recent joint-venture with Stefano. It
introduces a new rate control algorithm based on a PID controller. First
tests show it works much better than the old simple rate control
algorithm.

We think the current patch set is clean enough so we can push it into
2.6.25. However, there are some issues that need some more work:

* The algorithm parameters need more tuning. The current defaults are
  sane, but we hope to get a larger testing audience once this is in the
  tree. Once there are more test results, tuning should be easier.

* It seems mac80211 fails to clean up the rate control algorithm. This
  results in the debugfs entries created by patch 9 not being removed.

* Patch number 8 is actually a change to the debugfs code so I can add
  files for signed variables. I've sent it to Greg KH and LKML, let's
  see what happens. If it doesn't make 2.6.25, we could add similar
  infrastructure locally.

Hm, anything I forgot? Stefano?

Mattias

[patch 9/9] mac80211: Publish rc80211_pid parameters in debugfs

[mattias.nissler]

This adds all the tuneable parameters used by rc80211_pid to debugfs for easy
testing and tuning.

Signed-off-by: Mattias Nissler <mattias.nissler@gmx.de>
---
 net/mac80211/rc80211_pid.c |   77 +++++++++++++++++++++++++++++++++++++-------
 1 files changed, 65 insertions(+), 12 deletions(-)

Index: rt2x00/net/mac80211/rc80211_pid.c
===================================================================
--- rt2x00.orig/net/mac80211/rc80211_pid.c
+++ rt2x00/net/mac80211/rc80211_pid.c
@@ -66,16 +66,16 @@
  * RC_PID_ARITH_SHIFT.
  */
 
-/* Sampling period for measuring percentage of failed frames. */
-#define RC_PID_INTERVAL (HZ / 1)
+/* Sampling period for measuring percentage of failed frames in 0.001s. */
+#define RC_PID_INTERVAL 1000
 
 /* 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 RC_PID_SHARPENING_FACTOR 2
-#define RC_PID_SHARPENING_DURATION 1
+#define RC_PID_SHARPENING_FACTOR 0
+#define RC_PID_SHARPENING_DURATION 0
 
 /* Fixed point arithmetic shifting amount. */
 #define RC_PID_ARITH_SHIFT 8
@@ -84,11 +84,11 @@
 #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 20
 /* Integral PID component coefficient. */
 #define RC_PID_COEFF_I 10
 /* Derivative PID component coefficient. */
-#define RC_PID_COEFF_D 15
+#define RC_PID_COEFF_D 0
 
 /* Target failed frames rate for the PID controller. NB: This effectively gives
  * maximum failed frames percentage we're willing to accept. If the wireless
@@ -185,20 +185,21 @@ 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 *pinfo, int l)
 {
-	int i;
+	int i, norm_offset = pinfo->norm_offset;
+	struct rc_pid_rateinfo *r = pinfo->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;
+	if (r[0].diff > norm_offset)
+		r[0].diff -= norm_offset;
+	else if (r[0].diff < -norm_offset)
+		r[0].diff += 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;
+			if (r[i + 1].diff > r[i].diff + norm_offset)
+				r[i + 1].diff -= norm_offset;
 			else if (r[i + 1].diff <= r[i].diff)
-				r[i + 1].diff += RC_PID_NORM_OFFSET;
+				r[i + 1].diff += norm_offset;
 		}
 }
 
@@ -215,14 +216,16 @@ static void rate_control_pid_sample(stru
 	s32 err_int;
 	s32 err_der;
 	int adj, i, j, tmp;
+	unsigned long period;
 
 	mode = local->oper_hw_mode;
 	spinfo = sta->rate_ctrl_priv;
 
 	/* In case nothing happened during the previous control interval, turn
 	 * the sharpening factor on. */
-	if (jiffies - spinfo->last_sample > 2 * RC_PID_INTERVAL)
-		spinfo->sharp_cnt = RC_PID_SHARPENING_DURATION;
+	period = (HZ / 1000) * pinfo->sampling_period;
+	if (jiffies - spinfo->last_sample > 2 * period)
+		spinfo->sharp_cnt = pinfo->sharpen_duration;
 
 	spinfo->last_sample = jiffies;
 
@@ -253,17 +256,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 = pinfo->target - pf;
 
-	err_avg = spinfo->err_avg_sc >> RC_PID_SMOOTHING_SHIFT;
+	err_avg = spinfo->err_avg_sc >> pinfo->smoothing_shift;
 	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->smoothing_shift;
 
-	err_der = pf - spinfo->last_pf
-		  * (1 + RC_PID_SHARPENING_FACTOR * spinfo->sharp_cnt);
+	err_der = (pf - spinfo->last_pf) *
+		  (1 + pinfo->sharpen_factor * spinfo->sharp_cnt);
 	spinfo->last_pf = pf;
 	if (spinfo->sharp_cnt)
 			spinfo->sharp_cnt--;
@@ -292,6 +295,7 @@ static void rate_control_pid_tx_status(v
 	struct rc_pid_info *pinfo = priv;
 	struct sta_info *sta;
 	struct rc_pid_sta_info *spinfo;
+	unsigned long period;
 
 	sta = sta_info_get(local, hdr->addr1);
 
@@ -336,7 +340,8 @@ 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))
+	period = (HZ / 1000) * pinfo->sampling_period;
+	if (time_after(jiffies, spinfo->last_sample + period))
 		rate_control_pid_sample(pinfo, local, sta);
 
 	sta_info_put(sta);
@@ -394,6 +399,9 @@ static void *rate_control_pid_alloc(stru
 	struct ieee80211_hw_mode *mode;
 	int i, j, tmp;
 	bool s;
+#ifdef CONFIG_MAC80211_DEBUGFS
+	struct rc_pid_debugfs_entries *de;
+#endif
 
 	pinfo = kmalloc(sizeof(*pinfo), GFP_ATOMIC);
 	if (!pinfo)
@@ -414,7 +422,7 @@ static void *rate_control_pid_alloc(stru
 	for (i = 0; i < mode->num_rates; i++) {
 		rinfo[i].index = i;
 		rinfo[i].rev_index = i;
-		if (RC_PID_FAST_START) {
+		if (pinfo->fast_start) {
 			rinfo[i].valid = 1;
 			rinfo[i].diff = 0;
 		} else
@@ -440,18 +448,72 @@ static void *rate_control_pid_alloc(stru
 	rinfo[0].valid = 1;
 
 	pinfo->target = RC_PID_TARGET_PF;
+	pinfo->sampling_period = RC_PID_INTERVAL;
 	pinfo->coeff_p = RC_PID_COEFF_P;
 	pinfo->coeff_i = RC_PID_COEFF_I;
 	pinfo->coeff_d = RC_PID_COEFF_D;
+	pinfo->smoothing_shift = RC_PID_SMOOTHING_SHIFT;
+	pinfo->sharpen_factor = RC_PID_SHARPENING_FACTOR;
+	pinfo->sharpen_duration = RC_PID_SHARPENING_DURATION;
+	pinfo->norm_offset = RC_PID_NORM_OFFSET;
+	pinfo->fast_start = RC_PID_FAST_START;
 	pinfo->rinfo = rinfo;
 	pinfo->oldrate = 0;
 
+#ifdef CONFIG_MAC80211_DEBUGFS
+	de = &pinfo->dentries;
+	de->dir = debugfs_create_dir("rc80211_pid",
+				     local->hw.wiphy->debugfsdir);
+	de->target = debugfs_create_u32("target_pf", S_IRUSR | S_IWUSR,
+					de->dir, &pinfo->target);
+	de->sampling_period = debugfs_create_u32("sampling_period",
+						 S_IRUSR | S_IWUSR, de->dir,
+						 &pinfo->sampling_period);
+	de->coeff_p = debugfs_create_s32("coeff_p", S_IRUSR | S_IWUSR,
+					 de->dir, &pinfo->coeff_p);
+	de->coeff_i = debugfs_create_s32("coeff_i", S_IRUSR | S_IWUSR,
+					 de->dir, &pinfo->coeff_i);
+	de->coeff_d = debugfs_create_s32("coeff_d", S_IRUSR | S_IWUSR,
+					 de->dir, &pinfo->coeff_d);
+	de->smoothing_shift = debugfs_create_u32("smoothing_shift",
+						 S_IRUSR | S_IWUSR, de->dir,
+						 &pinfo->smoothing_shift);
+	de->sharpen_factor = debugfs_create_u32("sharpen_factor",
+					       S_IRUSR | S_IWUSR, de->dir,
+					       &pinfo->sharpen_factor);
+	de->sharpen_duration = debugfs_create_u32("sharpen_duration",
+						  S_IRUSR | S_IWUSR, de->dir,
+						  &pinfo->sharpen_duration);
+	de->norm_offset = debugfs_create_u32("norm_offset",
+					     S_IRUSR | S_IWUSR, de->dir,
+					     &pinfo->norm_offset);
+	de->fast_start = debugfs_create_bool("fast_start",
+					     S_IRUSR | S_IWUSR, de->dir,
+					     &pinfo->fast_start);
+#endif
+
 	return pinfo;
 }
 
 static void rate_control_pid_free(void *priv)
 {
 	struct rc_pid_info *pinfo = priv;
+#ifdef CONFIG_MAC80211_DEBUGFS
+	struct rc_pid_debugfs_entries *de = &pinfo->dentries;
+
+	debugfs_remove(de->fast_start);
+	debugfs_remove(de->norm_offset);
+	debugfs_remove(de->sharpen_duration);
+	debugfs_remove(de->sharpen_factor);
+	debugfs_remove(de->smoothing_shift);
+	debugfs_remove(de->coeff_d);
+	debugfs_remove(de->coeff_i);
+	debugfs_remove(de->coeff_p);
+	debugfs_remove(de->sampling_period);
+	debugfs_remove(de->target);
+	debugfs_remove(de->dir);
+#endif
+
 	kfree(pinfo->rinfo);
 	kfree(pinfo);
 }
Index: rt2x00/net/mac80211/rc80211_pid_debugfs.h
===================================================================
--- rt2x00.orig/net/mac80211/rc80211_pid_debugfs.h
+++ rt2x00/net/mac80211/rc80211_pid_debugfs.h
@@ -9,6 +9,20 @@
 #ifndef RC80211_PID_DEBUGFS_H
 #define RC80211_PID_DEBUGFS_H
 
+struct rc_pid_debugfs_entries {
+	struct dentry *dir;
+	struct dentry *target;
+	struct dentry *sampling_period;
+	struct dentry *coeff_p;
+	struct dentry *coeff_i;
+	struct dentry *coeff_d;
+	struct dentry *smoothing_shift;
+	struct dentry *sharpen_factor;
+	struct dentry *sharpen_duration;
+	struct dentry *norm_offset;
+	struct dentry *fast_start;
+};
+
 enum rc_pid_event_type {
 	RC_PID_EVENT_TYPE_TX_STATUS,
 	RC_PID_EVENT_TYPE_RATE_CHANGE,
Index: rt2x00/net/mac80211/rc80211_pid.h
===================================================================
--- rt2x00.orig/net/mac80211/rc80211_pid.h
+++ rt2x00/net/mac80211/rc80211_pid.h
@@ -94,8 +94,8 @@ struct rc_pid_info {
 	/* Exponential averaging shift. */
 	unsigned int smoothing_shift;
 
-	/* Sharpening shift and duration. */
-	unsigned int sharpen_shift;
+	/* Sharpening factor and duration. */
+	unsigned int sharpen_factor;
 	unsigned int sharpen_duration;
 
 	/* Normalization offset. */
@@ -109,6 +109,11 @@ struct rc_pid_info {
 
 	/* Index of the last used rate. */
 	int oldrate;
+
+#ifdef CONFIG_MAC80211_DEBUGFS
+	/* Debugfs entries created for the parameters above. */
+	struct rc_pid_debugfs_entries dentries;
+#endif
 };
 
 #endif /* RC80211_PID_H */

--