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