[PATCH 1/2] cfg80211: initialize rate control after station inserted

[PATCH 1/2] cfg80211: initialize rate control after station inserted

[Reinette Chatre]

From: Reinette Chatre <reinette.chatre@intel.com>

Station information may be needed by rate control algorithms, so call rate
scaling initialization after adding the station.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 net/mac80211/cfg.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 5608f6c..598db11 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -729,8 +729,6 @@ static int ieee80211_add_station(struct wiphy *wiphy, struct net_device *dev,
 
 	sta_apply_parameters(local, sta, params);
 
-	rate_control_rate_init(sta);
-
 	layer2_update = sdata->vif.type == NL80211_IFTYPE_AP_VLAN ||
 		sdata->vif.type == NL80211_IFTYPE_AP;
 
@@ -742,13 +740,17 @@ static int ieee80211_add_station(struct wiphy *wiphy, struct net_device *dev,
 		if (err == -EEXIST && layer2_update) {
 			/* Need to update layer 2 devices on reassociation */
 			sta = sta_info_get(local, mac);
-			if (sta)
+			if (sta) {
+				rate_control_rate_init(sta);
 				ieee80211_send_layer2_update(sta);
+			}
 		}
 		rcu_read_unlock();
 		return err;
 	}
 
+	rate_control_rate_init(sta);
+
 	if (layer2_update)
 		ieee80211_send_layer2_update(sta);
 
-- 
1.5.6.3

[PATCH 2/2] mac80211: initialize rate control after station inserted

[Reinette Chatre]

From: Reinette Chatre <reinette.chatre@intel.com>

Station information may be needed by rate control algorithms, so call rate
scaling initialization after adding the station.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 net/mac80211/ibss.c |    3 ++-
 net/mac80211/mlme.c |    4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 920ec87..040d184 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -413,11 +413,12 @@ struct sta_info *ieee80211_ibss_add_sta(struct ieee80211_sub_if_data *sdata,
 	sta->sta.supp_rates[band] = supp_rates |
 			ieee80211_mandatory_rates(local, band);
 
-	rate_control_rate_init(sta);
 
 	if (sta_info_insert(sta))
 		return NULL;
 
+	rate_control_rate_init(sta);
+
 	return sta;
 }
 
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 97a278a..307b33c 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1554,8 +1554,6 @@ ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 
 	ap_ht_cap_flags = sta->sta.ht_cap.cap;
 
-	rate_control_rate_init(sta);
-
 	if (ifmgd->flags & IEEE80211_STA_MFP_ENABLED)
 		set_sta_flags(sta, WLAN_STA_MFP);
 
@@ -1572,6 +1570,8 @@ ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 		}
 	}
 
+	rate_control_rate_init(sta);
+
 	rcu_read_unlock();
 
 	if (elems.wmm_param)
-- 
1.5.6.3

Re: [PATCH 1/2] cfg80211: initialize rate control after station inserted

[Johannes Berg]

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

Hi,

Thanks. I'll comment on both patches together. Can you please also tell
us what the problem is this is solving? I'm a bit lost.

I think both of these patches should just rolled into one since this is
also a mac80211 patch -- even if it's for the bit that interacts with
cfg80211.

However, I don't think I actually understand the changes.

>  	sta_apply_parameters(local, sta, params);
>  
> -	rate_control_rate_init(sta);
> -
>  	layer2_update = sdata->vif.type == NL80211_IFTYPE_AP_VLAN ||
>  		sdata->vif.type == NL80211_IFTYPE_AP;
>  
> @@ -742,13 +740,17 @@ static int ieee80211_add_station(struct wiphy *wiphy, struct net_device *dev,
>  		if (err == -EEXIST && layer2_update) {
>  			/* Need to update layer 2 devices on reassociation */
>  			sta = sta_info_get(local, mac);
> -			if (sta)
> +			if (sta) {
> +				rate_control_rate_init(sta);
>  				ieee80211_send_layer2_update(sta);
> +			}
>  		}

Why is this necessary? It should already have been called for this
station earlier?

>  		rcu_read_unlock();
>  		return err;
>  	}
>  
> +	rate_control_rate_init(sta);
> +

Also, I don't see anything between the old place that it was called and
the new place you're moving it to that could possibly change the station
parameters?

Same in ibss.c (not quoting it here) where you're only moving it to
after sta_info_insert() -- all that seems to do is add race conditions,
allowing other code to find not-yet-initialised stations.

So the only place I could see a change being necessary would be mlme.c,
but then only moving rate_control_rate_init() to after the flags
settings, not to after the insert.

Am I missing something?

johannes

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

Re: [PATCH 1/2] cfg80211: initialize rate control after station inserted

[reinette chatre]

Hi Johannes,

On Fri, 2009-08-28 at 00:45 -0700, Johannes Berg wrote:
> Thanks. I'll comment on both patches together. Can you please also tell
> us what the problem is this is solving? I'm a bit lost.

This work is motivated by an attempt to untangle the iwlwifi station
management to be able to use mac80211's sta notify callback. From 4965
and up the rate scaling in the device is done per station, so an entry
in the station table is required for the rate scaling initialization to
succeed. 

> I think both of these patches should just rolled into one since this is
> also a mac80211 patch -- even if it's for the bit that interacts with
> cfg80211.

ok - will do (if it is determined that they are needed).

> 
> However, I don't think I actually understand the changes.
> 
> >  	sta_apply_parameters(local, sta, params);
> >  
> > -	rate_control_rate_init(sta);
> > -
> >  	layer2_update = sdata->vif.type == NL80211_IFTYPE_AP_VLAN ||
> >  		sdata->vif.type == NL80211_IFTYPE_AP;
> >  
> > @@ -742,13 +740,17 @@ static int ieee80211_add_station(struct wiphy *wiphy, struct net_device *dev,
> >  		if (err == -EEXIST && layer2_update) {
> >  			/* Need to update layer 2 devices on reassociation */
> >  			sta = sta_info_get(local, mac);
> > -			if (sta)
> > +			if (sta) {
> > +				rate_control_rate_init(sta);
> >  				ieee80211_send_layer2_update(sta);
> > +			}
> >  		}
> 
> Why is this necessary? It should already have been called for this
> station earlier?

maybe - I just tried to have the code behave exactly as before, just
with the rate scale initialization called later. Even before this patch,
rate scaling initialization would be called if the station already
exists. 

If it is not necessary I can remove it.

> 
> >  		rcu_read_unlock();
> >  		return err;
> >  	}
> >  
> > +	rate_control_rate_init(sta);
> > +
> 
> Also, I don't see anything between the old place that it was called and
> the new place you're moving it to that could possibly change the station
> parameters?

Station parameters may not have changed, but at this point the driver
has been notified that this station exists and it is now able to do the
rate scaling initialization correctly.

Right now iwlwifi is adding stations inside the rate scaling code in
order to work around this issue. I'd like to clean this up and only use
the sta notify callback.

> 
> Same in ibss.c (not quoting it here) where you're only moving it to
> after sta_info_insert()

This was my goal actually.

>  -- all that seems to do is add race conditions,
> allowing other code to find not-yet-initialised stations.

I did not realize that this can happen. Can you please elaborate?

> So the only place I could see a change being necessary would be mlme.c,
> but then only moving rate_control_rate_init() to after the flags
> settings, not to after the insert.

Here too I moved it to after the insert for the same reason as above.


Reinette

Re: [PATCH 1/2] cfg80211: initialize rate control after station inserted

[Johannes Berg]

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

Hi Reinette,

> This work is motivated by an attempt to untangle the iwlwifi station
> management to be able to use mac80211's sta notify callback. From 4965
> and up the rate scaling in the device is done per station, so an entry
> in the station table is required for the rate scaling initialization to
> succeed. 

Interesting. I've been thinking about making it go the other way --
remove rate scaling hooks completely. wl1271 apparently has rate scaling
completely in the firmware, so the RS algorithm on the host is just
overhead. I've been thinking putting 4965+ RS into the _driver_ makes
more sense since it really does a lot in the firmware and not on the
host.

Do you think we should try to go that route? I'd think it would probably
be a hardware flag ("no RS algo please") and then we'd skip all the
hooks and put things into the driver. The advantage is that we don't
care about the mac80211 API any more, things get cleaner and we can just
do all RS init from sta_notify().

I've also been thinking if there's a way to make sta_notify() able to
sleep but so far I don't see one unfortunately.

Thoughts?

Anyhow, thanks for the explanation.

> > > @@ -742,13 +740,17 @@ static int ieee80211_add_station(struct wiphy *wiphy, struct net_device *dev,
> > >  		if (err == -EEXIST && layer2_update) {
> > >  			/* Need to update layer 2 devices on reassociation */
> > >  			sta = sta_info_get(local, mac);
> > > -			if (sta)
> > > +			if (sta) {
> > > +				rate_control_rate_init(sta);
> > >  				ieee80211_send_layer2_update(sta);
> > > +			}
> > >  		}
> > 
> > Why is this necessary? It should already have been called for this
> > station earlier?
> 
> maybe - I just tried to have the code behave exactly as before, just
> with the rate scale initialization called later. Even before this patch,
> rate scaling initialization would be called if the station already
> exists. 
> 
> If it is not necessary I can remove it.

No, right, I understand now.

> Right now iwlwifi is adding stations inside the rate scaling code in
> order to work around this issue. I'd like to clean this up and only use
> the sta notify callback.

Makes sense, thanks, I appreciate that -- should be a good cleanup to
the driver and reduce the number of places that try to add a station and
make the driver more streamlined.

> > Same in ibss.c (not quoting it here) where you're only moving it to
> > after sta_info_insert()
> 
> This was my goal actually.

Yeah, I finally understood :)

> >  -- all that seems to do is add race conditions,
> > allowing other code to find not-yet-initialised stations.
> 
> I did not realize that this can happen. Can you please elaborate?

As soon as sta_insert() got called, a packet transmitted to that station
can be processed, find the sta info, and it seems we could end up
calling rate_control_get_rate() before the init was done, through a race
condition.

johannes

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

Re: [PATCH 1/2] cfg80211: initialize rate control after station inserted

[Luis R. Rodriguez]

On Fri, Aug 28, 2009 at 2:01 PM, Johannes Berg<johannes@sipsolutions.net> wrote:
> Interesting. I've been thinking about making it go the other way --
> remove rate scaling hooks completely. wl1271 apparently has rate scaling
> completely in the firmware,

Just FYI -- ar9271 seems to also do this in firmware, haven't gotten
to that part yet but so I'm told.

  Luis

Re: [PATCH 1/2] cfg80211: initialize rate control after station inserted

[Tim Gardner]

Johannes Berg wrote:
> Hi Reinette,
> 
>> This work is motivated by an attempt to untangle the iwlwifi station
>> management to be able to use mac80211's sta notify callback. From 4965
>> and up the rate scaling in the device is done per station, so an entry
>> in the station table is required for the rate scaling initialization to
>> succeed. 
> 
> Interesting. I've been thinking about making it go the other way --
> remove rate scaling hooks completely. wl1271 apparently has rate scaling
> completely in the firmware, so the RS algorithm on the host is just
> overhead. I've been thinking putting 4965+ RS into the _driver_ makes
> more sense since it really does a lot in the firmware and not on the
> host.
> 
> Do you think we should try to go that route? I'd think it would probably
> be a hardware flag ("no RS algo please") and then we'd skip all the
> hooks and put things into the driver. The advantage is that we don't
> care about the mac80211 API any more, things get cleaner and we can just
> do all RS init from sta_notify().
> 

Wouldn't that make it difficult to experiment with external rate scaling 
algorithms? Not that minstrel or the other in-driver rate scaling 
algorithms always get it right, but they are certainly more transparent 
(and changeable) then firmware.

rtg
-- 
Tim Gardner tim.gardner@canonical.com

Re: [PATCH 1/2] cfg80211: initialize rate control after station inserted

[Kalle Valo]

Tim Gardner <tim.gardner@canonical.com> writes:

>> Interesting. I've been thinking about making it go the other way --
>> remove rate scaling hooks completely. wl1271 apparently has rate scaling
>> completely in the firmware, so the RS algorithm on the host is just
>> overhead. I've been thinking putting 4965+ RS into the _driver_ makes
>> more sense since it really does a lot in the firmware and not on the
>> host.
>>
>> Do you think we should try to go that route? I'd think it would probably
>> be a hardware flag ("no RS algo please") and then we'd skip all the
>> hooks and put things into the driver. The advantage is that we don't
>> care about the mac80211 API any more, things get cleaner and we can just
>> do all RS init from sta_notify().
>>
>
> Wouldn't that make it difficult to experiment with external rate
> scaling algorithms? Not that minstrel or the other in-driver rate
> scaling algorithms always get it right, but they are certainly more
> transparent (and changeable) then firmware.

In wl1271 you are forced to use the rate scaling algorithm from the
firmware. IIRC tx descriptor doesn't even have a field for the bitrate.

-- 
Kalle Valo

Re: [PATCH 1/2] cfg80211: initialize rate control after station inserted

[Johannes Berg]

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

On Fri, 2009-08-28 at 15:40 -0600, Tim Gardner wrote:

> Wouldn't that make it difficult to experiment with external rate scaling 
> algorithms? Not that minstrel or the other in-driver rate scaling 
> algorithms always get it right, but they are certainly more transparent 
> (and changeable) then firmware.

It does, in theory, but then again you don't really _have_ the choice.
iwlagn only lets you control the rate on the host to some extent,
there's not a lot we can do. IMHO using the rate control algorithms like
they are now is a kludge because we've always needed a rate control
algorithm, but it's still not really useful.

johannes

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

Re: [PATCH 1/2] cfg80211: initialize rate control after station inserted

[reinette chatre]

Hi Johannes,

On Fri, 2009-08-28 at 14:01 -0700, Johannes Berg wrote:
> > This work is motivated by an attempt to untangle the iwlwifi station
> > management to be able to use mac80211's sta notify callback. From 4965
> > and up the rate scaling in the device is done per station, so an entry
> > in the station table is required for the rate scaling initialization to
> > succeed. 
> 
> Interesting. I've been thinking about making it go the other way --
> remove rate scaling hooks completely. wl1271 apparently has rate scaling
> completely in the firmware, so the RS algorithm on the host is just
> overhead. I've been thinking putting 4965+ RS into the _driver_ makes
> more sense since it really does a lot in the firmware and not on the
> host.

Yes, it does do a lot in firmware. Unfortunately I am not too familiar
with the details (yet).

> Do you think we should try to go that route? I'd think it would probably
> be a hardware flag ("no RS algo please") and then we'd skip all the
> hooks and put things into the driver. The advantage is that we don't
> care about the mac80211 API any more, things get cleaner and we can just
> do all RS init from sta_notify().

It could do RS init from sta_notify and that would solve this problem. I
am not familiar with how the other hooks operate to know at this time
what it will take to do everything in the driver. Come to think of it,
as a test over here I can just make our RS init routine a no-op and then
do the init from sta notify. 

> 
> I've also been thinking if there's a way to make sta_notify() able to
> sleep but so far I don't see one unfortunately.

Having it sleep will help a lot. When a station is added we need to tell
the device about it. Since the call cannot sleep we cannot really tell
mac80211 if this failed because the failure will only be known at a
later time. I have not yet figured out how to deal with this case.


> > Right now iwlwifi is adding stations inside the rate scaling code in
> > order to work around this issue. I'd like to clean this up and only use
> > the sta notify callback.
> 
> Makes sense, thanks, I appreciate that -- should be a good cleanup to
> the driver and reduce the number of places that try to add a station and
> make the driver more streamlined.

Exactly.

> > >  -- all that seems to do is add race conditions,
> > > allowing other code to find not-yet-initialised stations.
> > 
> > I did not realize that this can happen. Can you please elaborate?
> 
> As soon as sta_insert() got called, a packet transmitted to that station
> can be processed, find the sta info, and it seems we could end up
> calling rate_control_get_rate() before the init was done, through a race
> condition.

oh - I see - that's bad. Although, that may explain why iwlwifi is
adding stations in get_rate() also. 

Reinette

Re: [PATCH 1/2] cfg80211: initialize rate control after station inserted

[Johannes Berg]

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

Hi Reinette,

> > Interesting. I've been thinking about making it go the other way --
> > remove rate scaling hooks completely. wl1271 apparently has rate scaling
> > completely in the firmware, so the RS algorithm on the host is just
> > overhead. I've been thinking putting 4965+ RS into the _driver_ makes
> > more sense since it really does a lot in the firmware and not on the
> > host.
> 
> Yes, it does do a lot in firmware. Unfortunately I am not too familiar
> with the details (yet).

As far as I know/can tell, you basically upload the LQ command to the
firmware for each station and it in a way controls the parameters. What
exactly it does I'm not sure, but I am pretty sure that we don't have up
to 16 retries programmed into the TX descriptor for each packet, but we
can do that many retries.

iwl_tx_cmd contains only a single rate_n_flags field, and then there's
TX_CMD_FLG_STA_RATE_MSK, there's a comment on that definition that
explains some more.

> > I've also been thinking if there's a way to make sta_notify() able to
> > sleep but so far I don't see one unfortunately.
> 
> Having it sleep will help a lot. When a station is added we need to tell
> the device about it. Since the call cannot sleep we cannot really tell
> mac80211 if this failed because the failure will only be known at a
> later time. I have not yet figured out how to deal with this case.

Yeah, that would help. On the other hand, mac80211 isn't actually
prepared to deal with that. In fact, sta_notify has no return value. And
I'm not really sure how to deal with errors from it. Leaving AP aside
for a minute, I think we probably can't do anything. If we exceed the
capacity of the microcode's station memory, I think the best we can do
is just not use rate control for that station, and use the broadcast
station. It won't really happen anyway. And software crypto, of course.
So there's not much to be done.

Now looking at AP again, it _might_ make sense to tell hostapd "sorry we
can't really talk to that station well", but on the other hand I suspect
it's not a case we should really consider. I'd say we can export the
number of stations so that hostapd can actually try not to exceed the
limit in the first place.

As such, having the sta_notify callback sleep will not actually help
much.

I'd say we do the following: At startup, we program the broadcast STA
into the device so we always have that, and do that synchronously so if
that fails for some stupid reason. I think we already do that.

Then, we use a station private area that mac80211 can allocate for us to
store the STA_ID for each station. Set this to the broadcast STA ID,
which is always valid in some sense, at sta_notify(add) time. Then,
asynchronously, tell the device about the station. Once that command
finishes, look up the sta struct again and set the STA_ID to the new ID
that we used in the device. This way, a sta struct will always have a
valid sta ID in it.

Now, when we need to set a key for a station, we actually get the
station struct. Thus, we can keep two separate flag in the station
struct that tells us whether the STA_ADD was successful. If this flag is
unset, then we reject the add_key with -ENOSPC. Or when we detect that
the key command was too fast after the sta_notify we can wait for the
ADD_STA to finish in the key notification since that can sleep.

And then rate stuff we can just also do as part of the async sta add
command, so that the sta ID is only set after we have the sta programmed
into the device and also initialised rate control properly for it.
Ultimately the rate control algorithm could do nothing at all, and then
we can remove it completely.

> > As soon as sta_insert() got called, a packet transmitted to that station
> > can be processed, find the sta info, and it seems we could end up
> > calling rate_control_get_rate() before the init was done, through a race
> > condition.
> 
> oh - I see - that's bad. Although, that may explain why iwlwifi is
> adding stations in get_rate() also. 

No, the explanation for that is "history" :) The rate algorithm was
written before mac80211 actually _had_ the sta_notify command, and it
was (and still is, as you've pointed out in these patches) the first
place that is notified about the new station. But I think we don't
really need that.

johannes

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

Re: [PATCH 1/2] cfg80211: initialize rate control after station inserted

[reinette chatre]

Hi Johannes,

On Sat, 2009-08-29 at 02:34 -0700, Johannes Berg wrote:
> I'd say we do the following: At startup, we program the broadcast STA
> into the device so we always have that, and do that synchronously so if
> that fails for some stupid reason. I think we already do that.

The station management in the driver is not entirely synchronous. That
is, adding a station and removing a station are not the only two
commands affecting the device's station knowledge. The often used "rxon"
command, when used without the association flag, always clears the
station table. This command is used frequently while not associated. So,
knowing when to add the broadcast station is not exact and is the reason
why it is currently done as part of the sending of the rxon command.
What I have been doing as part of testing here was to have a new
"restore stations" call which is called after sending the rxon command
to get device and driver in sync again wrt station management. If we
include something like this in the driver then it will be possible to
implement your proposal to add the broadcast station at the beginning.

> Then, we use a station private area that mac80211 can allocate for us to
> store the STA_ID for each station. Set this to the broadcast STA ID,
> which is always valid in some sense, at sta_notify(add) time. Then,
> asynchronously, tell the device about the station. Once that command
> finishes, look up the sta struct again and set the STA_ID to the new ID
> that we used in the device. This way, a sta struct will always have a
> valid sta ID in it.
> 
> Now, when we need to set a key for a station, we actually get the
> station struct. Thus, we can keep two separate flag in the station
> struct that tells us whether the STA_ADD was successful. If this flag is
> unset, then we reject the add_key with -ENOSPC. Or when we detect that
> the key command was too fast after the sta_notify we can wait for the
> ADD_STA to finish in the key notification since that can sleep.
> 
> And then rate stuff we can just also do as part of the async sta add
> command, so that the sta ID is only set after we have the sta programmed
> into the device and also initialised rate control properly for it.
> Ultimately the rate control algorithm could do nothing at all, and then
> we can remove it completely.

Thank you very much for these great suggestions. I'll look into this
implementation.

Reinette