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
