mac80211: AMPDU rx reorder timeout timer

WIP In review Christian Lamparter 2 participants 8 messages Jul 27, 2010 – Aug 2, 2010 mac80211

Share & tools

Christian Lamparter

author

2010-07-27 06:20 UTC #0 lore raw

This (rather _unfinished_ :-D ) patch adds a timer which if trigger schedules the automatic release of all expired A-MPDU frames. This implementation has the advantage that all queued-up MPDU will now arrive in the network core within a timely manner.

In my "experiments", the patch helped to ironed out the sudden throughput drops that have been _killing_ my average tcp performance ever since I can remember.

But there is a catch: The logs will quickly fill up with:
"release an RX reorder frame due to timeout on earlier frames". and the package loss will goes through the roof...

Now, I can think of several reasons why this could happen:
1. we don't wait long enough for the HT peer to finish their _retry_ procedure?

2. previously - when the stream simply _stopped_ - we had to wait until the tcp retry kick in again. So we had some "silent" periods and therefore less noise from the reordering code?

3. ->bugs<- but where are they?

Regards,
Christian
---
yeah, it's past 8 AM here...

so please excuse me, if this is nothing but utter rubbish.
---

net/mac80211/agg-rx.c +34 -1

index 965b272..947352e 100644
@@ -86,6 +86,11 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
 				     tid, 0, reason);
 
 	del_timer_sync(&tid_rx->session_timer);
+	del_timer_sync(&tid_rx->reorder_timer);
+
+	spin_lock_bh(&local->tid_rx_reorder_lock);
+	list_del_init(&tid_rx->reorder_head);
+	spin_unlock_bh(&local->tid_rx_reorder_lock);
 
 	call_rcu(&tid_rx->rcu_head, ieee80211_free_tid_rx);
 }

@@ -120,6 +125,28 @@ static void sta_rx_agg_session_timer_expired(unsigned long data)
 	ieee80211_queue_work(&sta->local->hw, &sta->ampdu_mlme.work);
 }
 
+static void sta_rx_agg_reorder_timer_expired(unsigned long data)
+{
+	/* not an elegant detour, but there is no choice as the timer passes
+	 * only one argument, and various sta_info are needed here, so init
+	 * flow in sta_info_create gives the TID as data, while the timer_to_id
+	 * array gives the sta through container_of */
+	u8 *ptid = (u8 *)data;
+	u8 *timer_to_id = ptid - *ptid;
+	struct sta_info *sta = container_of(timer_to_id, struct sta_info,
+					 timer_to_tid[0]);
+
+	struct ieee80211_local *local = sta->local;
+	struct tid_ampdu_rx *tid_rx = sta->ampdu_mlme.tid_rx[*ptid];
+
+	spin_lock_bh(&local->tid_rx_reorder_lock);
+	if (list_empty(&tid_rx->reorder_head))
+		list_add_tail(&tid_rx->reorder_head, &local->tid_rx_reorder);
+	spin_unlock_bh(&local->tid_rx_reorder_lock);
+
+	tasklet_schedule(&local->tasklet);
+}
+
 static void ieee80211_send_addba_resp(struct ieee80211_sub_if_data *sdata, u8 *da, u16 tid,
 				      u8 dialog_token, u16 status, u16 policy,
 				      u16 buf_size, u16 timeout)

@@ -251,11 +278,17 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
 		goto end;
 	}
 
-	/* rx timer */
+	/* rx session timer */
 	tid_agg_rx->session_timer.function = sta_rx_agg_session_timer_expired;
 	tid_agg_rx->session_timer.data = (unsigned long)&sta->timer_to_tid[tid];
 	init_timer(&tid_agg_rx->session_timer);
 
+	/* rx reorder timer */
+	INIT_LIST_HEAD(&tid_agg_rx->reorder_head);
+	tid_agg_rx->reorder_timer.function = sta_rx_agg_reorder_timer_expired;
+	tid_agg_rx->reorder_timer.data = (unsigned long)&sta->timer_to_tid[tid];
+	init_timer(&tid_agg_rx->reorder_timer);
+
 	/* prepare reordering buffer */
 	tid_agg_rx->reorder_buf =
 		kcalloc(buf_size, sizeof(struct sk_buff *), GFP_ATOMIC);

net/mac80211/ieee80211_i.h +4 -0

index c6b5c2d..b8bd0f9 100644
@@ -704,6 +704,8 @@ struct ieee80211_local {
 	struct tasklet_struct tasklet;
 	struct sk_buff_head skb_queue;
 	struct sk_buff_head skb_queue_unreliable;
+	spinlock_t tid_rx_reorder_lock;
+	struct list_head tid_rx_reorder;
 
 	/* Station data */
 	/*

@@ -1241,6 +1243,8 @@ int ieee80211_wk_remain_on_channel(struct ieee80211_sub_if_data *sdata,
 int ieee80211_wk_cancel_remain_on_channel(
 	struct ieee80211_sub_if_data *sdata, u64 cookie);
 
+void ieee80211_release_reorder_timeout(struct sta_info *sta, unsigned int tid);
+
 /* channel management */
 enum ieee80211_chan_mode {
 	CHAN_MODE_UNDEFINED,

net/mac80211/main.c +22 -0

index 0e95c75..7bff2db 100644
@@ -281,6 +281,26 @@ static void ieee80211_tasklet_handler(unsigned long data)
 			break;
 		}
 	}
+
+	spin_lock_bh(&local->tid_rx_reorder_lock);
+	while (!list_empty(&local->tid_rx_reorder)) {
+		struct tid_ampdu_rx *iter;
+		struct sta_info *sta;
+		u8 *ptid, *timer_to_id;
+
+		iter = list_first_entry(&local->tid_rx_reorder,
+					struct tid_ampdu_rx,
+					reorder_head);
+
+		ptid = (u8 *)iter->session_timer.data;
+		timer_to_id = ptid - *ptid;
+		sta = container_of(timer_to_id, struct sta_info,
+				   timer_to_tid[0]);
+
+		list_del_init(&iter->reorder_head);
+		ieee80211_release_reorder_timeout(sta, *ptid);
+	}
+	spin_unlock_bh(&local->tid_rx_reorder_lock);
 }
 
 static void ieee80211_restart_work(struct work_struct *work)

@@ -448,6 +468,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
 	local->uapsd_max_sp_len = IEEE80211_DEFAULT_MAX_SP_LEN;
 
 	INIT_LIST_HEAD(&local->interfaces);
+	INIT_LIST_HEAD(&local->tid_rx_reorder);
+	spin_lock_init(&local->tid_rx_reorder_lock);
 
 	__hw_addr_init(&local->mc_list);

net/mac80211/rx.c +175 ~~-93~~

328 lines not on this page · raw message

net/mac80211/sta_info.h +3 -0

index 54262e7..4dccce8 100644
@@ -110,6 +110,7 @@ struct tid_ampdu_tx {
  * @timeout: reset timer value (in TUs).
  * @dialog_token: dialog token for aggregation session
  * @rcu_head: RCU head used for freeing this struct
+ * @reorder_head: used to release expired frames
  *
  * This structure is protected by RCU and the per-station
  * spinlock. Assignments to the array holding it must hold

@@ -121,9 +122,11 @@ struct tid_ampdu_tx {
  */
 struct tid_ampdu_rx {
 	struct rcu_head rcu_head;
+	struct list_head reorder_head;
 	struct sk_buff **reorder_buf;
 	unsigned long *reorder_time;
 	struct timer_list session_timer;
+	struct timer_list reorder_timer;
 	u16 head_seq_num;
 	u16 stored_mpdu_num;
 	u16 ssn;

Johannes Berg

maintainer

2010-07-27 08:37 UTC #1 lore raw

Quoted 9 lines · Christian Lamparter

This (rather _unfinished_ :-D ) patch adds a timer which if trigger schedules the automatic release of all expired A-MPDU frames. This implementation has the advantage that all queued-up MPDU will now arrive in the network core within a timely manner.
In my "experiments", the patch helped to ironed out the sudden throughput drops that have been _killing_ my average tcp performance ever since I can remember.

But why is your BA session so damaged to start with? This is not a normal situtation!

Quoted 8 lines

But there is a catch: The logs will quickly fill up with:
"release an RX reorder frame due to timeout on earlier frames". and the package loss will goes through the roof...
Now, I can think of several reasons why this could happen:
1. we don't wait long enough for the HT peer to finish their _retry_ procedure?

Quoted 6 lines

2. previously - when the stream simply _stopped_ - we had to wait until the tcp retry kick in again. So we had some "silent" periods and therefore less noise from the reordering code?
3. ->bugs<- but where are they?

I'm having a hard time understanding this patch, can you split out the no-op code reorg parts or explain what it does?

johannes

Christian Lamparter

author

2010-07-28 15:16 UTC #2 lore raw

Quoted 7 lines · On Tuesday 27 July 2010 10:37:25 Johannes Berg wrote:

Quoted 6 lines · Christian Lamparter
This (rather _unfinished_ :-D ) patch adds a timer which if trigger schedules the automatic release of all expired A-MPDU frames. This implementation has the advantage that all queued-up MPDU will now arrive in the network core within a timely manner.

Quoted 6 lines

Quoted 3 lines
In my "experiments", the patch helped to ironed out the sudden throughput drops that have been _killing_ my average tcp performance ever since I can remember.
But why is your BA session so damaged to start with? This is not a normal situtation!

I can think of at least one possible reason:

commit 3ef83d745bf5220bef3a0fd11b96eb9ac64c8e8e

Author: Felix Fietkau <[email protected]>
Date:   Tue May 4 09:58:57 2010 +0200

ath9k: fix another source of corrupt frames

Atheros hardware supports receiving frames that span multiple descriptors and buffers. In this case, the rx status of every descriptor except for the last one is invalid and may contain random data. Because the driver does not support this, it needs to drop such frames.

Signed-off-by Felix Fietkau <[email protected]>

Signed-off-by John W. Linville <[email protected]>

and obviously, the HW marked the "last one" in the BA scoreboard as successfully received. Therefore we don't get a second attempt because the driver dropped it :(

(The same applies to ar9170, which will print "missing tags!",
or stream corruption in such a case)

Quoted 13 lines

Quoted 13 lines
But there is a catch: The logs will quickly fill up with:
"release an RX reorder frame due to timeout on earlier frames". and the package loss will goes through the roof...
Now, I can think of several reasons why this could happen:
1. we don't wait long enough for the HT peer to finish their _retry_ procedure?
2. previously - when the stream simply _stopped_ - we had to wait until the tcp retry kick in again. So we had some "silent" periods and therefore less noise from the reordering code?

Quoted 4 lines

Quoted 1 line
3. ->bugs<- but where are they?
I'm having a hard time understanding this patch, can you split out the no-op code reorg parts or explain what it does?

Sure, just finished clean-up.

Regards,
Chr

Christian Lamparter

author

2010-07-28 15:18 UTC #3 lore raw

PATCH 1/3 mac80211: put rx handlers into seperate functions

In reply to Johannes Berg

Note: In order to keep diff from removing and re-adding code, I used forward declarations. So it should be easier to verify that I didn't change anything related to the reordering logic.
---

net/mac80211/rx.c +82 ~~-29~~

index fa0f37e..8179e62 100644
@@ -574,6 +574,11 @@ static void ieee80211_release_reorder_frames(struct ieee80211_hw *hw,
 	}
 }
 
+static void ieee80211_sta_reorder_release(struct ieee80211_hw *hw,
+					  struct tid_ampdu_rx *tid_agg_rx,
+					  struct sk_buff_head *frames);
+
+
 /*
  * Timeout (in jiffies) for skb's that are waiting in the RX reorder buffer. If
  * the skb was added to the buffer longer than this time ago, the earlier

@@ -643,6 +648,17 @@ static bool ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
 	tid_agg_rx->reorder_buf[index] = skb;
 	tid_agg_rx->reorder_time[index] = jiffies;
 	tid_agg_rx->stored_mpdu_num++;
+	ieee80211_sta_reorder_release(hw, tid_agg_rx, frames);
+
+	return true;
+}
+
+static void ieee80211_sta_reorder_release(struct ieee80211_hw *hw,
+					  struct tid_ampdu_rx *tid_agg_rx,
+					  struct sk_buff_head *frames)
+{
+	int index;
+
 	/* release the buffer until next missing frame */
 	index = seq_sub(tid_agg_rx->head_seq_num, tid_agg_rx->ssn) %
 						tid_agg_rx->buf_size;

@@ -686,8 +702,6 @@ static bool ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
 		index =	seq_sub(tid_agg_rx->head_seq_num, tid_agg_rx->ssn) %
 							tid_agg_rx->buf_size;
 	}
-
-	return true;
 }
 
 /*

@@ -2267,6 +2281,11 @@ static void ieee80211_rx_cooked_monitor(struct ieee80211_rx_data *rx,
 	dev_kfree_skb(skb);
 }
 
+static void ieee80211_rx_handlers_result(struct ieee80211_rx_data *rx,
+					 ieee80211_rx_result res);
+
+static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx,
+				  struct sk_buff_head *frames);
 
 static void ieee80211_invoke_rx_handlers(struct ieee80211_sub_if_data *sdata,
 					 struct ieee80211_rx_data *rx,

@@ -2288,17 +2307,34 @@ static void ieee80211_invoke_rx_handlers(struct ieee80211_sub_if_data *sdata,
 			goto rxh_next;  \
 	} while (0);
 
-	/*
-	 * NB: the rxh_next label works even if we jump
-	 *     to it from here because then the list will
-	 *     be empty, which is a trivial check
-	 */
 	CALL_RXH(ieee80211_rx_h_passive_scan)
 	CALL_RXH(ieee80211_rx_h_check)
 
 	ieee80211_rx_reorder_ampdu(rx, &reorder_release);
 
-	while ((skb = __skb_dequeue(&reorder_release))) {
+	ieee80211_rx_handlers(rx, &reorder_release);
+	return;
+
+ rxh_next:
+	ieee80211_rx_handlers_result(rx, res);
+
+#undef CALL_RXH
+}
+
+static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx,
+				  struct sk_buff_head *frames)
+{
+	ieee80211_rx_result res = RX_DROP_MONITOR;
+	struct sk_buff *skb;
+
+#define CALL_RXH(rxh)			\
+	do {				\
+		res = rxh(rx);		\
+		if (res != RX_CONTINUE)	\
+			goto rxh_next;  \
+	} while (0);
+
+	while ((skb = __skb_dequeue(frames))) {
 		/*
 		 * all the other fields are valid across frames
 		 * that belong to an aMPDU since they are on the

@@ -2316,41 +2352,58 @@ static void ieee80211_invoke_rx_handlers(struct ieee80211_sub_if_data *sdata,
 		CALL_RXH(ieee80211_rx_h_remove_qos_control)
 		CALL_RXH(ieee80211_rx_h_amsdu)
 #ifdef CONFIG_MAC80211_MESH
-		if (ieee80211_vif_is_mesh(&sdata->vif))
+		if (ieee80211_vif_is_mesh(&rx->sdata->vif))
 			CALL_RXH(ieee80211_rx_h_mesh_fwding);
 #endif
 		CALL_RXH(ieee80211_rx_h_data)
 
 		/* special treatment -- needs the queue */
-		res = ieee80211_rx_h_ctrl(rx, &reorder_release);
+		res = ieee80211_rx_h_ctrl(rx, frames);
 		if (res != RX_CONTINUE)
 			goto rxh_next;
 
 		CALL_RXH(ieee80211_rx_h_action)
 		CALL_RXH(ieee80211_rx_h_mgmt)
 
+ rxh_next:
+		ieee80211_rx_handlers_result(rx, res);
+
 #undef CALL_RXH
+	}
+}
 
- rxh_next:
-		switch (res) {
-		case RX_DROP_MONITOR:
-			I802_DEBUG_INC(sdata->local->rx_handlers_drop);
-			if (rx->sta)
-				rx->sta->rx_dropped++;
-			/* fall through */
-		case RX_CONTINUE:
-			ieee80211_rx_cooked_monitor(rx, rate);
-			break;
-		case RX_DROP_UNUSABLE:
-			I802_DEBUG_INC(sdata->local->rx_handlers_drop);
-			if (rx->sta)
-				rx->sta->rx_dropped++;
-			dev_kfree_skb(rx->skb);
-			break;
-		case RX_QUEUED:
-			I802_DEBUG_INC(sdata->local->rx_handlers_queued);
-			break;
+static void ieee80211_rx_handlers_result(struct ieee80211_rx_data *rx,
+					 ieee80211_rx_result res)
+{
+	switch (res) {
+	case RX_DROP_MONITOR:
+		I802_DEBUG_INC(rx->sdata->local->rx_handlers_drop);
+		if (rx->sta)
+			rx->sta->rx_dropped++;
+		/* fall through */
+	case RX_CONTINUE: {
+		struct ieee80211_rate *rate = NULL;
+		struct ieee80211_supported_band *sband;
+		struct ieee80211_rx_status *status;
+
+		status = IEEE80211_SKB_RXCB((rx->skb));
+
+		sband = rx->local->hw.wiphy->bands[status->band];
+		if (!(status->flag & RX_FLAG_HT))
+			rate = &sband->bitrates[status->rate_idx];
+
+		ieee80211_rx_cooked_monitor(rx, rate);
+		break;
 		}
+	case RX_DROP_UNUSABLE:
+		I802_DEBUG_INC(rx->sdata->local->rx_handlers_drop);
+		if (rx->sta)
+			rx->sta->rx_dropped++;
+		dev_kfree_skb(rx->skb);
+		break;
+	case RX_QUEUED:
+		I802_DEBUG_INC(rx->sdata->local->rx_handlers_queued);
+		break;
 	}
 }

Christian Lamparter

author

2010-07-28 15:21 UTC #4 lore raw

PATCH 2/3 mac80211: remove (now) unused rate function parameters

In reply to Johannes Berg

dead code removal. This can probably go in in one form or another.

---
additionally: we could drop the extra sdata parameter of

prepare_for_handlers
ieee80211_invoke_rx_handlers

and use the one from the rx_data structure?
---

net/mac80211/rx.c +6 ~~-16~~

index 8179e62..6cb5541 100644
@@ -538,20 +538,12 @@ static void ieee80211_release_reorder_frame(struct ieee80211_hw *hw,
 					    int index,
 					    struct sk_buff_head *frames)
 {
-	struct ieee80211_supported_band *sband;
-	struct ieee80211_rate *rate = NULL;
 	struct sk_buff *skb = tid_agg_rx->reorder_buf[index];
-	struct ieee80211_rx_status *status;
 
 	if (!skb)
 		goto no_frame;
 
-	status = IEEE80211_SKB_RXCB(skb);
-
-	/* release the reordered frames to stack */
-	sband = hw->wiphy->bands[status->band];
-	if (!(status->flag & RX_FLAG_HT))
-		rate = &sband->bitrates[status->rate_idx];
+	/* release the frame from the reorder ring buffer */
 	tid_agg_rx->stored_mpdu_num--;
 	tid_agg_rx->reorder_buf[index] = NULL;
 	__skb_queue_tail(frames, skb);

@@ -2289,8 +2281,7 @@ static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx,
 
 static void ieee80211_invoke_rx_handlers(struct ieee80211_sub_if_data *sdata,
 					 struct ieee80211_rx_data *rx,
-					 struct sk_buff *skb,
-					 struct ieee80211_rate *rate)
+					 struct sk_buff *skb)
 {
 	struct sk_buff_head reorder_release;
 	ieee80211_rx_result res = RX_DROP_MONITOR;

@@ -2500,8 +2491,7 @@ static int prepare_for_handlers(struct ieee80211_sub_if_data *sdata,
  * be called with rcu_read_lock protection.
  */
 static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
-					 struct sk_buff *skb,
-					 struct ieee80211_rate *rate)
+					 struct sk_buff *skb)
 {
 	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
 	struct ieee80211_local *local = hw_to_local(hw);

@@ -2609,7 +2599,7 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
 					       prev->name);
 				goto next;
 			}
-			ieee80211_invoke_rx_handlers(prev, &rx, skb_new, rate);
+			ieee80211_invoke_rx_handlers(prev, &rx, skb_new);
 next:
 			prev = sdata;
 		}

@@ -2625,7 +2615,7 @@ next:
 		}
 	}
 	if (prev)
-		ieee80211_invoke_rx_handlers(prev, &rx, skb, rate);
+		ieee80211_invoke_rx_handlers(prev, &rx, skb);
 	else
 		dev_kfree_skb(skb);
 }

@@ -2711,7 +2701,7 @@ void ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb)
 		return;
 	}
 
-	__ieee80211_rx_handle_packet(hw, skb, rate);
+	__ieee80211_rx_handle_packet(hw, skb);
 
 	rcu_read_unlock();

Christian Lamparter

author

2010-07-28 15:21 UTC #5 lore raw

PATCH 3/3 mac80211: AMPDU rx reorder timeout timer

In reply to Johannes Berg

Everytime a hole is discovered in the reorder ring-buffer, the release timer (for this tid_ampdu_rx) is set to fire after HT_RX_REORDER_BUF_TIMEOUT jiffies.

If the missing frame(s) is/are arriving in time, the timer gets deactivated/postponed(another hole has opened up).

Or, if the frame is really "lost", then the timer puts the reorder tid_ampdu_rx into the local device's "tid_rx_reorder" list and schedules the tasklet, which will look-up the tid_ampdu_rx and release all expired frames.

---
Hmm, I'm not sure if the tasklet code is 100% sane. On one hand: we set rx.flags and sdata/sta pointers without the usual rcu-protection.

but on the other hand,it should be OK because the sta/sdata don't disappear without initiating a teardown. (or simply poison reorder_head by using list_del)
---

net/mac80211/agg-rx.c +29 -0

index 965b272..393f4e5 100644
@@ -86,6 +86,11 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
 				     tid, 0, reason);
 
 	del_timer_sync(&tid_rx->session_timer);
+	del_timer_sync(&tid_rx->reorder_timer);
+
+	spin_lock_bh(&local->tid_rx_reorder_lock);
+	list_del_init(&tid_rx->reorder_list);
+	spin_unlock_bh(&local->tid_rx_reorder_lock);
 
 	call_rcu(&tid_rx->rcu_head, ieee80211_free_tid_rx);
 }

@@ -120,6 +125,24 @@ static void sta_rx_agg_session_timer_expired(unsigned long data)
 	ieee80211_queue_work(&sta->local->hw, &sta->ampdu_mlme.work);
 }
 
+static void sta_rx_agg_reorder_timer_expired(unsigned long data)
+{
+	u8 *ptid = (u8 *)data;
+	u8 *timer_to_id = ptid - *ptid;
+	struct sta_info *sta = container_of(timer_to_id, struct sta_info,
+			timer_to_tid[0]);
+
+	struct ieee80211_local *local = sta->local;
+	struct tid_ampdu_rx *tid_rx = sta->ampdu_mlme.tid_rx[*ptid];
+
+	spin_lock(&local->tid_rx_reorder_lock);
+	if (list_empty(&tid_rx->reorder_list))
+		list_add_tail(&tid_rx->reorder_list, &local->tid_rx_reorder);
+	spin_unlock(&local->tid_rx_reorder_lock);
+
+	tasklet_schedule(&local->tasklet);
+}
+
 static void ieee80211_send_addba_resp(struct ieee80211_sub_if_data *sdata, u8 *da, u16 tid,
 				      u8 dialog_token, u16 status, u16 policy,
 				      u16 buf_size, u16 timeout)

@@ -256,6 +279,12 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
 	tid_agg_rx->session_timer.data = (unsigned long)&sta->timer_to_tid[tid];
 	init_timer(&tid_agg_rx->session_timer);
 
+	/* rx reorder timer */
+	INIT_LIST_HEAD(&tid_agg_rx->reorder_list);
+	tid_agg_rx->reorder_timer.function = sta_rx_agg_reorder_timer_expired;
+	tid_agg_rx->reorder_timer.data = (unsigned long)&sta->timer_to_tid[tid];
+	init_timer(&tid_agg_rx->reorder_timer);
+
 	/* prepare reordering buffer */
 	tid_agg_rx->reorder_buf =
 		kcalloc(buf_size, sizeof(struct sk_buff *), GFP_ATOMIC);

net/mac80211/ieee80211_i.h +3 -0

index ef47006..fd658ef 100644
@@ -704,6 +704,8 @@ struct ieee80211_local {
 	struct tasklet_struct tasklet;
 	struct sk_buff_head skb_queue;
 	struct sk_buff_head skb_queue_unreliable;
+	spinlock_t tid_rx_reorder_lock;
+	struct list_head tid_rx_reorder;
 
 	/* Station data */
 	/*

@@ -1130,6 +1132,7 @@ void ieee80211_start_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u16 tid);
 void ieee80211_stop_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u8 tid);
 void ieee80211_ba_session_work(struct work_struct *work);
 void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid);
+void ieee80211_release_expired_mpdus(struct ieee80211_local *local);
 
 /* Spectrum management */
 void ieee80211_process_measurement_req(struct ieee80211_sub_if_data *sdata,

net/mac80211/main.c +4 -0

index 0e95c75..7d8e556 100644
@@ -281,6 +281,8 @@ static void ieee80211_tasklet_handler(unsigned long data)
 			break;
 		}
 	}
+
+	ieee80211_release_expired_mpdus(local);
 }
 
 static void ieee80211_restart_work(struct work_struct *work)

@@ -448,6 +450,7 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
 	local->uapsd_max_sp_len = IEEE80211_DEFAULT_MAX_SP_LEN;
 
 	INIT_LIST_HEAD(&local->interfaces);
+	INIT_LIST_HEAD(&local->tid_rx_reorder);
 
 	__hw_addr_init(&local->mc_list);

@@ -457,6 +460,7 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
 	mutex_init(&local->key_mtx);
 	spin_lock_init(&local->filter_lock);
 	spin_lock_init(&local->queue_stop_reason_lock);
+	spin_lock_init(&local->tid_rx_reorder_lock);
 
 	INIT_DELAYED_WORK(&local->scan_work, ieee80211_scan_work);

net/mac80211/rx.c +77 -4

117 lines not on this page · raw message

net/mac80211/sta_info.h +3 -0

index 54262e7..5040c47 100644
@@ -110,6 +110,7 @@ struct tid_ampdu_tx {
  * @timeout: reset timer value (in TUs).
  * @dialog_token: dialog token for aggregation session
  * @rcu_head: RCU head used for freeing this struct
+ * @reorder_list: used to release expired frames
  *
  * This structure is protected by RCU and the per-station
  * spinlock. Assignments to the array holding it must hold

@@ -121,9 +122,11 @@ struct tid_ampdu_tx {
  */
 struct tid_ampdu_rx {
 	struct rcu_head rcu_head;
+	struct list_head reorder_list;
 	struct sk_buff **reorder_buf;
 	unsigned long *reorder_time;
 	struct timer_list session_timer;
+	struct timer_list reorder_timer;
 	u16 head_seq_num;
 	u16 stored_mpdu_num;
 	u16 ssn;

Christian Lamparter

author

2010-08-02 13:16 UTC #6 lore raw

PATCH 3/3 mac80211: AMPDU rx reorder timeout timer

Everytime a hole is discovered in the reorder ring-buffer, the release timer (for this tid_ampdu_rx) is set to fire after HT_RX_REORDER_BUF_TIMEOUT jiffies.

If the missing frame(s) is/are arriving in time, the timer gets deactivated/postponed(another hole has opened up).

Else, timer releases all expired frames after the timeout period has elapsed.

Signed-off-by Christian Lamparter <[email protected]>

---
WIP -> PATCH:
- thread-safe ampdu reordering
- do the reorder release in the timer
---

net/mac80211/agg-rx.c +18 -0

index 965b272..080d792 100644
@@ -86,6 +86,7 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
 				     tid, 0, reason);
 
 	del_timer_sync(&tid_rx->session_timer);
+	del_timer_sync(&tid_rx->reorder_timer);
 
 	call_rcu(&tid_rx->rcu_head, ieee80211_free_tid_rx);
 }

@@ -120,6 +121,16 @@ static void sta_rx_agg_session_timer_expired(unsigned long data)
 	ieee80211_queue_work(&sta->local->hw, &sta->ampdu_mlme.work);
 }
 
+static void sta_rx_agg_reorder_timer_expired(unsigned long data)
+{
+	u8 *ptid = (u8 *)data;
+	u8 *timer_to_id = ptid - *ptid;
+	struct sta_info *sta = container_of(timer_to_id, struct sta_info,
+			timer_to_tid[0]);
+
+	ieee80211_release_reorder_timeout(sta, *ptid);
+}
+
 static void ieee80211_send_addba_resp(struct ieee80211_sub_if_data *sdata, u8 *da, u16 tid,
 				      u8 dialog_token, u16 status, u16 policy,
 				      u16 buf_size, u16 timeout)

@@ -251,11 +262,18 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
 		goto end;
 	}
 
+	spin_lock_init(&tid_agg_rx->lock);
+
 	/* rx timer */
 	tid_agg_rx->session_timer.function = sta_rx_agg_session_timer_expired;
 	tid_agg_rx->session_timer.data = (unsigned long)&sta->timer_to_tid[tid];
 	init_timer(&tid_agg_rx->session_timer);
 
+	/* rx reorder timer */
+	tid_agg_rx->reorder_timer.function = sta_rx_agg_reorder_timer_expired;
+	tid_agg_rx->reorder_timer.data = (unsigned long)&sta->timer_to_tid[tid];
+	init_timer(&tid_agg_rx->reorder_timer);
+
 	/* prepare reordering buffer */
 	tid_agg_rx->reorder_buf =
 		kcalloc(buf_size, sizeof(struct sk_buff *), GFP_ATOMIC);

net/mac80211/ieee80211_i.h +1 -0

index ef47006..9d1ce88 100644
@@ -1130,6 +1130,7 @@ void ieee80211_start_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u16 tid);
 void ieee80211_stop_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u8 tid);
 void ieee80211_ba_session_work(struct work_struct *work);
 void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid);
+void ieee80211_release_reorder_timeout(struct sta_info *sta, int tid);
 
 /* Spectrum management */
 void ieee80211_process_measurement_req(struct ieee80211_sub_if_data *sdata,

net/mac80211/rx.c +63 -7

131 lines not on this page · raw message

net/mac80211/sta_info.h +3 -0

index 54262e7..5e83c83 100644
@@ -110,6 +110,7 @@ struct tid_ampdu_tx {
  * @timeout: reset timer value (in TUs).
  * @dialog_token: dialog token for aggregation session
  * @rcu_head: RCU head used for freeing this struct
+ * @lock: controls exclusive access to the struct
  *
  * This structure is protected by RCU and the per-station
  * spinlock. Assignments to the array holding it must hold

@@ -121,9 +122,11 @@ struct tid_ampdu_tx {
  */
 struct tid_ampdu_rx {
 	struct rcu_head rcu_head;
+	spinlock_t lock;
 	struct sk_buff **reorder_buf;
 	unsigned long *reorder_time;
 	struct timer_list session_timer;
+	struct timer_list reorder_timer;
 	u16 head_seq_num;
 	u16 stored_mpdu_num;
 	u16 ssn;

Johannes Berg

maintainer

2010-08-02 13:24 UTC #7 lore raw

net/mac80211/sta_info.h struct tid_ampdu_tx { · line 116

Quoted 8 lines · Christian Lamparter

net/mac80211/sta_info.h +1 -0

@@ -110,6 +110,7 @@ struct tid_ampdu_tx {
  * @timeout: reset timer value (in TUs).
  * @dialog_token: dialog token for aggregation session
  * @rcu_head: RCU head used for freeing this struct
+ * @lock: controls exclusive access to the struct
  *
  * This structure is protected by RCU and the per-station
  * spinlock. Assignments to the array holding it must hold

I think that's misleading. You're only using it to control access to the reorder stuff here, not any of the other things that are used in agg-rx.c.

Also, documentation for reorder_timer is missing.

johannes