| Message ID | 1251416094-10420-1-git-send-email-reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);