[PATCH 0/5] mac80211: fix station cleanup/destruction

[PATCH 0/5] mac80211: fix station cleanup/destruction

[Johannes Berg]

Turns out that not only was flush_workqueue() slow as reported
by Ben, it was also broken because this is way too late to
finish the station destruction in AP mode.

Fixing this with keeping the roaming semantics (no RCU barrier
or synchronize in there) is a bit tricky so the first patch just
fixes the problem, and the last patch puts back the optimisation.

The patches inbetween are just padding ;-)

johannes

[PATCH 1/5] mac80211: fix station destruction in AP/mesh modes

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Unfortunately, commit b22cfcfcae5b, intended to speed up roaming
by avoiding the synchronize_rcu() broke AP/mesh modes as it moved
some code into that work item that will still call into the driver
at a time where it's no longer expected to handle this: after the
AP or mesh has been stopped.

To fix this problem remove the per-station work struct, maintain a
station cleanup list instead and flush this list when stations are
flushed. To keep this patch smaller for stable, do this when the
stations are flushed (sta_info_flush()). This unfortunately brings
back the original roaming delay; I'll fix that again in a separate
patch.

Also, Ben reported that the original commit could sometimes (with
many interfaces) cause long delays when an interface is set down,
due to blocking on flush_workqueue(). Since we now maintain the
cleanup list, this particular change of the original patch can be
reverted.

Cc: stable@vger.kernel.org [3.7]
Reported-by: Ben Greear <greearb@candelatech.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/ieee80211_i.h |  4 ++++
 net/mac80211/iface.c       | 28 ++++++++++++++++------------
 net/mac80211/sta_info.c    | 44 ++++++++++++++++++++++++++++++++++++++++----
 net/mac80211/sta_info.h    |  3 ++-
 4 files changed, 62 insertions(+), 17 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 47875e0..fe86472 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -773,6 +773,10 @@ struct ieee80211_sub_if_data {
 		u32 mntr_flags;
 	} u;
 
+	spinlock_t cleanup_stations_lock;
+	struct list_head cleanup_stations;
+	struct work_struct cleanup_stations_wk;
+
 #ifdef CONFIG_MAC80211_DEBUGFS
 	struct {
 		struct dentry *dir;
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 54fb7f9..0f2a9f9 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -868,20 +868,11 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 		cancel_work_sync(&sdata->work);
 		/*
 		 * When we get here, the interface is marked down.
-		 * Call rcu_barrier() to wait both for the RX path
+		 * Call synchronize_rcu() to wait for the RX path
 		 * should it be using the interface and enqueuing
-		 * frames at this very time on another CPU, and
-		 * for the sta free call_rcu callbacks.
+		 * frames at this very time on another CPU.
 		 */
-		rcu_barrier();
-
-		/*
-		 * free_sta_rcu() enqueues a work for the actual
-		 * sta cleanup, so we need to flush it while
-		 * sdata is still valid.
-		 */
-		flush_workqueue(local->workqueue);
-
+		synchronize_rcu();
 		skb_queue_purge(&sdata->skb_queue);
 
 		/*
@@ -1501,6 +1492,15 @@ static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
 	mutex_unlock(&local->iflist_mtx);
 }
 
+static void ieee80211_cleanup_sdata_stas_wk(struct work_struct *wk)
+{
+	struct ieee80211_sub_if_data *sdata;
+
+	sdata = container_of(wk, struct ieee80211_sub_if_data, cleanup_stations_wk);
+
+	ieee80211_cleanup_sdata_stas(sdata);
+}
+
 int ieee80211_if_add(struct ieee80211_local *local, const char *name,
 		     struct wireless_dev **new_wdev, enum nl80211_iftype type,
 		     struct vif_params *params)
@@ -1576,6 +1576,10 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
 
 	INIT_LIST_HEAD(&sdata->key_list);
 
+	spin_lock_init(&sdata->cleanup_stations_lock);
+	INIT_LIST_HEAD(&sdata->cleanup_stations);
+	INIT_WORK(&sdata->cleanup_stations_wk, ieee80211_cleanup_sdata_stas_wk);
+
 	for (i = 0; i < IEEE80211_NUM_BANDS; i++) {
 		struct ieee80211_supported_band *sband;
 		sband = local->hw.wiphy->bands[i];
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index dadcfcf..0f709d54 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -91,9 +91,8 @@ static int sta_info_hash_del(struct ieee80211_local *local,
 	return -ENOENT;
 }
 
-static void free_sta_work(struct work_struct *wk)
+static void cleanup_single_sta(struct sta_info *sta)
 {
-	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;
@@ -153,11 +152,35 @@ static void free_sta_work(struct work_struct *wk)
 	sta_info_free(local, sta);
 }
 
+void ieee80211_cleanup_sdata_stas(struct ieee80211_sub_if_data *sdata)
+{
+	struct sta_info *sta;
+
+	spin_lock_bh(&sdata->cleanup_stations_lock);
+	while (!list_empty(&sdata->cleanup_stations)) {
+		sta = list_first_entry(&sdata->cleanup_stations,
+				       struct sta_info, list);
+		list_del(&sta->list);
+		spin_unlock_bh(&sdata->cleanup_stations_lock);
+
+		cleanup_single_sta(sta);
+
+		spin_lock_bh(&sdata->cleanup_stations_lock);
+	}
+
+	spin_unlock_bh(&sdata->cleanup_stations_lock);
+}
+
 static void free_sta_rcu(struct rcu_head *h)
 {
 	struct sta_info *sta = container_of(h, struct sta_info, rcu_head);
+	struct ieee80211_sub_if_data *sdata = sta->sdata;
 
-	ieee80211_queue_work(&sta->local->hw, &sta->free_sta_wk);
+	spin_lock(&sdata->cleanup_stations_lock);
+	list_add_tail(&sta->list, &sdata->cleanup_stations);
+	spin_unlock(&sdata->cleanup_stations_lock);
+
+	ieee80211_queue_work(&sdata->local->hw, &sdata->cleanup_stations_wk);
 }
 
 /* protected by RCU */
@@ -310,7 +333,6 @@ 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);
 
@@ -891,6 +913,20 @@ int sta_info_flush(struct ieee80211_local *local,
 	}
 	mutex_unlock(&local->sta_mtx);
 
+	rcu_barrier();
+
+	if (sdata) {
+		ieee80211_cleanup_sdata_stas(sdata);
+		cancel_work_sync(&sdata->cleanup_stations_wk);
+	} else {
+		mutex_lock(&local->iflist_mtx);
+		list_for_each_entry(sdata, &local->interfaces, list) {
+			ieee80211_cleanup_sdata_stas(sdata);
+			cancel_work_sync(&sdata->cleanup_stations_wk);
+		}
+		mutex_unlock(&local->iflist_mtx);
+	}
+
 	return ret;
 }
 
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 1489bca..37c1889 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -299,7 +299,6 @@ struct sta_info {
 	spinlock_t lock;
 
 	struct work_struct drv_unblock_wk;
-	struct work_struct free_sta_wk;
 
 	u16 listen_interval;
 
@@ -563,4 +562,6 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta);
 void ieee80211_sta_ps_deliver_poll_response(struct sta_info *sta);
 void ieee80211_sta_ps_deliver_uapsd(struct sta_info *sta);
 
+void ieee80211_cleanup_sdata_stas(struct ieee80211_sub_if_data *sdata);
+
 #endif /* STA_INFO_H */
-- 
1.8.0

[PATCH 2/5] mac80211: use del_timer_sync for final sta cleanup timer deletion

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

This is a very old bug, but there's nothing that prevents the
timer from running while the module is being removed when we
only do del_timer() instead of del_timer_sync().

The timer should normally not be running at this point, but
it's not clearly impossible (or we could just remove this.)

Cc: stable@vger.kernel.org
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/sta_info.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 0f709d54..527453d 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -884,7 +884,7 @@ void sta_info_init(struct ieee80211_local *local)
 
 void sta_info_stop(struct ieee80211_local *local)
 {
-	del_timer(&local->sta_cleanup);
+	del_timer_sync(&local->sta_cleanup);
 	sta_info_flush(local, NULL);
 }
 
-- 
1.8.0

[PATCH 3/5] mac80211: remove final sta_info_flush()

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

When all interfaces have been removed, there can't
be any stations left over, so there's no need to
flush again. Remove this, and all code associated
with it, which also simplifies the function.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/cfg.c      |  7 +++----
 net/mac80211/ibss.c     |  4 ++--
 net/mac80211/iface.c    |  5 ++---
 net/mac80211/mesh.c     |  2 +-
 net/mac80211/mlme.c     |  2 +-
 net/mac80211/sta_info.c | 23 ++++++-----------------
 net/mac80211/sta_info.h |  3 +--
 7 files changed, 16 insertions(+), 30 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index a813735..58a4362 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1010,8 +1010,8 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 		kfree_rcu(old_probe_resp, rcu_head);
 
 	list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list)
-		sta_info_flush(local, vlan);
-	sta_info_flush(local, sdata);
+		sta_info_flush(vlan);
+	sta_info_flush(sdata);
 	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BEACON_ENABLED);
 
 	drv_stop_ap(sdata->local, sdata);
@@ -1301,7 +1301,6 @@ static int ieee80211_add_station(struct wiphy *wiphy, struct net_device *dev,
 static int ieee80211_del_station(struct wiphy *wiphy, struct net_device *dev,
 				 u8 *mac)
 {
-	struct ieee80211_local *local = wiphy_priv(wiphy);
 	struct ieee80211_sub_if_data *sdata;
 
 	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
@@ -1309,7 +1308,7 @@ static int ieee80211_del_station(struct wiphy *wiphy, struct net_device *dev,
 	if (mac)
 		return sta_info_destroy_addr_bss(sdata, mac);
 
-	sta_info_flush(local, sdata);
+	sta_info_flush(sdata);
 	return 0;
 }
 
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 5e54c15..6cd6979 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -67,7 +67,7 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 	skb_reserve(skb, sdata->local->hw.extra_tx_headroom);
 
 	if (!ether_addr_equal(ifibss->bssid, bssid))
-		sta_info_flush(sdata->local, sdata);
+		sta_info_flush(sdata);
 
 	/* if merging, indicate to driver that we leave the old IBSS */
 	if (sdata->vif.bss_conf.ibss_joined) {
@@ -1182,7 +1182,7 @@ int ieee80211_ibss_leave(struct ieee80211_sub_if_data *sdata)
 		}
 	}
 
-	sta_info_flush(sdata->local, sdata);
+	sta_info_flush(sdata);
 
 	spin_lock_bh(&ifibss->incomplete_lock);
 	while (!list_empty(&ifibss->incomplete_stations)) {
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 0f2a9f9..36109eb 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -785,7 +785,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 	 * all other modes we've already removed all stations when
 	 * disconnecting etc.
 	 */
-	sta_info_flush(local, sdata);
+	sta_info_flush(sdata);
 
 	/*
 	 * Don't count this interface for promisc/allmulti while it
@@ -970,7 +970,6 @@ static void ieee80211_set_multicast_list(struct net_device *dev)
  */
 static void ieee80211_teardown_sdata(struct ieee80211_sub_if_data *sdata)
 {
-	struct ieee80211_local *local = sdata->local;
 	int flushed;
 	int i;
 
@@ -986,7 +985,7 @@ static void ieee80211_teardown_sdata(struct ieee80211_sub_if_data *sdata)
 	if (ieee80211_vif_is_mesh(&sdata->vif))
 		mesh_rmc_free(sdata);
 
-	flushed = sta_info_flush(local, sdata);
+	flushed = sta_info_flush(sdata);
 	WARN_ON(flushed);
 }
 
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 9b00c2c..f5f01e0 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -656,7 +656,7 @@ void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
 	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BEACON_ENABLED);
 
 	/* flush STAs and mpaths on this iface */
-	sta_info_flush(sdata->local, sdata);
+	sta_info_flush(sdata);
 	mesh_path_flush_by_iface(sdata);
 
 	del_timer_sync(&sdata->u.mesh.housekeeping_timer);
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 7753a9c..7cbb4cb 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1518,7 +1518,7 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 	memset(ifmgd->bssid, 0, ETH_ALEN);
 
 	/* remove AP and TDLS peers */
-	sta_info_flush(local, sdata);
+	sta_info_flush(sdata);
 
 	/* finally reset all BSS / config parameters */
 	changed |= ieee80211_reset_erp_info(sdata);
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 527453d..1a84a52 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -885,7 +885,6 @@ void sta_info_init(struct ieee80211_local *local)
 void sta_info_stop(struct ieee80211_local *local)
 {
 	del_timer_sync(&local->sta_cleanup);
-	sta_info_flush(local, NULL);
 }
 
 /**
@@ -893,12 +892,11 @@ void sta_info_stop(struct ieee80211_local *local)
  *
  * Returns the number of removed STA entries.
  *
- * @local: local interface data
- * @sdata: matching rule for the net device (sta->dev) or %NULL to match all STAs
+ * @sdata: sdata to remove all stations from
  */
-int sta_info_flush(struct ieee80211_local *local,
-		   struct ieee80211_sub_if_data *sdata)
+int sta_info_flush(struct ieee80211_sub_if_data *sdata)
 {
+	struct ieee80211_local *local = sdata->local;
 	struct sta_info *sta, *tmp;
 	int ret = 0;
 
@@ -906,7 +904,7 @@ int sta_info_flush(struct ieee80211_local *local,
 
 	mutex_lock(&local->sta_mtx);
 	list_for_each_entry_safe(sta, tmp, &local->sta_list, list) {
-		if (!sdata || sdata == sta->sdata) {
+		if (sdata == sta->sdata) {
 			WARN_ON(__sta_info_destroy(sta));
 			ret++;
 		}
@@ -915,17 +913,8 @@ int sta_info_flush(struct ieee80211_local *local,
 
 	rcu_barrier();
 
-	if (sdata) {
-		ieee80211_cleanup_sdata_stas(sdata);
-		cancel_work_sync(&sdata->cleanup_stations_wk);
-	} else {
-		mutex_lock(&local->iflist_mtx);
-		list_for_each_entry(sdata, &local->interfaces, list) {
-			ieee80211_cleanup_sdata_stas(sdata);
-			cancel_work_sync(&sdata->cleanup_stations_wk);
-		}
-		mutex_unlock(&local->iflist_mtx);
-	}
+	ieee80211_cleanup_sdata_stas(sdata);
+	cancel_work_sync(&sdata->cleanup_stations_wk);
 
 	return ret;
 }
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 37c1889..13698da 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -548,8 +548,7 @@ void sta_info_recalc_tim(struct sta_info *sta);
 
 void sta_info_init(struct ieee80211_local *local);
 void sta_info_stop(struct ieee80211_local *local);
-int sta_info_flush(struct ieee80211_local *local,
-		   struct ieee80211_sub_if_data *sdata);
+int sta_info_flush(struct ieee80211_sub_if_data *sdata);
 void sta_set_rate_info_tx(struct sta_info *sta,
 			  const struct ieee80211_tx_rate *rate,
 			  struct rate_info *rinfo);
-- 
1.8.0

[PATCH 4/5] mac80211: warn if unexpectedly removing stations

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

When an interface is brought down it must have been
disconnected (or similar) in all modes other than WDS,
so warn if any stations were removed in other modes.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/iface.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 36109eb..9dacc3a 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -756,7 +756,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 	unsigned long flags;
 	struct sk_buff *skb, *tmp;
 	u32 hw_reconf_flags = 0;
-	int i;
+	int i, flushed;
 
 	clear_bit(SDATA_STATE_RUNNING, &sdata->state);
 
@@ -781,11 +781,13 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 	 * (because if we remove a STA after ops->remove_interface()
 	 * the driver will have removed the vif info already!)
 	 *
-	 * This is relevant only in AP, WDS and mesh modes, since in
-	 * all other modes we've already removed all stations when
-	 * disconnecting etc.
+	 * This is relevant only in WDS mode, in all other modes we've
+	 * already removed all stations when disconnecting or similar,
+	 * so warn otherwise.
 	 */
-	sta_info_flush(sdata);
+	flushed = sta_info_flush(sdata);
+	WARN_ON_ONCE((sdata->vif.type != NL80211_IFTYPE_WDS && flushed > 0) ||
+		     (sdata->vif.type == NL80211_IFTYPE_WDS && flushed != 1));
 
 	/*
 	 * Don't count this interface for promisc/allmulti while it
-- 
1.8.0

[PATCH 5/5] mac80211: optimise roaming time again

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

The last fixes re-added the RCU synchronize penalty
on roaming to fix the races. Split up sta_info_flush()
now to get rid of that again, and let managed mode
(and only it) delay the actual destruction.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/iface.c    | 15 ++++++++++-----
 net/mac80211/mlme.c     |  2 +-
 net/mac80211/sta_info.c | 27 +++++++++++++++++----------
 net/mac80211/sta_info.h | 21 ++++++++++++++++++++-
 4 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 9dacc3a..ab47df0 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -784,8 +784,10 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 	 * This is relevant only in WDS mode, in all other modes we've
 	 * already removed all stations when disconnecting or similar,
 	 * so warn otherwise.
+	 *
+	 * We call sta_info_flush_cleanup() later, to combine RCU waits.
 	 */
-	flushed = sta_info_flush(sdata);
+	flushed = sta_info_flush_defer(sdata);
 	WARN_ON_ONCE((sdata->vif.type != NL80211_IFTYPE_WDS && flushed > 0) ||
 		     (sdata->vif.type == NL80211_IFTYPE_WDS && flushed != 1));
 
@@ -870,11 +872,14 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 		cancel_work_sync(&sdata->work);
 		/*
 		 * When we get here, the interface is marked down.
-		 * Call synchronize_rcu() to wait for the RX path
-		 * should it be using the interface and enqueuing
-		 * frames at this very time on another CPU.
+		 * sta_info_flush_cleanup() calls rcu_barrier to
+		 * wait for the station call_rcu() calls to complete,
+		 * here we require it to wait for the RX path in case
+		 * it is using the interface and enqueuing frames at
+		 * this very time on another CPU.
 		 */
-		synchronize_rcu();
+		sta_info_flush_cleanup(sdata);
+
 		skb_queue_purge(&sdata->skb_queue);
 
 		/*
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 7cbb4cb..964c33e 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1518,7 +1518,7 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 	memset(ifmgd->bssid, 0, ETH_ALEN);
 
 	/* remove AP and TDLS peers */
-	sta_info_flush(sdata);
+	sta_info_flush_defer(sdata);
 
 	/* finally reset all BSS / config parameters */
 	changed |= ieee80211_reset_erp_info(sdata);
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 1a84a52..2f74031 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -104,6 +104,16 @@ static void cleanup_single_sta(struct sta_info *sta)
 	 * 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.
+	 *
+	 * Note though that this still uses the sdata and even
+	 * calls the driver in AP and mesh mode, so interfaces
+	 * of those types mush use call sta_info_flush_cleanup()
+	 * (typically via sta_info_flush()) before deconfiguring
+	 * the driver.
+	 *
+	 * In station mode, nothing happens here so it doesn't
+	 * have to (and doesn't) do that, this is intentional to
+	 * speed up roaming.
 	 */
 
 	if (test_sta_flag(sta, WLAN_STA_PS_STA)) {
@@ -887,14 +897,8 @@ void sta_info_stop(struct ieee80211_local *local)
 	del_timer_sync(&local->sta_cleanup);
 }
 
-/**
- * sta_info_flush - flush matching STA entries from the STA table
- *
- * Returns the number of removed STA entries.
- *
- * @sdata: sdata to remove all stations from
- */
-int sta_info_flush(struct ieee80211_sub_if_data *sdata)
+
+int sta_info_flush_defer(struct ieee80211_sub_if_data *sdata)
 {
 	struct ieee80211_local *local = sdata->local;
 	struct sta_info *sta, *tmp;
@@ -911,12 +915,15 @@ int sta_info_flush(struct ieee80211_sub_if_data *sdata)
 	}
 	mutex_unlock(&local->sta_mtx);
 
+	return ret;
+}
+
+void sta_info_flush_cleanup(struct ieee80211_sub_if_data *sdata)
+{
 	rcu_barrier();
 
 	ieee80211_cleanup_sdata_stas(sdata);
 	cancel_work_sync(&sdata->cleanup_stations_wk);
-
-	return ret;
 }
 
 void ieee80211_sta_expire(struct ieee80211_sub_if_data *sdata,
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 13698da..93e8ae7 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -548,7 +548,26 @@ void sta_info_recalc_tim(struct sta_info *sta);
 
 void sta_info_init(struct ieee80211_local *local);
 void sta_info_stop(struct ieee80211_local *local);
-int sta_info_flush(struct ieee80211_sub_if_data *sdata);
+int sta_info_flush_defer(struct ieee80211_sub_if_data *sdata);
+void sta_info_flush_cleanup(struct ieee80211_sub_if_data *sdata);
+
+/**
+ * sta_info_flush - flush matching STA entries from the STA table
+ *
+ * Returns the number of removed STA entries.
+ *
+ * @sdata: sdata to remove all stations from
+ */
+static inline int sta_info_flush(struct ieee80211_sub_if_data *sdata)
+{
+	int ret = sta_info_flush_defer(sdata);
+
+	if (ret)
+		sta_info_flush_cleanup(sdata);
+
+	return ret;
+}
+
 void sta_set_rate_info_tx(struct sta_info *sta,
 			  const struct ieee80211_tx_rate *rate,
 			  struct rate_info *rinfo);
-- 
1.8.0

Re: [PATCH 2/5] mac80211: use del_timer_sync for final sta cleanup timer deletion

[Johannes Berg]

On Thu, 2012-12-13 at 23:59 +0100, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> This is a very old bug, but there's nothing that prevents the
> timer from running while the module is being removed when we
> only do del_timer() instead of del_timer_sync().
> 
> The timer should normally not be running at this point, but
> it's not clearly impossible (or we could just remove this.)

Ok it's very unlikely -- the timer would have to rearm itself and the
fire the exact same moment we try to del_timer() it, so it's still
running when we return, and then we'd have to unload the module quickly
enough to invalidate that code that's running there ... it's a bit of a
stretch, but not completely impossible.

johannes

Re: [PATCH 5/5] mac80211: optimise roaming time again

[Eliad Peller]

On Fri, Dec 14, 2012 at 12:59 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> The last fixes re-added the RCU synchronize penalty
> on roaming to fix the races. Split up sta_info_flush()
> now to get rid of that again, and let managed mode
> (and only it) delay the actual destruction.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
The patchset looks good. thanks for fixing it :)

> +/**
> + * sta_info_flush - flush matching STA entries from the STA table
> + *
> + * Returns the number of removed STA entries.
> + *
> + * @sdata: sdata to remove all stations from
> + */
> +static inline int sta_info_flush(struct ieee80211_sub_if_data *sdata)
> +{
> +       int ret = sta_info_flush_defer(sdata);
> +
> +       if (ret)
> +               sta_info_flush_cleanup(sdata);
> +
> +       return ret;
> +}
> +

but i think this is wrong - there might already be stations in the cleanup list.
you should always call sta_info_flush_cleanup.

Eliad.

Re: [PATCH 5/5] mac80211: optimise roaming time again

[Johannes Berg]

On Fri, 2012-12-14 at 01:32 +0200, Eliad Peller wrote:
> On Fri, Dec 14, 2012 at 12:59 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > The last fixes re-added the RCU synchronize penalty
> > on roaming to fix the races. Split up sta_info_flush()
> > now to get rid of that again, and let managed mode
> > (and only it) delay the actual destruction.
> >
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > ---
> The patchset looks good. thanks for fixing it :)

:)

> > +static inline int sta_info_flush(struct ieee80211_sub_if_data *sdata)
> > +{
> > +       int ret = sta_info_flush_defer(sdata);
> > +
> > +       if (ret)
> > +               sta_info_flush_cleanup(sdata);
> > +
> > +       return ret;
> > +}
> > +
> 
> but i think this is wrong - there might already be stations in the cleanup list.
> you should always call sta_info_flush_cleanup.

I don't think so? I thought about this for a while, but I see only the
case where there are stations being added to the list in the
sta_info_flush_defer() call -- then the return value is bigger than zero
and when we return, nothing is on the cleanup list.

The other case is that there's something on the cleanup list for managed
mode, but that only needs to be guaranteed to be cleaned up when the
interface goes down (nothing in the destroy calls back to the driver) so
there the explicit call to sta_info_flush_cleanup() should be enough?

Re: [PATCH 5/5] mac80211: optimise roaming time again

[Eliad Peller]

On Fri, Dec 14, 2012 at 1:41 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2012-12-14 at 01:32 +0200, Eliad Peller wrote:
>> On Fri, Dec 14, 2012 at 12:59 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > From: Johannes Berg <johannes.berg@intel.com>
>> >
>> > The last fixes re-added the RCU synchronize penalty
>> > on roaming to fix the races. Split up sta_info_flush()
>> > now to get rid of that again, and let managed mode
>> > (and only it) delay the actual destruction.
>> >
>> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>> > ---

>> > +static inline int sta_info_flush(struct ieee80211_sub_if_data *sdata)
>> > +{
>> > +       int ret = sta_info_flush_defer(sdata);
>> > +
>> > +       if (ret)
>> > +               sta_info_flush_cleanup(sdata);
>> > +
>> > +       return ret;
>> > +}
>> > +
>>
>> but i think this is wrong - there might already be stations in the cleanup list.
>> you should always call sta_info_flush_cleanup.
>
> I don't think so? I thought about this for a while, but I see only the
> case where there are stations being added to the list in the
> sta_info_flush_defer() call -- then the return value is bigger than zero
> and when we return, nothing is on the cleanup list.
>
> The other case is that there's something on the cleanup list for managed
> mode, but that only needs to be guaranteed to be cleaned up when the
> interface goes down (nothing in the destroy calls back to the driver) so
> there the explicit call to sta_info_flush_cleanup() should be enough?
>
well, i didn't delve into the exact use cases, but semantically, after
calling sta_info_flush(), i'd expect all the vif's stations to get
flushed.
this might be redundant right now (as there are no
sta_info_flush_defer calls followed by sta_info_flush), but it sounds
more correct to me.

Eliad.

Re: [PATCH 5/5] mac80211: optimise roaming time again

[Johannes Berg]

On Fri, 2012-12-14 at 01:57 +0200, Eliad Peller wrote:

> >> > +static inline int sta_info_flush(struct ieee80211_sub_if_data *sdata)
> >> > +{
> >> > +       int ret = sta_info_flush_defer(sdata);
> >> > +
> >> > +       if (ret)
> >> > +               sta_info_flush_cleanup(sdata);
> >> > +
> >> > +       return ret;
> >> > +}
> >> > +
> >>
> >> but i think this is wrong - there might already be stations in the cleanup list.
> >> you should always call sta_info_flush_cleanup.
> >
> > I don't think so? I thought about this for a while, but I see only the
> > case where there are stations being added to the list in the
> > sta_info_flush_defer() call -- then the return value is bigger than zero
> > and when we return, nothing is on the cleanup list.
> >
> > The other case is that there's something on the cleanup list for managed
> > mode, but that only needs to be guaranteed to be cleaned up when the
> > interface goes down (nothing in the destroy calls back to the driver) so
> > there the explicit call to sta_info_flush_cleanup() should be enough?
> >
> well, i didn't delve into the exact use cases, but semantically, after
> calling sta_info_flush(), i'd expect all the vif's stations to get
> flushed.
> this might be redundant right now (as there are no
> sta_info_flush_defer calls followed by sta_info_flush), but it sounds
> more correct to me.

Actually you're right, if a station is deleted explicitly (just before
the AP interface goes down) it still goes on the cleanup list, and if
it's the only one then this patch brings back the bug.

I'll put it back unconditionally.

johannes

[PATCH] mac80211: optimise AP stop RCU handling

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

If there are VLANs, stopping an AP is inefficient as it
calls rcu_barrier() once for each interface (the VLANs
and the AP itself). Optimise this by moving rcu_barrier()
out of the station cleanups and calling it only once for
all interfaces combined.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/cfg.c      |  8 ++++++--
 net/mac80211/iface.c    | 13 ++++++++-----
 net/mac80211/sta_info.c |  2 --
 net/mac80211/sta_info.h | 14 ++++++++++++++
 4 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 8576723..c485b36 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1014,8 +1014,12 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 		kfree_rcu(old_probe_resp, rcu_head);
 
 	list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list)
-		sta_info_flush(vlan);
-	sta_info_flush(sdata);
+		sta_info_flush_defer(vlan);
+	sta_info_flush_defer(sdata);
+	rcu_barrier();
+	list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list)
+		sta_info_flush_cleanup(vlan);
+	sta_info_flush_cleanup(sdata);
 
 	sdata->vif.bss_conf.enable_beacon = false;
 	clear_bit(SDATA_STATE_OFFCHANNEL_BEACON_STOPPED, &sdata->state);
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index dcf943e..e02f7a0 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -872,12 +872,15 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 		cancel_work_sync(&sdata->work);
 		/*
 		 * When we get here, the interface is marked down.
-		 * sta_info_flush_cleanup() calls rcu_barrier to
-		 * wait for the station call_rcu() calls to complete,
-		 * here we require it to wait for the RX path in case
-		 * it is using the interface and enqueuing frames at
-		 * this very time on another CPU.
+		 *
+		 * sta_info_flush_cleanup() requires rcu_barrier()
+		 * first to wait for the station call_rcu() calls
+		 * to complete, here we need at least sychronize_rcu()
+		 * it to wait for the RX path in case it is using the
+		 * interface and enqueuing frames at this very time on
+		 * another CPU.
 		 */
+		rcu_barrier();
 		sta_info_flush_cleanup(sdata);
 
 		skb_queue_purge(&sdata->skb_queue);
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 2f74031..f61bb46 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -920,8 +920,6 @@ int sta_info_flush_defer(struct ieee80211_sub_if_data *sdata)
 
 void sta_info_flush_cleanup(struct ieee80211_sub_if_data *sdata)
 {
-	rcu_barrier();
-
 	ieee80211_cleanup_sdata_stas(sdata);
 	cancel_work_sync(&sdata->cleanup_stations_wk);
 }
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index c3266aed..031e4a5 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -549,6 +549,19 @@ void sta_info_recalc_tim(struct sta_info *sta);
 void sta_info_init(struct ieee80211_local *local);
 void sta_info_stop(struct ieee80211_local *local);
 int sta_info_flush_defer(struct ieee80211_sub_if_data *sdata);
+
+/**
+ * sta_info_flush_cleanup - flush the sta_info cleanup queue
+ * @sdata: the interface
+ *
+ * Flushes the sta_info cleanup queue for a given interface;
+ * this is necessary before the interface is removed or, for
+ * AP/mesh interfaces, before it is deconfigured.
+ *
+ * Note an rcu_barrier() must precede the function, after all
+ * stations have been flushed/removed to ensure the call_rcu()
+ * calls that add stations to the cleanup queue have completed.
+ */
 void sta_info_flush_cleanup(struct ieee80211_sub_if_data *sdata);
 
 /**
@@ -562,6 +575,7 @@ static inline int sta_info_flush(struct ieee80211_sub_if_data *sdata)
 {
 	int ret = sta_info_flush_defer(sdata);
 
+	rcu_barrier();
 	sta_info_flush_cleanup(sdata);
 
 	return ret;
-- 
1.8.0

Re: [PATCH 0/5] mac80211: fix station cleanup/destruction

[Ben Greear]

On 12/13/2012 02:59 PM, Johannes Berg wrote:
> Turns out that not only was flush_workqueue() slow as reported
> by Ben, it was also broken because this is way too late to
> finish the station destruction in AP mode.
>
> Fixing this with keeping the roaming semantics (no RCU barrier
> or synchronize in there) is a bit tricky so the first patch just
> fixes the problem, and the last patch puts back the optimisation.
>
> The patches inbetween are just padding ;-)

I applied this series (as well as the beacon cleanup from Luius)
and so far, the 600-station test case is working fine.

I backported this to 3.7.0, by the way.  It was mostly clean,
but I had to tweak a few things to resolve conflicts.

We will of course let you know if we see anything weird :)

So, you can add:

Tested-by:  Ben Greear <greearb@candelatech.com>


Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 0/5] mac80211: fix station cleanup/destruction

[Johannes Berg]

On Thu, 2012-12-13 at 23:59 +0100, Johannes Berg wrote:
> Turns out that not only was flush_workqueue() slow as reported
> by Ben, it was also broken because this is way too late to
> finish the station destruction in AP mode.
> 
> Fixing this with keeping the roaming semantics (no RCU barrier
> or synchronize in there) is a bit tricky so the first patch just
> fixes the problem, and the last patch puts back the optimisation.

Applied all.

johannes

Re: [PATCH] mac80211: optimise AP stop RCU handling

[Johannes Berg]

On Fri, 2012-12-14 at 14:57 +0100, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> If there are VLANs, stopping an AP is inefficient as it
> calls rcu_barrier() once for each interface (the VLANs
> and the AP itself). Optimise this by moving rcu_barrier()
> out of the station cleanups and calling it only once for
> all interfaces combined.

Applied.

johannes