hi,
I haven't made any respectable contributions yet to the state of linux wireless
but I've been quietly following the course of action for several months. (both here and
over at the iwlwifi front)
And I would like to bring up the issue of allowing the drivers to choose the rate
scaling once more. (wrt ilwiwifi)
I know that several months ago this was somewhat a heated topic, but i would really like
to delicately retry this subject. From my perspective with the ultimate goal of iwlwifi
being merged into the mainline kernel at one point. And causing less problems for
developers and users.
I was once one of those users that for more than a year had to patch their kernel with a
specific net80211 package to get ipw3945 (which due to regulatory daemon is an entirely
different story). All the problems it caused getting it to work, never mind between
kernels, was a bit of a challenge.So I would really like to help minimize similar problems
with iwlwifi and mac80211
Currently iwlwifi requires (when used with wireless dev or the mac80211 in the .22/.23
kernels) to work around the fact that mac80211 doesn't allow the driver to choose the rate
scaling algorithm, that mac80211 isn't compiled in the kernel, and no other
rc80211_* modules are loaded. Because due to several reasons iwlwifi uses its ratescaling
to do some tasks like maintain some synchronization (station list) between the ucode and
driver.(and at least for 3945 iw3945 rs works well.)
It's understandable that this functionality might belong in the software stack that mac80211
wants to be. And that there is no reason not to be critical about proposed changes, why else
would linux be open source. As well that code can always be moved from one level
of abstraction to another.
Personally I don't see the harm in allowing the drivers the choice of rate scaling algorithm,
and the patches that were proposed months ago(don't shoot me if it was less than 60 days)
appear to be elegant and clean enough(9 lines of which 4 are debug).
A version of the iwlwifi driver (merge patch v4, with the goal of merging it in .24) which has
been up for review for about a week now. Hasn't gotten too diminishing feedback yet,
some -to my opinion- grounded remarks from Johannes Berg, but not much else.
To be blunt: looking at the possible routes of action that i see:
- re-review the patch to allow the drivers to choose (preferred) rate scaling, and give it
a chance to make it in a future kernel.
- propose a feasible other course of action
- Simply refuse this for good reasons, leaving this issue to exist for longer,
and giving me (one of the kids with too much time) more explaining in the irc channel.
for 5 lousy lines of code.
I'll conclude this mail before it becomes too long and naggy, I'm sure the general point
is clear by now. Feel free to tell me to stfu if i totally missed the point. However I do
honestly believe that merging this patch or resolving this in another mutual satisfactory way.
Would benefit most of us. Obviously I don't have any say in the decision making of the intel
developers, but expecting that everything comes from Zhu Yi or others is unrealistic too.
[And for the record: I too would love an free for all ucode license, I honestly don't believe that
intel is only after product diferenciation, full specifications would be great. :-)]
ps: for reference, I'm talking about the "Specifing rate control algorithm?" thread which started
the 10th of may on this list.
Ian Schram.
Johannes Berg wrote:
>
> There were a few specific remarks about the patch
I reread the discussion carefully and can't find any specific
comments on the patch which for clarity I will reproduce below.
>, e.g. that the default
> shouldn't change as soon as a driver loads another rate control
> algorithm etc.
The "default" rate scaling algorithm, is currently just the "first registered",
and as long as there is already an algorithm loaded, the driver can
"load" as many as it wants and wont change this. The patch doesn't touch
that logic.
> Once those are addressed I see no problems with merging
> such a patch.
>
> johannes
ian
The patch in question:
Signed-off-by: James Ketrenos <[email protected]>
---
include/net/mac80211.h | 4 ++++
net/mac80211/ieee80211.c | 10 +++++++---
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index ef9b613..f9ad839 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -533,6 +533,10 @@ struct ieee80211_hw {
/* Number of available hardware TX queues for data packets.
* WMM requires at least four queues. */
int queues;
+
+ /* Preferred rate control algorithm. Leave as NULL for stack
+ * to select algorithm */
+ char *preferred_rate_control;
};
static inline void SET_IEEE80211_DEV(struct ieee80211_hw *hw, struct device *dev)
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index 873ccb0..6dcc002 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -4875,10 +4875,14 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
ieee80211_debugfs_add_netdev(IEEE80211_DEV_TO_SUB_IF(local->mdev));
- result = ieee80211_init_rate_ctrl_alg(local, NULL);
+ result = ieee80211_init_rate_ctrl_alg(local,
+ hw->preferred_rate_control);
+
if (result < 0) {
- printk(KERN_DEBUG "%s: Failed to initialize rate control "
- "algorithm\n", local->mdev->name);
+ printk(KERN_DEBUG "%s: Failed to initialize %s rate control "
+ "algorithm\n", local->mdev->name,
+ hw->preferred_rate_control ?
+ hw->preferred_rate_control : "default");
goto fail_rate;
}
On Mon, Aug 27, 2007 at 12:49:09PM +0200, Johannes Berg wrote:
> On Sat, 2007-08-25 at 14:15 +0200, ian wrote:
>
> > I reread the discussion carefully and can't find any specific
> > comments on the patch which for clarity I will reproduce below.
>
> Then I guess I remember wrong and the comment was raised in another
> thread. The problem we have is that once any rate control algorithm is
> loaded, it will be default for any future drivers, which is often
> inappropriate. Hence, it would be great if
> hw->preferred_rate_control==NULL would indicate "use default" where
> "default" would be set to "simple" to start with but also changeable.
> (although the change functionality could just be not usable from
> userspace for now)
I ACK this approach. Anyone have a patch hacked-up to post?
John
--
John W. Linville
[email protected]
Hi,
just a cosmetic issue:
On Sat, Aug 25, 2007 at 02:15:19PM +0200, ian wrote:
> + printk(KERN_DEBUG "%s: Failed to initialize %s rate control "
> + "algorithm\n", local->mdev->name,
> + hw->preferred_rate_control ?
> + hw->preferred_rate_control : "default");
It would be nicer if this was
+ printk(KERN_DEBUG "%s: Failed to initialize %srate control "
+ "algorithm\n", local->mdev->name,
+ hw->preferred_rate_control ?
+ hw->preferred_rate_control : "default ");
to avoid a spurious space in the non-default case.
I hope this helps,
Jochen
--
http://seehuhn.de/
On Sat, 2007-08-25 at 14:15 +0200, ian wrote:
> I reread the discussion carefully and can't find any specific
> comments on the patch which for clarity I will reproduce below.
Then I guess I remember wrong and the comment was raised in another
thread. The problem we have is that once any rate control algorithm is
loaded, it will be default for any future drivers, which is often
inappropriate. Hence, it would be great if
hw->preferred_rate_control==NULL would indicate "use default" where
"default" would be set to "simple" to start with but also changeable.
(although the change functionality could just be not usable from
userspace for now)
johannes
Ian,
> To be blunt: looking at the possible routes of action that i see:
> - re-review the patch to allow the drivers to choose (preferred) rate scaling, and give it
> a chance to make it in a future kernel.
> - propose a feasible other course of action
> - Simply refuse this for good reasons, leaving this issue to exist for longer,
> and giving me (one of the kids with too much time) more explaining in the irc channel.
> for 5 lousy lines of code.
> ps: for reference, I'm talking about the "Specifing rate control algorithm?" thread which started
> the 10th of may on this list.
There were a few specific remarks about the patch, e.g. that the default
shouldn't change as soon as a driver loads another rate control
algorithm etc. Once those are addressed I see no problems with merging
such a patch.
johannes
On Mon, 2007-09-03 at 02:21 +0200, ian wrote:
> I read up on the code and kernel mechanisms involved and this is
> what i came up with
>
> Right now there is no way of knowing what the "default" rc should be
> other than that it has a module alias rc80211_default, checking
> for this is probably not done, and would be pretty unpractical.
> And it wouldn't be settable from userspace in the future either.
>
> We could however replace "simple" with a user settable variable.
> (global variable in ieee80211.c like the rate_ctrl_algs and mutex,
> with some accessors for a module parameter/debugfs/..?)
>
> we could additionally remove the module_alias from rc80211_simple
>
> I don't know too much of the rest of mac80211 code and conventions,
> so feel free to be brutal if this patch makes no sense.
> ---
> Make rc80211_simple the default rate scaling algorithm
>
> Signed-of-by: Ian Schram <[email protected]>
I think along with the patch from James this should work, with simple
being the default. We should additionally have a debugfs file associated
with "local" that allows you to set the "simple" string you've used
there to something else. That also allows us to test the error path
there :P
> --- a/net/mac80211/ieee80211_rate.c 2007-09-03 01:26:18.000000000 +0200
> +++ b/net/mac80211/ieee80211_rate.c 2007-09-03 01:46:54.000000000 +0200
> @@ -62,7 +62,7 @@ ieee80211_try_rate_control_ops_get(const
>
> mutex_lock(&rate_ctrl_mutex);
> list_for_each_entry(alg, &rate_ctrl_algs, list) {
> - if (!name || !strcmp(alg->ops->name, name))
> + if (!strcmp(alg->ops->name, name))
> if (try_module_get(alg->ops->module)) {
> ops = alg->ops;
> break;
> @@ -78,11 +78,12 @@ static struct rate_control_ops *
> ieee80211_rate_control_ops_get(const char *name)
> {
> struct rate_control_ops *ops;
> + const char *try_name = name ? name : "simple";
>
> - ops = ieee80211_try_rate_control_ops_get(name);
> + ops = ieee80211_try_rate_control_ops_get(try_name);
> if (!ops) {
> - request_module("rc80211_%s", name ? name : "default");
> - ops = ieee80211_try_rate_control_ops_get(name);
> + request_module("rc80211_%s", try_name);
> + ops = ieee80211_try_rate_control_ops_get(try_name);
> }
> return ops;
> }
>
>
>
John W. Linville wrote:
> On Mon, Aug 27, 2007 at 12:49:09PM +0200, Johannes Berg wrote:
>> On Sat, 2007-08-25 at 14:15 +0200, ian wrote:
>>
>>> I reread the discussion carefully and can't find any specific
>>> comments on the patch which for clarity I will reproduce below.
>> Then I guess I remember wrong and the comment was raised in another
>> thread. The problem we have is that once any rate control algorithm is
>> loaded, it will be default for any future drivers, which is often
>> inappropriate. Hence, it would be great if
>> hw->preferred_rate_control==NULL would indicate "use default" where
>> "default" would be set to "simple" to start with but also changeable.
>> (although the change functionality could just be not usable from
>> userspace for now)
>
> I ACK this approach. Anyone have a patch hacked-up to post?
>
I read up on the code and kernel mechanisms involved and this is
what i came up with
Right now there is no way of knowing what the "default" rc should be
other than that it has a module alias rc80211_default, checking
for this is probably not done, and would be pretty unpractical.
And it wouldn't be settable from userspace in the future either.
We could however replace "simple" with a user settable variable.
(global variable in ieee80211.c like the rate_ctrl_algs and mutex,
with some accessors for a module parameter/debugfs/..?)
we could additionally remove the module_alias from rc80211_simple
I don't know too much of the rest of mac80211 code and conventions,
so feel free to be brutal if this patch makes no sense.
---
Make rc80211_simple the default rate scaling algorithm
Signed-of-by: Ian Schram <[email protected]>
--- a/net/mac80211/ieee80211_rate.c 2007-09-03 01:26:18.000000000 +0200
+++ b/net/mac80211/ieee80211_rate.c 2007-09-03 01:46:54.000000000 +0200
@@ -62,7 +62,7 @@ ieee80211_try_rate_control_ops_get(const
mutex_lock(&rate_ctrl_mutex);
list_for_each_entry(alg, &rate_ctrl_algs, list) {
- if (!name || !strcmp(alg->ops->name, name))
+ if (!strcmp(alg->ops->name, name))
if (try_module_get(alg->ops->module)) {
ops = alg->ops;
break;
@@ -78,11 +78,12 @@ static struct rate_control_ops *
ieee80211_rate_control_ops_get(const char *name)
{
struct rate_control_ops *ops;
+ const char *try_name = name ? name : "simple";
- ops = ieee80211_try_rate_control_ops_get(name);
+ ops = ieee80211_try_rate_control_ops_get(try_name);
if (!ops) {
- request_module("rc80211_%s", name ? name : "default");
- ops = ieee80211_try_rate_control_ops_get(name);
+ request_module("rc80211_%s", try_name);
+ ops = ieee80211_try_rate_control_ops_get(try_name);
}
return ops;
}
John W. Linville wrote:
> On Mon, Sep 03, 2007 at 04:06:56PM +0200, ian wrote:
>> Johannes Berg wrote:
>>> On Mon, 2007-09-03 at 02:21 +0200, ian wrote:
>>>
>>>> we could additionally remove the module_alias from rc80211_simple
>>> indeed, please roll into this patch
>>>
>> I don't know enough from debugfs yet, to do that part, but I'll read up.
>>
>> ---
>> Make rc80211_simple the default rate scaling algorithm
>
>> @@ -78,11 +78,12 @@ static struct rate_control_ops *
>> ieee80211_rate_control_ops_get(const char *name)
>> {
>> struct rate_control_ops *ops;
>> + const char *try_name = name ? name : "simple";
>>
>> - ops = ieee80211_try_rate_control_ops_get(name);
>> + ops = ieee80211_try_rate_control_ops_get(try_name);
>> if (!ops) {
>> - request_module("rc80211_%s", name ? name : "default");
>> - ops = ieee80211_try_rate_control_ops_get(name);
>> + request_module("rc80211_%s", try_name);
>> + ops = ieee80211_try_rate_control_ops_get(try_name);
>> }
>> return ops;
>> }
>> --- a/net/mac80211/rc80211_simple.c 2007-09-03 15:53:42.000000000 +0200
>> +++ b/net/mac80211/rc80211_simple.c 2007-09-03 15:54:40.000000000 +0200
>> @@ -29,7 +29,6 @@
>> #define RATE_CONTROL_INTERVAL (HZ / 20)
>> #define RATE_CONTROL_MIN_TX 10
>>
>> -MODULE_ALIAS("rc80211_default");
>>
>> static void rate_control_rate_inc(struct ieee80211_local *local,
>> struct sta_info *sta)
>
> Does this actually change anything? With the MODULE_ALIAS line
> in rc80211_simple.c, doesn't the request_module("rc80211_default")
> just load rc80211_simple?
>
> Confused...
I believe it should. :) obviously. I'll Do my best to explain it. It
seems a bit overkill for the I don't know 10 lines it changes.
The main clue is ieee80211_try_rate_control_ops_get never being called with NULL.
not so much the request_module line.
The way it was probably intended:
1) find default
2) if not found load rc80211_default
3) find default
The thing is that there is no way to find "default". There is nothing Except the
module alias(which you can't really check for at runtime? in the list of loaded algos)
that designated a certain algorithm to be default.
To compensate and hide the code's inability to find the default, it just grabbed the first
module that was loaded.
this find default was implemented by this null check
- if (!name || !strcmp(alg->ops->name, name))
+ if (!strcmp(alg->ops->name, name))
which now serves no more purpose and was hence removed
The idea that a module itself could determine it is the default is probably
broken from a security point of view.
The patch is really trivial, but I believe this is what was meant. I could Just
have let it load rc80211_default. However the logic behind the alias vanished by
(for now) hard coding which rs is the default. Johannes then asked
me to remove the alias all together.
The whole point of this exercise is to make simple the default.
perhaps it would have been clearer if I had changed step 2 and replaced the null_check
with an strcmp for rc80211_simple. But this patch seemed cleanest.
I hope I answered your question.
>
> John
On Thu, 2007-09-13 at 23:42 +0200, Ian Schram wrote:
> The whole point of this exercise is to make simple the default.
What I actually wanted, though, was to have the default be a string that
is settable at runtime to any of the loaded rate control algorithms.
johannes
Johannes Berg wrote:
> On Mon, 2007-09-03 at 02:21 +0200, ian wrote:
>
>> we could additionally remove the module_alias from rc80211_simple
>
> indeed, please roll into this patch
>
I don't know enough from debugfs yet, to do that part, but I'll read up.
---
Make rc80211_simple the default rate scaling algorithm
Signed-of-by: Ian Schram <[email protected]>
--- a/net/mac80211/ieee80211_rate.c 2007-09-03 01:26:18.000000000 +0200
+++ b/net/mac80211/ieee80211_rate.c 2007-09-03 01:46:54.000000000 +0200
@@ -62,7 +62,7 @@ ieee80211_try_rate_control_ops_get(const
mutex_lock(&rate_ctrl_mutex);
list_for_each_entry(alg, &rate_ctrl_algs, list) {
- if (!name || !strcmp(alg->ops->name, name))
+ if (!strcmp(alg->ops->name, name))
if (try_module_get(alg->ops->module)) {
ops = alg->ops;
break;
@@ -78,11 +78,12 @@ static struct rate_control_ops *
ieee80211_rate_control_ops_get(const char *name)
{
struct rate_control_ops *ops;
+ const char *try_name = name ? name : "simple";
- ops = ieee80211_try_rate_control_ops_get(name);
+ ops = ieee80211_try_rate_control_ops_get(try_name);
if (!ops) {
- request_module("rc80211_%s", name ? name : "default");
- ops = ieee80211_try_rate_control_ops_get(name);
+ request_module("rc80211_%s", try_name);
+ ops = ieee80211_try_rate_control_ops_get(try_name);
}
return ops;
}
--- a/net/mac80211/rc80211_simple.c 2007-09-03 15:53:42.000000000 +0200
+++ b/net/mac80211/rc80211_simple.c 2007-09-03 15:54:40.000000000 +0200
@@ -29,7 +29,6 @@
#define RATE_CONTROL_INTERVAL (HZ / 20)
#define RATE_CONTROL_MIN_TX 10
-MODULE_ALIAS("rc80211_default");
static void rate_control_rate_inc(struct ieee80211_local *local,
struct sta_info *sta)
> johannes
On Mon, 2007-09-03 at 02:21 +0200, ian wrote:
> we could additionally remove the module_alias from rc80211_simple
indeed, please roll into this patch
johannes
On Mon, Sep 03, 2007 at 04:06:56PM +0200, ian wrote:
> Johannes Berg wrote:
> > On Mon, 2007-09-03 at 02:21 +0200, ian wrote:
> >
> >> we could additionally remove the module_alias from rc80211_simple
> >
> > indeed, please roll into this patch
> >
> I don't know enough from debugfs yet, to do that part, but I'll read up.
>
> ---
> Make rc80211_simple the default rate scaling algorithm
> @@ -78,11 +78,12 @@ static struct rate_control_ops *
> ieee80211_rate_control_ops_get(const char *name)
> {
> struct rate_control_ops *ops;
> + const char *try_name = name ? name : "simple";
>
> - ops = ieee80211_try_rate_control_ops_get(name);
> + ops = ieee80211_try_rate_control_ops_get(try_name);
> if (!ops) {
> - request_module("rc80211_%s", name ? name : "default");
> - ops = ieee80211_try_rate_control_ops_get(name);
> + request_module("rc80211_%s", try_name);
> + ops = ieee80211_try_rate_control_ops_get(try_name);
> }
> return ops;
> }
> --- a/net/mac80211/rc80211_simple.c 2007-09-03 15:53:42.000000000 +0200
> +++ b/net/mac80211/rc80211_simple.c 2007-09-03 15:54:40.000000000 +0200
> @@ -29,7 +29,6 @@
> #define RATE_CONTROL_INTERVAL (HZ / 20)
> #define RATE_CONTROL_MIN_TX 10
>
> -MODULE_ALIAS("rc80211_default");
>
> static void rate_control_rate_inc(struct ieee80211_local *local,
> struct sta_info *sta)
Does this actually change anything? With the MODULE_ALIAS line
in rc80211_simple.c, doesn't the request_module("rc80211_default")
just load rc80211_simple?
Confused...
John
--
John W. Linville
[email protected]