* [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
* [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 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
* 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.