Skip to contentSkip to conversation

mac80211 tx mq aggregation fixes

RFC In review Johannes Berg 1 participant 5 messages Jul 19, 2008

Subsystems (2)
Networking drivers (wireless) mac80211
lore
Share & tools
lore

Johannes Berg

author maintainer
#0 lore raw

cover mac80211 tx mq aggregation fixes

As we discussed earlier, there is now a race in mac80211 where you can get packets queued onto the aggregation queue while draining it, and some can be left on there. Fixing that is achieved by deferring the draining to a workqueue and calling synchronize_net() before doing it.

Also noticed another problem: when we start aggregation we add the queue before the hardware said it's ok, thus we can end up queuing packets to a new aggregation queue we won't be using, fix that one before since it was easier.

Locking gets a bit tricky unfortunately.

johannes

author maintainer
#1 lore raw

RFC 1/3 mac80211: sane arguments to ieee80211_ht_agg_queue_remove

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/main.c |    8 +++++---
 net/mac80211/wme.c  |    7 +------
 net/mac80211/wme.h  |    3 +--
 3 files changed, 7 insertions(+), 11 deletions(-)
net/mac80211/main.c +5 -3
@@ -644,7 +644,8 @@ int ieee80211_start_tx_ba_session(struct
 		/* No need to requeue the packets in the agg queue, since we
 		 * held the tx lock: no packet could be enqueued to the newly
 		 * allocated queue */
-		ieee80211_ht_agg_queue_remove(local, sta, tid, 0);
+		ieee80211_ht_agg_queue_remove(local, sta->tid_to_tx_q[tid], false);
+		sta->tid_to_tx_q[tid] = ieee80211_num_queues(hw);
 #ifdef CONFIG_MAC80211_HT_DEBUG
 		printk(KERN_DEBUG "BA request denied - HW unavailable for"
 					" tid %d\n", tid);

@@ -740,7 +741,7 @@ int ieee80211_stop_tx_ba_session(struct 
 		goto stop_BA_exit;
 	}
 
-stop_BA_exit:
+ stop_BA_exit:
 	spin_unlock_bh(&sta->lock);
 	rcu_read_unlock();
 	return ret;

@@ -852,7 +853,8 @@ void ieee80211_stop_tx_ba_cb(struct ieee
 
 	agg_queue = sta->tid_to_tx_q[tid];
 
-	ieee80211_ht_agg_queue_remove(local, sta, tid, 1);
+	ieee80211_ht_agg_queue_remove(local, agg_queue, true);
+	sta->tid_to_tx_q[tid] = ieee80211_num_queues(hw);
 
 	/* We just requeued the all the frames that were in the
 	 * removed queue, and since we might miss a softirq we do
net/mac80211/wme.c +1 -6
@@ -223,15 +223,10 @@ int ieee80211_ht_agg_queue_add(struct ie
  * the caller needs to hold netdev_get_tx_queue(local->mdev, X)->lock
  */
 void ieee80211_ht_agg_queue_remove(struct ieee80211_local *local,
-				   struct sta_info *sta, u16 tid,
-				   u8 requeue)
+				   u16 agg_queue, bool requeue)
 {
-	int agg_queue = sta->tid_to_tx_q[tid];
-	struct ieee80211_hw *hw = &local->hw;
-
 	/* return the qdisc to the pool */
 	clear_bit(agg_queue, local->queue_pool);
-	sta->tid_to_tx_q[tid] = ieee80211_num_queues(hw);
 
 	if (requeue) {
 		ieee80211_requeue(local, agg_queue);
net/mac80211/wme.h +1 -2
@@ -27,8 +27,7 @@ u16 ieee80211_select_queue(struct net_de
 int ieee80211_ht_agg_queue_add(struct ieee80211_local *local,
 			       struct sta_info *sta, u16 tid);
 void ieee80211_ht_agg_queue_remove(struct ieee80211_local *local,
-				   struct sta_info *sta, u16 tid,
-				   u8 requeue);
+				   u16 agg_queue, bool requeue);
 void ieee80211_requeue(struct ieee80211_local *local, int queue);
 
 #endif /* _WME_H */
author maintainer
#2 lore raw

RFC 2/3 mac80211: fix race during adding HT queues

In reply to Johannes Berg
Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/ieee80211_i.h |    6 ++++++
 net/mac80211/main.c        |   21 ++++++++++++---------
 net/mac80211/wme.c         |   13 ++++++++-----
 3 files changed, 26 insertions(+), 14 deletions(-)
net/mac80211/main.c +12 -9
@@ -558,7 +558,7 @@ int ieee80211_start_tx_ba_session(struct
 	struct ieee80211_sub_if_data *sdata;
 	u16 start_seq_num = 0;
 	u8 *state;
-	int ret;
+	int ret = 0, reserved_queue;
 	DECLARE_MAC_BUF(mac);
 
 	if (tid >= STA_TID_NUM)

@@ -619,16 +619,16 @@ int ieee80211_start_tx_ba_session(struct
 	init_timer(&sta->ampdu_mlme.tid_tx[tid]->addba_resp_timer);
 
 	/* create a new queue for this aggregation */
-	ret = ieee80211_ht_agg_queue_add(local, sta, tid);
+	reserved_queue = ieee80211_ht_agg_queue_add(local, sta, tid);
 
 	/* case no queue is available to aggregation
 	 * don't switch to aggregation */
-	if (ret) {
+	if (reserved_queue < 0) {
 #ifdef CONFIG_MAC80211_HT_DEBUG
 		printk(KERN_DEBUG "BA request denied - queue unavailable for"
 					" tid %d\n", tid);
 #endif /* CONFIG_MAC80211_HT_DEBUG */
-		goto err_unlock_queue;
+		goto err_free_state;
 	}
 	sdata = sta->sdata;
 

@@ -642,8 +642,8 @@ int ieee80211_start_tx_ba_session(struct
 
 	if (ret) {
 		/* No need to requeue the packets in the agg queue, since we
-		 * held the tx lock: no packet could be enqueued to the newly
-		 * allocated queue */
+		 * haven't yet marked the queue in the queue_pool no packet
+		 * can have been enqueued to the newly allocated queue */
 		ieee80211_ht_agg_queue_remove(local, sta->tid_to_tx_q[tid], false);
 		sta->tid_to_tx_q[tid] = ieee80211_num_queues(hw);
 #ifdef CONFIG_MAC80211_HT_DEBUG

@@ -651,10 +651,13 @@ int ieee80211_start_tx_ba_session(struct
 					" tid %d\n", tid);
 #endif /* CONFIG_MAC80211_HT_DEBUG */
 		*state = HT_AGG_STATE_IDLE;
-		goto err_unlock_queue;
+		goto err_free_state;
 	}
 
-	/* Will put all the packets in the new SW queue */
+	/* mark the queue as used now */
+	set_bit(reserved_queue, local->queue_pool);
+
+	/* Put all the packets onto the new aggregation queue */
 	ieee80211_requeue(local, ieee802_1d_to_ac[tid]);
 	spin_unlock_bh(&sta->lock);
 

@@ -678,7 +681,7 @@ int ieee80211_start_tx_ba_session(struct
 #endif
 	goto exit;
 
-err_unlock_queue:
+err_free_state:
 	kfree(sta->ampdu_mlme.tid_tx[tid]);
 	sta->ampdu_mlme.tid_tx[tid] = NULL;
 	ret = -EBUSY;
net/mac80211/wme.c +8 -5
@@ -183,6 +183,10 @@ u16 ieee80211_select_queue(struct net_de
 	return queue;
 }
 
+/*
+ * Must be called under local->sta_lock, will return a negative
+ * error code or the number of the reserved queue.
+ */
 int ieee80211_ht_agg_queue_add(struct ieee80211_local *local,
 			       struct sta_info *sta, u16 tid)
 {

@@ -196,7 +200,7 @@ int ieee80211_ht_agg_queue_add(struct ie
 
 	/* try to get a Qdisc from the pool */
 	for (i = local->hw.queues; i < ieee80211_num_queues(&local->hw); i++)
-		if (!test_and_set_bit(i, local->queue_pool)) {
+		if (!test_bit(i, local->queue_pool)) {
 			ieee80211_stop_queue(local_to_hw(local), i);
 			sta->tid_to_tx_q[tid] = i;
 

@@ -208,12 +212,11 @@ int ieee80211_ht_agg_queue_add(struct ie
 			if (net_ratelimit()) {
 				DECLARE_MAC_BUF(mac);
 				printk(KERN_DEBUG "allocated aggregation queue"
-					" %d tid %d addr %s pool=0x%lX\n",
-					i, tid, print_mac(mac, sta->addr),
-					local->queue_pool[0]);
+					" %d tid %d addr %s\n",
+					i, tid, print_mac(mac, sta->addr));
 			}
 #endif /* CONFIG_MAC80211_HT_DEBUG */
-			return 0;
+			return i;
 		}
 
 	return -EAGAIN;
net/mac80211/ieee80211_i.h +6 -0
@@ -549,6 +549,12 @@ struct ieee80211_local {
 
 	const struct ieee80211_ops *ops;
 
+	/*
+	 * Pool of reserved ampdu queues. Note that setting a bit
+	 * here to reserve the queue must only be done under sta_lock
+	 * to protect the setting process, we can unfortunately not
+	 * do it with atomic operations at this time.
+	 */
 	unsigned long queue_pool[BITS_TO_LONGS(QD_MAX_QUEUES)];
 
 	struct net_device *mdev; /* wmaster# - "master" 802.11 device */
author maintainer
#3 lore raw

RFC 3/3 mac80211: fix requeue race

In reply to Johannes Berg
Signed-off-by Johannes Berg <[email protected]>
---
 drivers/net/wireless/iwlwifi/iwl-tx.c |    4 
 include/net/mac80211.h                |   16 ---
 net/mac80211/ieee80211_i.h            |    7 +
 net/mac80211/main.c                   |  146 +++++++++++++++++++++-------------
 net/mac80211/wme.c                    |   76 ++++++++++++++++-
 net/mac80211/wme.h                    |    4 
 6 files changed, 175 insertions(+), 78 deletions(-)
net/mac80211/ieee80211_i.h +5 -2
@@ -534,8 +534,7 @@ struct ieee80211_sub_if_data *vif_to_sda
 enum {
 	IEEE80211_RX_MSG	= 1,
 	IEEE80211_TX_STATUS_MSG	= 2,
-	IEEE80211_DELBA_MSG	= 3,
-	IEEE80211_ADDBA_MSG	= 4,
+	IEEE80211_ADDBA_MSG	= 3,
 };
 
 /* maximum number of hardware queues we support. */

@@ -557,6 +556,10 @@ struct ieee80211_local {
 	 */
 	unsigned long queue_pool[BITS_TO_LONGS(QD_MAX_QUEUES)];
 
+	struct work_struct tx_ba_stop_work;
+	spinlock_t tx_ba_stop_lock;
+	struct list_head tx_ba_stop_list;
+
 	struct net_device *mdev; /* wmaster# - "master" 802.11 device */
 	int open_count;
 	int monitors, cooked_mntrs;
net/mac80211/main.c +93 -53

207 lines not on this page · raw message

include/net/mac80211.h +2 -14
@@ -1734,21 +1734,9 @@ int ieee80211_stop_tx_ba_session(struct 
  *
  * This function must be called by low level driver once it has
  * finished with preparations for the BA session tear down.
+ * This function can be called in any context.
  */
-void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid);
-
-/**
- * ieee80211_stop_tx_ba_cb_irqsafe - low level driver ready to stop aggregate.
- * @hw: pointer as obtained from ieee80211_alloc_hw().
- * @ra: receiver address of the BA session recipient.
- * @tid: the desired TID to BA on.
- *
- * This function must be called by low level driver once it has
- * finished with preparations for the BA session tear down.
- * This version of the function is IRQ-safe.
- */
-void ieee80211_stop_tx_ba_cb_irqsafe(struct ieee80211_hw *hw, const u8 *ra,
-				     u16 tid);
+void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, const u8 *ra, u8 tid);
 
 /**
  * ieee80211_notify_mac - low level driver notification
drivers/net/wireless/iwlwifi/iwl-tx.c +2 -2
@@ -1304,7 +1304,7 @@ int iwl_tx_agg_stop(struct iwl_priv *pri
 	if (ret)
 		return ret;
 
-	ieee80211_stop_tx_ba_cb_irqsafe(priv->hw, ra, tid);
+	ieee80211_stop_tx_ba_cb(priv->hw, ra, tid);
 
 	return 0;
 }

@@ -1328,7 +1328,7 @@ int iwl_txq_check_empty(struct iwl_priv 
 			priv->cfg->ops->lib->txq_agg_disable(priv, txq_id,
 							     ssn, tx_fifo);
 			tid_data->agg.state = IWL_AGG_OFF;
-			ieee80211_stop_tx_ba_cb_irqsafe(priv->hw, addr, tid);
+			ieee80211_stop_tx_ba_cb(priv->hw, addr, tid);
 		}
 		break;
 	case IWL_EMPTYING_HW_QUEUE_ADDBA:
net/mac80211/wme.c +71 -5

101 lines not on this page · raw message

net/mac80211/wme.h +2 -2
@@ -27,7 +27,7 @@ u16 ieee80211_select_queue(struct net_de
 int ieee80211_ht_agg_queue_add(struct ieee80211_local *local,
 			       struct sta_info *sta, u16 tid);
 void ieee80211_ht_agg_queue_remove(struct ieee80211_local *local,
-				   u16 agg_queue, bool requeue);
-void ieee80211_requeue(struct ieee80211_local *local, int queue);
+				   u16 agg_queue, int regular_queue);
+void ieee80211_requeue_toagg(struct ieee80211_local *local, int from);
 
 #endif /* _WME_H */
author maintainer
#4 lore raw

Re: [RFC 3/3] mac80211: fix requeue race

net/mac80211/main.c void ieee80211_stop_tx_ba_cb(struct ieee · line 920

Quoted 8 lines
net/mac80211/main.c +7 -0
+	/*
+	 * We just requeued the all the frames that were in the
+	 * removed queue, and since we might miss a softirq we do
+	 * netif_schedule_queue.  ieee80211_wake_queue is not used
+	 * here as this queue is not necessarily stopped
+	 */
+	netif_schedule_queue(netdev_get_tx_queue(local->mdev, agg_queue));

Come to think of it, we should probably do

netif_schedule_queue(netdev_get_tx_queue(local->mdev, ieee802_1d_to_ac[tid]));

since we just drained that agg_queue in favour of the tid one.

instead