RFC: always enable MAC80211_RC_PID?

RFC: always enable MAC80211_RC_PID?

[Adrian Bunk]

After the scheduled removal of MAC80211_RC_SIMPLE only 
MAC80211_RC_DEFAULT_PID and MAC80211_RC_DEFAULT_NONE are
left as choices.

Does MAC80211_RC_DEFAULT_NONE have any userbase serious enough to 
justify all the constructs made for this case or could we simply
always enable MAC80211_RC_PID?

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: RFC: always enable MAC80211_RC_PID?

[Luis R. Rodriguez]

On Sun, Jun 22, 2008 at 6:25 PM, Adrian Bunk <bunk@stusta.de> wrote:
> After the scheduled removal of MAC80211_RC_SIMPLE only
> MAC80211_RC_DEFAULT_PID and MAC80211_RC_DEFAULT_NONE are
> left as choices.
>
> Does MAC80211_RC_DEFAULT_NONE have any userbase serious enough to
> justify all the constructs made for this case or could we simply
> always enable MAC80211_RC_PID?

You can currently only get away living without it if you are just
using iwl3945 or iwl4965 as they provide their own rate control
algorithms. In the future if other vendor drivers are added with their
own rate control algorithm this list grows. Disabling PID will save
you some space only on some embedded platforms. Although currently its
not evident people would do this I do see this being a reasonable
option to keep around for later.

  Luis

Re: RFC: always enable MAC80211_RC_PID?

[Kalle Valo]

Luis R. Rodriguez <mcgrof@gmail.com> writes:

> On Sun, Jun 22, 2008 at 6:25 PM, Adrian Bunk <bunk@stusta.de> wrote:
>> After the scheduled removal of MAC80211_RC_SIMPLE only
>> MAC80211_RC_DEFAULT_PID and MAC80211_RC_DEFAULT_NONE are
>> left as choices.
>>
>> Does MAC80211_RC_DEFAULT_NONE have any userbase serious enough to
>> justify all the constructs made for this case or could we simply
>> always enable MAC80211_RC_PID?
>
> You can currently only get away living without it if you are just
> using iwl3945 or iwl4965 as they provide their own rate control
> algorithms. In the future if other vendor drivers are added with their
> own rate control algorithm this list grows. Disabling PID will save
> you some space only on some embedded platforms. Although currently its
> not evident people would do this I do see this being a reasonable
> option to keep around for later.

I agree with Luis. There is a need for MAC80211_RC_DEFAULT_NONE,
please do not remove it.

-- 
Kalle Valo

Re: RFC: always enable MAC80211_RC_PID?

[Johannes Berg]

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


> You can currently only get away living without it if you are just
> using iwl3945 or iwl4965 as they provide their own rate control
> algorithms. In the future if other vendor drivers are added with their
> own rate control algorithm this list grows.

FWIW, I would like to see those algorithms be supported as part of
mac80211 and not be internal like iwlwifi hacked them on, and have some
sort of feature negotiation between the hardware and the RC algorithm
when choosing one.

johannes

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

Re: RFC: always enable MAC80211_RC_PID?

[Adrian Bunk]

On Tue, Jun 24, 2008 at 11:04:19AM +0200, Johannes Berg wrote:
> 
> > You can currently only get away living without it if you are just
> > using iwl3945 or iwl4965 as they provide their own rate control
> > algorithms. In the future if other vendor drivers are added with their
> > own rate control algorithm this list grows.
> 
> FWIW, I would like to see those algorithms be supported as part of
> mac80211 and not be internal like iwlwifi hacked them on, and have some
> sort of feature negotiation between the hardware and the RC algorithm
> when choosing one.

Do we need it as complicated as it is today?

Currently the infrastructure is:
- the default algorithm is built into mac80211
- other algorithms get into their own modules

The implementation of this complicated scheme is horrible
(just look at net/mac80211/Makefile), and anyone adding a new
algorithm will most likely not get it right at his first attempt.

Can we get one of either:
- all selected mac80211 algorithms are built into the mac80211 module or
- all selected mac80211 algorithms (including the default one) are in
  their own modules
?

It wouldn't make any difference today for the CONFIG_EMBEDDED=n case, 
but allows to make things less complicated and will make it easier to 
add additional algorithms.

I'll be glad to provide a patch if one of these is considered 
acceptable.

> johannes

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: RFC: always enable MAC80211_RC_PID?

[John W. Linville]

On Tue, Jun 24, 2008 at 03:43:14PM +0300, Adrian Bunk wrote:

> Can we get one of either:
> - all selected mac80211 algorithms are built into the mac80211 module or

This seems fine to me.

> - all selected mac80211 algorithms (including the default one) are in
>   their own modules

We got here trying to avoid this one in the first place.

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

[2.6 patch] build algorithms into the mac80211 module

[Adrian Bunk]

On Tue, Jun 24, 2008 at 05:12:22PM -0400, John W. Linville wrote:
> On Tue, Jun 24, 2008 at 03:43:14PM +0300, Adrian Bunk wrote:
> 
> > Can we get one of either:
> > - all selected mac80211 algorithms are built into the mac80211 module or
> 
> This seems fine to me.
> 
> > - all selected mac80211 algorithms (including the default one) are in
> >   their own modules
> 
> We got here trying to avoid this one in the first place.

What about the patch below?

> John

cu
Adrian


<--  snip  -->


The old infrastructure was:
- the default algorithm is built into mac80211
- other algorithms get into their own modules

The implementation of this complicated scheme was horrible
(just look at net/mac80211/Makefile), and anyone adding a new
algorithm would most likely not get it right at his first attempt.

This patch therefore builds all enabled algorithms into the mac80211 
module.

The user interface for the rate control algorithms changes as follows:
- first the user can choose which algorithms to enable (currently only
  MAC80211_RC_PID is available)
- if more than one algorithm is enabled (currently not possible since 
  only one algorithm is present) the user then chooses the default one

Note:
- MAC80211_RC_PID is always enables for CONFIG_EMBEDDED=n

Technical changes:
- all selected algorithms get into the mac80211 module
- net/mac80211/Makefile can now become much less complicated
- support for rc80211_pid_algo.c being modular is no longer required
- this includes unexporting mesh_plink_broken

Signed-off-by: Adrian Bunk <bunk@kernel.org>

---

 net/mac80211/Kconfig            |   31 +++++++++----------------------
 net/mac80211/Makefile           |   18 ++++--------------
 net/mac80211/mesh_pathtbl.c     |    1 -
 net/mac80211/rate.h             |    4 +---
 net/mac80211/rc80211_pid_algo.c |   10 ----------
 5 files changed, 14 insertions(+), 50 deletions(-)

commit 2d61eb39a63d9a4de6f4ecab85432d39d8116961
Author: Adrian Bunk <bunk@kernel.org>
Date:   Wed Jun 25 02:31:03 2008 +0300

diff --git a/net/mac80211/Kconfig b/net/mac80211/Kconfig
index a24b459..2d5bb42 100644
--- a/net/mac80211/Kconfig
+++ b/net/mac80211/Kconfig
@@ -15,6 +15,14 @@ config MAC80211
 menu "Rate control algorithm selection"
 	depends on MAC80211 != n
 
+config MAC80211_RC_PID
+	bool "PID controller based rate control algorithm" if EMBEDDED
+	default y
+	---help---
+	  This option enables a TX rate control algorithm for
+	  mac80211 that uses a PID controller to select the TX
+	  rate.
+
 choice
 	prompt "Default rate control algorithm"
 	default MAC80211_RC_DEFAULT_PID
@@ -26,40 +34,19 @@ choice
 
 config MAC80211_RC_DEFAULT_PID
 	bool "PID controller based rate control algorithm"
-	select MAC80211_RC_PID
+	depends on MAC80211_RC_PID
 	---help---
 	  Select the PID controller based rate control as the
 	  default rate control algorithm. You should choose
 	  this unless you know what you are doing.
 
-config MAC80211_RC_DEFAULT_NONE
-	bool "No default algorithm"
-	depends on EMBEDDED
-	help
-	  Selecting this option will select no default algorithm
-	  and allow you to not build any. Do not choose this
-	  option unless you know your driver comes with another
-	  suitable algorithm.
 endchoice
 
-comment "Selecting 'y' for an algorithm will"
-comment "build the algorithm into mac80211."
-
 config MAC80211_RC_DEFAULT
 	string
 	default "pid" if MAC80211_RC_DEFAULT_PID
 	default ""
 
-config MAC80211_RC_PID
-	tristate "PID controller based rate control algorithm"
-	---help---
-	  This option enables a TX rate control algorithm for
-	  mac80211 that uses a PID controller to select the TX
-	  rate.
-
-	  Say Y or M unless you're sure you want to use a
-	  different rate control algorithm.
-
 endmenu
 
 config MAC80211_MESH
diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile
index 4e5847f..268b0a7 100644
--- a/net/mac80211/Makefile
+++ b/net/mac80211/Makefile
@@ -1,13 +1,5 @@
 obj-$(CONFIG_MAC80211) += mac80211.o
 
-# objects for PID algorithm
-rc80211_pid-y := rc80211_pid_algo.o
-rc80211_pid-$(CONFIG_MAC80211_DEBUGFS) += rc80211_pid_debugfs.o
-
-# build helper for PID algorithm
-rc-pid-y := $(rc80211_pid-y)
-rc-pid-m := rc80211_pid.o
-
 # mac80211 objects
 mac80211-y := \
 	main.o \
@@ -42,10 +34,8 @@ mac80211-$(CONFIG_MAC80211_MESH) += \
 	mesh_plink.o \
 	mesh_hwmp.o
 
+# objects for PID algorithm
+rc80211_pid-y := rc80211_pid_algo.o
+rc80211_pid-$(CONFIG_MAC80211_DEBUGFS) += rc80211_pid_debugfs.o
 
-# Build rate control algorithm(s)
-CFLAGS_rc80211_pid_algo.o += -DRC80211_PID_COMPILE
-mac80211-$(CONFIG_MAC80211_RC_PID) += $(rc-pid-$(CONFIG_MAC80211_RC_PID))
-
-# Modular rate algorithms are assigned to mac80211-m - make separate modules
-obj-m += $(mac80211-m)
+mac80211-$(CONFIG_MAC80211_RC_PID) += $(rc80211_pid-y)
diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index 99c2d36..086d47f 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -264,7 +264,6 @@ void mesh_plink_broken(struct sta_info *sta)
 	}
 	rcu_read_unlock();
 }
-EXPORT_SYMBOL(mesh_plink_broken);
 
 /**
  * mesh_path_flush_by_nexthop - Deletes mesh paths if their next hop matches
diff --git a/net/mac80211/rate.h b/net/mac80211/rate.h
index 5b45f33..54aa78b 100644
--- a/net/mac80211/rate.h
+++ b/net/mac80211/rate.h
@@ -171,9 +171,7 @@ void rate_control_deinitialize(struct ieee80211_local *local);
 
 
 /* Rate control algorithms */
-#if defined(RC80211_PID_COMPILE) || \
-	(defined(CONFIG_MAC80211_RC_PID) && \
-	 !defined(CONFIG_MAC80211_RC_PID_MODULE))
+#ifdef CONFIG_MAC80211_RC_PID
 extern int rc80211_pid_init(void);
 extern void rc80211_pid_exit(void);
 #else
diff --git a/net/mac80211/rc80211_pid_algo.c b/net/mac80211/rc80211_pid_algo.c
index a849b74..68c6641 100644
--- a/net/mac80211/rc80211_pid_algo.c
+++ b/net/mac80211/rc80211_pid_algo.c
@@ -540,11 +540,6 @@ static struct rate_control_ops mac80211_rcpid = {
 #endif
 };
 
-MODULE_DESCRIPTION("PID controller based rate control algorithm");
-MODULE_AUTHOR("Stefano Brivio");
-MODULE_AUTHOR("Mattias Nissler");
-MODULE_LICENSE("GPL");
-
 int __init rc80211_pid_init(void)
 {
 	return ieee80211_rate_control_register(&mac80211_rcpid);
@@ -554,8 +549,3 @@ void rc80211_pid_exit(void)
 {
 	ieee80211_rate_control_unregister(&mac80211_rcpid);
 }
-
-#ifdef CONFIG_MAC80211_RC_PID_MODULE
-module_init(rc80211_pid_init);
-module_exit(rc80211_pid_exit);
-#endif

Re: [2.6 patch] build algorithms into the mac80211 module

[Johannes Berg]

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

On Thu, 2008-06-26 at 13:38 +0300, Adrian Bunk wrote:
> On Tue, Jun 24, 2008 at 05:12:22PM -0400, John W. Linville wrote:
> > On Tue, Jun 24, 2008 at 03:43:14PM +0300, Adrian Bunk wrote:
> > 
> > > Can we get one of either:
> > > - all selected mac80211 algorithms are built into the mac80211 module or
> > 
> > This seems fine to me.
> > 
> > > - all selected mac80211 algorithms (including the default one) are in
> > >   their own modules
> > 
> > We got here trying to avoid this one in the first place.
> 
> What about the patch below?

Seems ok to me, especially since we don't have a tunable for the rate
control algorithm nor any other than the iwlwifi ones and this one...

johannes

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

Re: RFC: always enable MAC80211_RC_PID?

[Luis R. Rodriguez]

On Tue, Jun 24, 2008 at 2:12 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Tue, Jun 24, 2008 at 03:43:14PM +0300, Adrian Bunk wrote:
>
>> Can we get one of either:
>> - all selected mac80211 algorithms are built into the mac80211 module or
>
> This seems fine to me.

OK so just to be clear -- moving forward we strive to not allow driver
specific rate control algorithms and push out current ones into
mac80211? This would mean we don't have to *cleanup* support for
driver specific RCs but instead just have them stashed in as part of
the build. The difficulty here lies in ensuring they do work for well
for other drivers but I do agree it strives to cleanup RC code. I
think vendors also tend to use a few driver specific tweaks to boost
their own throughput in their own RCs though. Can't say for sure right
now of specific details but I'll try to get back to you with them.
Perhaps Tomas can say more about that for iwl's RCs.

  Luis

Re: RFC: always enable MAC80211_RC_PID?

[Tomas Winkler]

On Fri, Jun 27, 2008 at 12:35 AM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Tue, Jun 24, 2008 at 2:12 PM, John W. Linville
> <linville@tuxdriver.com> wrote:
>> On Tue, Jun 24, 2008 at 03:43:14PM +0300, Adrian Bunk wrote:
>>
>>> Can we get one of either:
>>> - all selected mac80211 algorithms are built into the mac80211 module or
>>
>> This seems fine to me.
>
> OK so just to be clear -- moving forward we strive to not allow driver
> specific rate control algorithms and push out current ones into
> mac80211? This would mean we don't have to *cleanup* support for
> driver specific RCs but instead just have them stashed in as part of
> the build. The difficulty here lies in ensuring they do work for well
> for other drivers but I do agree it strives to cleanup RC code. I
> think vendors also tend to use a few driver specific tweaks to boost
> their own throughput in their own RCs though. Can't say for sure right
> now of specific details but I'll try to get back to you with them.
> Perhaps Tomas can say more about that for iwl's RCs.
>
Annual talk about rate scaling algorithm :)
I understand of need of clean framework however:
Our rate scaling algorithm has intimate knowledge of iwlwifi firmware
and hardware and I'm currently not seeing any benefit for it for other
HW.
I'm also pretty much sure that no other algorithm can do same job for
iwlwifi driver.
For example the rate is not attached to each frame but supplied out of
band in a form of 16 steps rate sale table. I've explained already
benefits of this.

Detaching algorithm form the driver would at minimum require opening
another API for rate scaling not usable by others. And this is not the
end. This just doesn't worth the sweat if no other algorithm can be
used for iwlwifi and this algorithm cannot be used by others.
This would be cleaning code for sake of cleaning code without any
benefits. If there be finally some other 11n card in Linux we can see
the new picture and reevaluate.
I'm not the Kconfig expert but  desalinizing the current iwlwifi
driver for prize of removing MAC80211_RC_DEFAULT_NONE just doesn't
feel OK.
Thanks
Tomas

Re: RFC: always enable MAC80211_RC_PID?

[Mattias Nissler]

Just what I remember about the discussion that led to the ugly Makefile.

On Tue, 2008-06-24 at 15:43 +0300, Adrian Bunk wrote:
> 
> Can we get one of either:
> - all selected mac80211 algorithms are built into the mac80211 module or

IIRC, this was originally voted against because this approach would
increase kernel image size or module size unnecessarily.

> - all selected mac80211 algorithms (including the default one) are in
>   their own modules

The argument against this was that there are people who don't compile in
module autoloading, can't figure out what's wrong (in spite of the
syslog message) and complain about wireless not working...

Anyway, I always liked any of the two things you suggested better than
what we have now.

Mattias

Re: RFC: always enable MAC80211_RC_PID?

[Adrian Bunk]

On Thu, Jun 26, 2008 at 07:55:42PM +0200, Mattias Nissler wrote:
> Just what I remember about the discussion that led to the ugly Makefile.
> 
> On Tue, 2008-06-24 at 15:43 +0300, Adrian Bunk wrote:
> > 
> > Can we get one of either:
> > - all selected mac80211 algorithms are built into the mac80211 module or
> 
> IIRC, this was originally voted against because this approach would
> increase kernel image size or module size unnecessarily.
>...

I don't see this point here.

People who _really_ need small kernels can still disable unneeded 
algorithms, and distribution kernels are anyway not usable for cases 
where a few kB more or less would matter.

> Mattias

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: RFC: always enable MAC80211_RC_PID?

[Nick Kossifidis]

2008/6/24 Johannes Berg <johannes@sipsolutions.net>:
>
>> You can currently only get away living without it if you are just
>> using iwl3945 or iwl4965 as they provide their own rate control
>> algorithms. In the future if other vendor drivers are added with their
>> own rate control algorithm this list grows.
>
> FWIW, I would like to see those algorithms be supported as part of
> mac80211 and not be internal like iwlwifi hacked them on, and have some
> sort of feature negotiation between the hardware and the RC algorithm
> when choosing one.
>
> johannes
>

I also agree on that, this way we can eg. port samplerate or minstrel
from MadWiFi to mac80211 for any card that can do multirate
transmitions for example, not only Atheros cards.

We need however a fallback algorithm compiled in by default if we go
that way because the user might not know what algorithms are
compatible with his card during kernel configuration. I guess PID is
generic enough for this.

Also ideally the user should be able to change rate control algorithm
during runtime (without the need to reload the driver), i think having
rate control inside mac80211 makes it easier.


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick