| Message ID | 1531126681-5146-1-git-send-email-mpubbise@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
On Mon, 2018-07-09 at 14:28 +0530, Manikanta Pubbisetty wrote: > As explained in ieee80211_delayed_tailroom_dec(), during roam, > keys of the old AP will be destroyed and new keys will be > installed. Deletion of the old key causes > crypto_tx_tailroom_needed_cnt to go from 1 to 0 and the new key > installation causes a transition from 0 to 1. > > Whenever crypto_tx_tailroom_needed_cnt transitions from 0 to 1, > we invoke synchronize_net(); the reason for doing this is to avoid > a race in the TX path as explained in increment_tailroom_need_count(). > This synchronize_net() operation can be slow and can affect the station > roam time. To avoid this, decrementing the crypto_tx_tailroom_needed_cnt > is delayed for a while so that upon installation of new key the > transition would be from 1 to 2 instead of 0 to 1 and thereby > improving the roam time. Right. > This is all correct for a STA iftype, but deferring the tailroom_needed > decrement for other iftypes may be incorrect. I don't understand this. What do you mean by "incorrect"? > For example, let's consider the case of a 4-addr client connecting to > an AP for which AP_VLAN interface is also created, let the initial > value for tailroom_needed on the AP be 1. > > * 4-addr client connects to the AP (AP: tailroom_needed = 1) > * AP will clear old keys, delay decrement of tailroom_needed count > * AP_VLAN is created, it takes the tailroom count from master > (AP_VLAN: tailroom_needed = 1, AP: tailroom_needed = 1) > * Install new key for the station, assume key is plumbed in the HW, > there won't be any change in tailroom_needed count on AP iface > * Delayed decrement of tailroom_needed count on AP > (AP: tailroom_needed = 0, AP_VLAN: tailroom_needed = 1) > > Because of the delayed decrement on AP iface, tailroom_needed count goes > out of sync between AP(master iface) and AP_VLAN(slave iface) and > there would be unnecessary tailroom created for the packets going > through AP_VLAN iface. This describes a scenario where there's *unnecessary* work done, but not really one where we have something that's *incorrect*? Are you saying that you found a problem with this? Did this show up in some sort of measurements? > +++ b/net/mac80211/key.c > @@ -583,7 +583,8 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key, > > ieee80211_debugfs_key_remove(key); > > - if (delay_tailroom) { > + if (delay_tailroom && > + sdata->vif.type == NL80211_IFTYPE_STATION) { > /* see ieee80211_delayed_tailroom_dec */ > sdata->crypto_tx_tailroom_pending_dec++; > schedule_delayed_work(&sdata->dec_tailroom_needed_wk, I think you'd better change the caller, i.e. ieee80211_del_key(), and a add a comment there that explains what's going on. But I'm not yet really sure what you're trying to do :-) johannes
>> For example, let's consider the case of a 4-addr client connecting to >> an AP for which AP_VLAN interface is also created, let the initial >> value for tailroom_needed on the AP be 1. >> >> * 4-addr client connects to the AP (AP: tailroom_needed = 1) >> * AP will clear old keys, delay decrement of tailroom_needed count >> * AP_VLAN is created, it takes the tailroom count from master >> (AP_VLAN: tailroom_needed = 1, AP: tailroom_needed = 1) >> * Install new key for the station, assume key is plumbed in the HW, >> there won't be any change in tailroom_needed count on AP iface >> * Delayed decrement of tailroom_needed count on AP >> (AP: tailroom_needed = 0, AP_VLAN: tailroom_needed = 1) >> >> Because of the delayed decrement on AP iface, tailroom_needed count goes >> out of sync between AP(master iface) and AP_VLAN(slave iface) and >> there would be unnecessary tailroom created for the packets going >> through AP_VLAN iface. > This describes a scenario where there's *unnecessary* work done, but not > really one where we have something that's *incorrect*? > To me at least doing unnecessary things is incorrect :-D, may be we can change the statement. > Are you saying that you found a problem with this? Did this show up in > some sort of measurements? Not really, I have observed in my testing that there were warnings whenever a AP_VLAN was brought down; this is done in ieee80211_free_keys. Warnings show up every time we bring down the AP_VLAN interface(using ifconfig); warnings apart but I thought there would be unnecessary overhead in the tailroom expansion though the overhead may be trivial. >> +++ b/net/mac80211/key.c >> @@ -583,7 +583,8 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key, >> >> ieee80211_debugfs_key_remove(key); >> >> - if (delay_tailroom) { >> + if (delay_tailroom && >> + sdata->vif.type == NL80211_IFTYPE_STATION) { >> /* see ieee80211_delayed_tailroom_dec */ >> sdata->crypto_tx_tailroom_pending_dec++; >> schedule_delayed_work(&sdata->dec_tailroom_needed_wk, > I think you'd better change the caller, i.e. ieee80211_del_key(), and a > add a comment there that explains what's going on. Not really sure what you were trying to tell here. Manikanta
On Mon, 2018-07-09 at 14:54 +0530, Manikanta Pubbisetty wrote: > > This describes a scenario where there's *unnecessary* work done, but not > > really one where we have something that's *incorrect*? > > > > To me at least doing unnecessary things is incorrect :-D, may be we can > change the statement. Well, I guess it's a question of semantics, but this doesn't really result in any observable incorrect behaviour. > > Are you saying that you found a problem with this? Did this show up in > > some sort of measurements? > > Not really, I have observed in my testing that there were warnings > whenever a AP_VLAN was brought down; this is done in ieee80211_free_keys. > Warnings show up every time we bring down the AP_VLAN interface(using > ifconfig); warnings apart but I thought there would be unnecessary > overhead in the tailroom expansion though the overhead may be trivial. Except for that maybe :-) > > > +++ b/net/mac80211/key.c > > > @@ -583,7 +583,8 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key, > > > > > > ieee80211_debugfs_key_remove(key); > > > > > > - if (delay_tailroom) { > > > + if (delay_tailroom && > > > + sdata->vif.type == NL80211_IFTYPE_STATION) { > > > /* see ieee80211_delayed_tailroom_dec */ > > > sdata->crypto_tx_tailroom_pending_dec++; > > > schedule_delayed_work(&sdata->dec_tailroom_needed_wk, > > > > I think you'd better change the caller, i.e. ieee80211_del_key(), and a > > add a comment there that explains what's going on. > > Not really sure what you were trying to tell here. I think you should do ieee80211_key_destroy(..., type == STATION); in the caller, instead of hard-coding the thing here. There may be more places that want the delay, perhaps for other reasons? johannes
On 7/9/2018 2:56 PM, Johannes Berg wrote: > On Mon, 2018-07-09 at 14:54 +0530, Manikanta Pubbisetty wrote: > >>> This describes a scenario where there's *unnecessary* work done, but not >>> really one where we have something that's *incorrect*? >>> >> To me at least doing unnecessary things is incorrect :-D, may be we can >> change the statement. > Well, I guess it's a question of semantics, but this doesn't really > result in any observable incorrect behaviour. Okay. >>> Are you saying that you found a problem with this? Did this show up in >>> some sort of measurements? >> Not really, I have observed in my testing that there were warnings >> whenever a AP_VLAN was brought down; this is done in ieee80211_free_keys. >> Warnings show up every time we bring down the AP_VLAN interface(using >> ifconfig); warnings apart but I thought there would be unnecessary >> overhead in the tailroom expansion though the overhead may be trivial. > Except for that maybe :-) :-) >>>> +++ b/net/mac80211/key.c >>>> @@ -583,7 +583,8 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key, >>>> >>>> ieee80211_debugfs_key_remove(key); >>>> >>>> - if (delay_tailroom) { >>>> + if (delay_tailroom && >>>> + sdata->vif.type == NL80211_IFTYPE_STATION) { >>>> /* see ieee80211_delayed_tailroom_dec */ >>>> sdata->crypto_tx_tailroom_pending_dec++; >>>> schedule_delayed_work(&sdata->dec_tailroom_needed_wk, >>> I think you'd better change the caller, i.e. ieee80211_del_key(), and a >>> add a comment there that explains what's going on. >> Not really sure what you were trying to tell here. > I think you should do > > ieee80211_key_destroy(..., type == STATION); Even this would boil down to the same behavior, no? > > in the caller, instead of hard-coding the thing here. > > There may be more places that want the delay, perhaps for other reasons? I was going through the source to figure out that; but all that I could understand is this was mainly done to improve roam time.
On Mon, 2018-07-09 at 15:07 +0530, Manikanta Pubbisetty wrote: > > I think you should do > > > > ieee80211_key_destroy(..., type == STATION); > > Even this would boil down to the same behavior, no? Yes, but I think it's easier to understand than thinking "ok this will delay" when you pass the parameter as true, but then it won't unless extra conditions are met. > I was going through the source to figure out that; but all that I could > understand is this was mainly done to improve roam time. Ok, so perhaps more callers should be updated - still I think it'd be easier to understand. johannes
diff --git a/net/mac80211/key.c b/net/mac80211/key.c index ee0d0cc..d1ea86a 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -583,7 +583,8 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key, ieee80211_debugfs_key_remove(key); - if (delay_tailroom) { + if (delay_tailroom && + sdata->vif.type == NL80211_IFTYPE_STATION) { /* see ieee80211_delayed_tailroom_dec */ sdata->crypto_tx_tailroom_pending_dec++; schedule_delayed_work(&sdata->dec_tailroom_needed_wk,
As explained in ieee80211_delayed_tailroom_dec(), during roam, keys of the old AP will be destroyed and new keys will be installed. Deletion of the old key causes crypto_tx_tailroom_needed_cnt to go from 1 to 0 and the new key installation causes a transition from 0 to 1. Whenever crypto_tx_tailroom_needed_cnt transitions from 0 to 1, we invoke synchronize_net(); the reason for doing this is to avoid a race in the TX path as explained in increment_tailroom_need_count(). This synchronize_net() operation can be slow and can affect the station roam time. To avoid this, decrementing the crypto_tx_tailroom_needed_cnt is delayed for a while so that upon installation of new key the transition would be from 1 to 2 instead of 0 to 1 and thereby improving the roam time. This is all correct for a STA iftype, but deferring the tailroom_needed decrement for other iftypes may be incorrect. For example, let's consider the case of a 4-addr client connecting to an AP for which AP_VLAN interface is also created, let the initial value for tailroom_needed on the AP be 1. * 4-addr client connects to the AP (AP: tailroom_needed = 1) * AP will clear old keys, delay decrement of tailroom_needed count * AP_VLAN is created, it takes the tailroom count from master (AP_VLAN: tailroom_needed = 1, AP: tailroom_needed = 1) * Install new key for the station, assume key is plumbed in the HW, there won't be any change in tailroom_needed count on AP iface * Delayed decrement of tailroom_needed count on AP (AP: tailroom_needed = 0, AP_VLAN: tailroom_needed = 1) Because of the delayed decrement on AP iface, tailroom_needed count goes out of sync between AP(master iface) and AP_VLAN(slave iface) and there would be unnecessary tailroom created for the packets going through AP_VLAN iface. Restricting delayed decrement to station interface alone fixes the problem and it makes sense to do so because delayed decrement is done to improve roam time which is applicable only for client devices. Signed-off-by: Manikanta Pubbisetty <mpubbise@codeaurora.org> --- net/mac80211/key.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)