* [PATCH 0/2] make roaming a bit faster @ 2012-09-05 17:23 Eliad Peller 2012-09-05 17:23 ` [PATCH 1/2] mac80211: use synchronize_net() on key destroying Eliad Peller 2012-09-05 17:23 ` [PATCH 2/2] mac80211: use call_rcu() on sta deletion Eliad Peller 0 siblings, 2 replies; 7+ messages in thread From: Eliad Peller @ 2012-09-05 17:23 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless The use of synchronize_rcu during roaming adds a significant delay. try minimizing it by replacing these calls with other alternatives. Eliad Peller (2): mac80211: use synchronize_net() on key destroying mac80211: use call_rcu() on sta deletion net/mac80211/iface.c | 5 ++ net/mac80211/key.c | 2 +- net/mac80211/sta_info.c | 117 ++++++++++++++++++++++++++--------------------- net/mac80211/sta_info.h | 2 + 4 files changed, 73 insertions(+), 53 deletions(-) -- 1.7.6.401.g6a319 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] mac80211: use synchronize_net() on key destroying 2012-09-05 17:23 [PATCH 0/2] make roaming a bit faster Eliad Peller @ 2012-09-05 17:23 ` Eliad Peller 2012-09-06 15:38 ` Johannes Berg 2012-09-05 17:23 ` [PATCH 2/2] mac80211: use call_rcu() on sta deletion Eliad Peller 1 sibling, 1 reply; 7+ messages in thread From: Eliad Peller @ 2012-09-05 17:23 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless __ieee80211_key_destroy() calls synchronize_rcu() in order to sync the tx path before destroying the key. However, synching the tx path can be done with synchronize_net() as well, which is usually faster (the timing might be important for roaming scenarios). Signed-off-by: Eliad Peller <eliad@wizery.com> --- net/mac80211/key.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/net/mac80211/key.c b/net/mac80211/key.c index f86d09d..3d29d34 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -406,7 +406,7 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key) * Synchronize so the TX path can no longer be using * this key before we free/remove it. */ - synchronize_rcu(); + synchronize_net(); if (key->local) ieee80211_key_disable_hw_accel(key); -- 1.7.6.401.g6a319 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mac80211: use synchronize_net() on key destroying 2012-09-05 17:23 ` [PATCH 1/2] mac80211: use synchronize_net() on key destroying Eliad Peller @ 2012-09-06 15:38 ` Johannes Berg 0 siblings, 0 replies; 7+ messages in thread From: Johannes Berg @ 2012-09-06 15:38 UTC (permalink / raw) To: Eliad Peller; +Cc: linux-wireless On Wed, 2012-09-05 at 20:23 +0300, Eliad Peller wrote: > __ieee80211_key_destroy() calls synchronize_rcu() in > order to sync the tx path before destroying the key. > > However, synching the tx path can be done with > synchronize_net() as well, which is usually faster > (the timing might be important for roaming scenarios). Applied. johannes ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] mac80211: use call_rcu() on sta deletion 2012-09-05 17:23 [PATCH 0/2] make roaming a bit faster Eliad Peller 2012-09-05 17:23 ` [PATCH 1/2] mac80211: use synchronize_net() on key destroying Eliad Peller @ 2012-09-05 17:23 ` Eliad Peller 2012-09-06 15:41 ` Johannes Berg 1 sibling, 1 reply; 7+ messages in thread From: Eliad Peller @ 2012-09-05 17:23 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless mac80211 calls synchronize_rcu() on sta deletion, which increase the roaming time significantly. Convert it into a call_rcu() mechanism, in order to avoid blocking. Since some of the cleanup functions might sleep, schedule from the call_rcu callback a new work that will do the actual cleanup. In order to make sure the cleanup occurs before the interface went down, flush local->workqueue on ieee80211_do_stop(). Signed-off-by: Yoni Divinsky <yoni.divinsky@ti.com> Signed-off-by: Eliad Peller <eliad@wizery.com> --- net/mac80211/iface.c | 4 ++ net/mac80211/sta_info.c | 121 ++++++++++++++++++++++++++--------------------- net/mac80211/sta_info.h | 2 + 3 files changed, 73 insertions(+), 54 deletions(-) diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 513b85e..d13bde4 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -774,6 +774,10 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, * frames at this very time on another CPU. */ synchronize_rcu(); + + /* flush any remaining cleanup work */ + flush_workqueue(local->workqueue); + skb_queue_purge(&sdata->skb_queue); /* diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 06fa75c..354326d 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -91,6 +91,70 @@ static int sta_info_hash_del(struct ieee80211_local *local, return -ENOENT; } +static void free_sta_work(struct work_struct *wk) +{ + struct sta_info *sta = container_of(wk, struct sta_info, free_sta_wk); + int ac, i; + struct tid_ampdu_tx *tid_tx; + struct ieee80211_sub_if_data *sdata = sta->sdata; + struct ieee80211_local *local = sdata->local; + + /* + * At this point, after we wait for an RCU grace period, + * neither mac80211 nor the driver can reference this + * sta struct any more except by still existing timers + * associated with this station that we clean up below. + */ + + if (test_sta_flag(sta, WLAN_STA_PS_STA)) { + BUG_ON(!sdata->bss); + + clear_sta_flag(sta, WLAN_STA_PS_STA); + + atomic_dec(&sdata->bss->num_sta_ps); + sta_info_recalc_tim(sta); + } + + for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) { + local->total_ps_buffered -= skb_queue_len(&sta->ps_tx_buf[ac]); + __skb_queue_purge(&sta->ps_tx_buf[ac]); + __skb_queue_purge(&sta->tx_filtered[ac]); + } + +#ifdef CONFIG_MAC80211_MESH + if (ieee80211_vif_is_mesh(&sdata->vif)) { + mesh_accept_plinks_update(sdata); + mesh_plink_deactivate(sta); + del_timer_sync(&sta->plink_timer); + } +#endif + + cancel_work_sync(&sta->drv_unblock_wk); + + /* + * Destroy aggregation state here. It would be nice to wait for the + * driver to finish aggregation stop and then clean up, but for now + * drivers have to handle aggregation stop being requested, followed + * directly by station destruction. + */ + for (i = 0; i < STA_TID_NUM; i++) { + tid_tx = rcu_dereference_raw(sta->ampdu_mlme.tid_tx[i]); + if (!tid_tx) + continue; + __skb_queue_purge(&tid_tx->pending); + kfree(tid_tx); + } + + sta_info_free(local, sta); +} + +static void free_sta_rcu(struct rcu_head *h) +{ + struct sta_info *sta = container_of(h, struct sta_info, rcu_head); + + ieee80211_queue_work(&sta->local->hw, &sta->free_sta_wk); +} + /* protected by RCU */ struct sta_info *sta_info_get(struct ieee80211_sub_if_data *sdata, const u8 *addr) @@ -241,6 +305,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata, spin_lock_init(&sta->lock); INIT_WORK(&sta->drv_unblock_wk, sta_unblock); + INIT_WORK(&sta->free_sta_wk, free_sta_work); INIT_WORK(&sta->ampdu_mlme.work, ieee80211_ba_session_work); mutex_init(&sta->ampdu_mlme.mtx); @@ -654,8 +719,7 @@ int __must_check __sta_info_destroy(struct sta_info *sta) { struct ieee80211_local *local; struct ieee80211_sub_if_data *sdata; - int ret, i, ac; - struct tid_ampdu_tx *tid_tx; + int ret, i; might_sleep(); @@ -711,65 +775,14 @@ int __must_check __sta_info_destroy(struct sta_info *sta) WARN_ON_ONCE(ret != 0); } - /* - * At this point, after we wait for an RCU grace period, - * neither mac80211 nor the driver can reference this - * sta struct any more except by still existing timers - * associated with this station that we clean up below. - */ - synchronize_rcu(); - - if (test_sta_flag(sta, WLAN_STA_PS_STA)) { - BUG_ON(!sdata->bss); - - clear_sta_flag(sta, WLAN_STA_PS_STA); - - atomic_dec(&sdata->bss->num_sta_ps); - sta_info_recalc_tim(sta); - } - - for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) { - local->total_ps_buffered -= skb_queue_len(&sta->ps_tx_buf[ac]); - __skb_queue_purge(&sta->ps_tx_buf[ac]); - __skb_queue_purge(&sta->tx_filtered[ac]); - } - -#ifdef CONFIG_MAC80211_MESH - if (ieee80211_vif_is_mesh(&sdata->vif)) - mesh_accept_plinks_update(sdata); -#endif - sta_dbg(sdata, "Removed STA %pM\n", sta->sta.addr); - cancel_work_sync(&sta->drv_unblock_wk); - cfg80211_del_sta(sdata->dev, sta->sta.addr, GFP_KERNEL); rate_control_remove_sta_debugfs(sta); ieee80211_sta_debugfs_remove(sta); -#ifdef CONFIG_MAC80211_MESH - if (ieee80211_vif_is_mesh(&sta->sdata->vif)) { - mesh_plink_deactivate(sta); - del_timer_sync(&sta->plink_timer); - } -#endif - - /* - * Destroy aggregation state here. It would be nice to wait for the - * driver to finish aggregation stop and then clean up, but for now - * drivers have to handle aggregation stop being requested, followed - * directly by station destruction. - */ - for (i = 0; i < STA_TID_NUM; i++) { - tid_tx = rcu_dereference_raw(sta->ampdu_mlme.tid_tx[i]); - if (!tid_tx) - continue; - __skb_queue_purge(&tid_tx->pending); - kfree(tid_tx); - } - - sta_info_free(local, sta); + call_rcu(&sta->rcu_head, free_sta_rcu); return 0; } diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h index a470e11..c88f161f 100644 --- a/net/mac80211/sta_info.h +++ b/net/mac80211/sta_info.h @@ -287,6 +287,7 @@ struct sta_ampdu_mlme { struct sta_info { /* General information, mostly static */ struct list_head list; + struct rcu_head rcu_head; struct sta_info __rcu *hnext; struct ieee80211_local *local; struct ieee80211_sub_if_data *sdata; @@ -297,6 +298,7 @@ struct sta_info { spinlock_t lock; struct work_struct drv_unblock_wk; + struct work_struct free_sta_wk; u16 listen_interval; -- 1.7.6.401.g6a319 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] mac80211: use call_rcu() on sta deletion 2012-09-05 17:23 ` [PATCH 2/2] mac80211: use call_rcu() on sta deletion Eliad Peller @ 2012-09-06 15:41 ` Johannes Berg 2012-09-06 21:17 ` Eliad Peller 0 siblings, 1 reply; 7+ messages in thread From: Johannes Berg @ 2012-09-06 15:41 UTC (permalink / raw) To: Eliad Peller; +Cc: linux-wireless On Wed, 2012-09-05 at 20:23 +0300, Eliad Peller wrote: > +++ b/net/mac80211/iface.c > @@ -774,6 +774,10 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, > * frames at this very time on another CPU. > */ > synchronize_rcu(); > + > + /* flush any remaining cleanup work */ > + flush_workqueue(local->workqueue); I have a feeling we need rcu_barrier() here instead of synchronize_rcu() now to guarantee that all the potentially pending call_rcu() functions from the sta_info_flush() above have actually completed and queued the work items that we want to flush here. Right? That's a bit unfortunate, but I guess the best way of handling it and ifdown should be rare anyway compared to everything else. I think it'd also be good to make the comment more verbose and explain which cleanup work. > +static void free_sta_work(struct work_struct *wk) > +{ > + struct sta_info *sta = container_of(wk, struct sta_info, free_sta_wk); > + int ac, i; > + struct tid_ampdu_tx *tid_tx; > + struct ieee80211_sub_if_data *sdata = sta->sdata; > + struct ieee80211_local *local = sdata->local; > + > + /* > + * At this point, after we wait for an RCU grace period, > + * neither mac80211 nor the driver can reference this > + * sta struct any more except by still existing timers > + * associated with this station that we clean up below. > + */ Should probably rewrite the comment a bit to indicate that we have in fact waited since we get called via call_rcu(). > + cancel_work_sync(&sta->drv_unblock_wk); This will deadlock now, but cancel_work() will be safe without the sync since the workqueue is single-threaded. johannes ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] mac80211: use call_rcu() on sta deletion 2012-09-06 15:41 ` Johannes Berg @ 2012-09-06 21:17 ` Eliad Peller 2012-09-07 7:34 ` Johannes Berg 0 siblings, 1 reply; 7+ messages in thread From: Eliad Peller @ 2012-09-06 21:17 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless On Thu, Sep 6, 2012 at 6:41 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Wed, 2012-09-05 at 20:23 +0300, Eliad Peller wrote: > >> +++ b/net/mac80211/iface.c >> @@ -774,6 +774,10 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, >> * frames at this very time on another CPU. >> */ >> synchronize_rcu(); >> + >> + /* flush any remaining cleanup work */ >> + flush_workqueue(local->workqueue); > > I have a feeling we need rcu_barrier() here instead of synchronize_rcu() > now to guarantee that all the potentially pending call_rcu() functions > from the sta_info_flush() above have actually completed and queued the > work items that we want to flush here. Right? That's a bit unfortunate, > but I guess the best way of handling it and ifdown should be rare anyway > compared to everything else. > i wrongly assumed synchronize_rcu() takes care of call_rcu() callbacks as well. thanks for clarifying this. > I think it'd also be good to make the comment more verbose and explain > which cleanup work. > sure. >> +static void free_sta_work(struct work_struct *wk) >> +{ >> + struct sta_info *sta = container_of(wk, struct sta_info, free_sta_wk); >> + int ac, i; >> + struct tid_ampdu_tx *tid_tx; >> + struct ieee80211_sub_if_data *sdata = sta->sdata; >> + struct ieee80211_local *local = sdata->local; >> + >> + /* >> + * At this point, after we wait for an RCU grace period, >> + * neither mac80211 nor the driver can reference this >> + * sta struct any more except by still existing timers >> + * associated with this station that we clean up below. >> + */ > > Should probably rewrite the comment a bit to indicate that we have in > fact waited since we get called via call_rcu(). > sure. >> + cancel_work_sync(&sta->drv_unblock_wk); > > This will deadlock now, but cancel_work() will be safe without the sync > since the workqueue is single-threaded. > i guess that because we run on an ordered workqueue we can't really deadlock, but sure, i'll change it. thanks for the review. Eliad. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] mac80211: use call_rcu() on sta deletion 2012-09-06 21:17 ` Eliad Peller @ 2012-09-07 7:34 ` Johannes Berg 0 siblings, 0 replies; 7+ messages in thread From: Johannes Berg @ 2012-09-07 7:34 UTC (permalink / raw) To: Eliad Peller; +Cc: linux-wireless On Fri, 2012-09-07 at 00:17 +0300, Eliad Peller wrote: > >> + cancel_work_sync(&sta->drv_unblock_wk); > > > > This will deadlock now, but cancel_work() will be safe without the sync > > since the workqueue is single-threaded. Good point, I confused it with flush_work(). johannes ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-09-07 7:33 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-09-05 17:23 [PATCH 0/2] make roaming a bit faster Eliad Peller 2012-09-05 17:23 ` [PATCH 1/2] mac80211: use synchronize_net() on key destroying Eliad Peller 2012-09-06 15:38 ` Johannes Berg 2012-09-05 17:23 ` [PATCH 2/2] mac80211: use call_rcu() on sta deletion Eliad Peller 2012-09-06 15:41 ` Johannes Berg 2012-09-06 21:17 ` Eliad Peller 2012-09-07 7:34 ` Johannes Berg
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.