[PATCH 0/3] Add session timeout support for tx aggregation

[PATCH 0/3] Add session timeout support for tx aggregation

[Nikolay Martynov]

  Currently tx aggregation is not being timed out even if timeout is specified when aggregation is opened. Tx tid stays active until delba arrives from recipient (i.e. recipient times out tid when it is inactive). The problem with this approach is that delba can get lost in the air and tx tid will stay perpetually opened on the originator while closed on recipient thus all data sent via this tid will be lost.
  The problem manifests itself with connection becoming slow/unusable with ping times jumping to 4s. At such time opened tx tid can be seen on one side of the connection without corresponding rx tid one the other side. This seems to be happening quite often soon after connection on ar9102 I have.
  This patch implements tx tid timeouting in way very similar to rx tid timeouting.

  All comments and suggestions are appreciated.
  Thanks.

kolya (3):
  mac80211: add tx agg session timer to timeout inactive tids in way
    similar to rx agg sessions are being timed out
  mac80211: use WLAN_BACK_RECIPIENT instead of hardcoded 0
  mac80211: format debugfs agg_status output

 net/mac80211/agg-rx.c      |    7 ++++---
 net/mac80211/agg-tx.c      |   35 ++++++++++++++++++++++++++++++++++-
 net/mac80211/debugfs_sta.c |    8 +++++---
 net/mac80211/sta_info.h    |    2 ++
 net/mac80211/tx.c          |    9 +++++++++
 5 files changed, 54 insertions(+), 7 deletions(-)

-- 
1.7.4.1

[PATCH 1/3] mac80211: add tx agg session timer to timeout inactive tids in way similar to rx agg sessions are being timed out

[Nikolay Martynov]

From: kolya <mar.kolya@gmail.com>

---
 net/mac80211/agg-tx.c   |   35 ++++++++++++++++++++++++++++++++++-
 net/mac80211/sta_info.h |    2 ++
 net/mac80211/tx.c       |    9 +++++++++
 3 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 39d72cc..915f3ff 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -349,6 +349,28 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
 				     tid_tx->timeout);
 }
 
+/*
+ * After accepting the AddBA Response we activated a timer,
+ * resetting it after each frame that we send.
+ */
+static void sta_tx_agg_session_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]);
+
+#ifdef CONFIG_MAC80211_HT_DEBUG
+	printk(KERN_DEBUG "tx session timer expired on tid %d\n", (u16)*ptid);
+#endif
+
+        ieee80211_stop_tx_ba_session(&sta->sta, *ptid);
+}
+
 int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid,
 				  u16 timeout)
 {
@@ -418,11 +440,16 @@ int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid,
 
 	tid_tx->timeout = timeout;
 
-	/* Tx timer */
+	/* response timer */
 	tid_tx->addba_resp_timer.function = sta_addba_resp_timer_expired;
 	tid_tx->addba_resp_timer.data = (unsigned long)&sta->timer_to_tid[tid];
 	init_timer(&tid_tx->addba_resp_timer);
 
+	/* tx timer */
+	tid_tx->session_timer.function = sta_tx_agg_session_timer_expired;
+	tid_tx->session_timer.data = (unsigned long)&sta->timer_to_tid[tid];
+	init_timer(&tid_tx->session_timer);
+
 	/* assign a dialog token */
 	sta->ampdu_mlme.dialog_token_allocator++;
 	tid_tx->dialog_token = sta->ampdu_mlme.dialog_token_allocator;
@@ -689,6 +716,8 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u8 tid)
 	 * more.
 	 */
 
+	del_timer_sync(&tid_tx->session_timer);
+
 	ieee80211_agg_splice_packets(local, tid_tx, tid);
 
 	/* future packets must not find the tid_tx struct any more */
@@ -778,6 +807,10 @@ void ieee80211_process_addba_resp(struct ieee80211_local *local,
 			ieee80211_agg_tx_operational(local, sta, tid);
 
 		sta->ampdu_mlme.addba_req_num[tid] = 0;
+
+                if (tid_tx->timeout)
+                        mod_timer(&tid_tx->session_timer, TU_TO_EXP_TIME(tid_tx->timeout));
+
 	} else {
 		___ieee80211_stop_tx_ba_session(sta, tid, WLAN_BACK_INITIATOR,
 						true);
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index c5923ab..6027a53 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -90,6 +90,7 @@ enum ieee80211_sta_info_flags {
  * struct tid_ampdu_tx - TID aggregation information (Tx).
  *
  * @rcu_head: rcu head for freeing structure
+ * @session_timer: check if we keep Tx-ing on the TID (by timeout value)
  * @addba_resp_timer: timer for peer's response to addba request
  * @pending: pending frames queue -- use sta's spinlock to protect
  * @dialog_token: dialog token for aggregation session
@@ -112,6 +113,7 @@ enum ieee80211_sta_info_flags {
  */
 struct tid_ampdu_tx {
 	struct rcu_head rcu_head;
+	struct timer_list session_timer;
 	struct timer_list addba_resp_timer;
 	struct sk_buff_head pending;
 	unsigned long state;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 30d212a..a6d46ca 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1044,6 +1044,10 @@ static bool ieee80211_tx_prep_agg(struct ieee80211_tx_data *tx,
 
 	if (test_bit(HT_AGG_STATE_OPERATIONAL, &tid_tx->state)) {
 		info->flags |= IEEE80211_TX_CTL_AMPDU;
+                /* reset session timer */
+                if (tid_tx->timeout)
+                        mod_timer(&tid_tx->session_timer,
+                                  TU_TO_EXP_TIME(tid_tx->timeout));
 	} else if (test_bit(HT_AGG_STATE_WANT_START, &tid_tx->state)) {
 		/*
 		 * nothing -- this aggregation session is being started
@@ -1075,6 +1079,11 @@ static bool ieee80211_tx_prep_agg(struct ieee80211_tx_data *tx,
 			/* do nothing, let packet pass through */
 		} else if (test_bit(HT_AGG_STATE_OPERATIONAL, &tid_tx->state)) {
 			info->flags |= IEEE80211_TX_CTL_AMPDU;
+
+                        /* reset session timer */
+                        if (tid_tx->timeout)
+                                mod_timer(&tid_tx->session_timer,
+                                          TU_TO_EXP_TIME(tid_tx->timeout));
 		} else {
 			queued = true;
 			info->control.vif = &tx->sdata->vif;
-- 
1.7.4.1

[PATCH 2/3] mac80211: use WLAN_BACK_RECIPIENT instead of hardcoded 0

[Nikolay Martynov]

From: kolya <mar.kolya@gmail.com>

---
 net/mac80211/agg-rx.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 476b106..449d77b 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -73,8 +73,9 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
 	RCU_INIT_POINTER(sta->ampdu_mlme.tid_rx[tid], NULL);
 
 #ifdef CONFIG_MAC80211_HT_DEBUG
-	printk(KERN_DEBUG "Rx BA session stop requested for %pM tid %u\n",
-	       sta->sta.addr, tid);
+	printk(KERN_DEBUG "Rx BA session stop requested for %pM tid %u %s reason: %d\n",
+	       sta->sta.addr, tid, initiator == WLAN_BACK_RECIPIENT ? "recipient" : "inititator",
+               (int)reason);
 #endif /* CONFIG_MAC80211_HT_DEBUG */
 
 	if (drv_ampdu_action(local, sta->sdata, IEEE80211_AMPDU_RX_STOP,
@@ -85,7 +86,7 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
 	/* check if this is a self generated aggregation halt */
 	if (initiator == WLAN_BACK_RECIPIENT && tx)
 		ieee80211_send_delba(sta->sdata, sta->sta.addr,
-				     tid, 0, reason);
+				     tid, WLAN_BACK_RECIPIENT, reason);
 
 	del_timer_sync(&tid_rx->session_timer);
 	del_timer_sync(&tid_rx->reorder_timer);
-- 
1.7.4.1

[PATCH 3/3] mac80211: format debugfs agg_status output

[Nikolay Martynov]

From: kolya <mar.kolya@gmail.com>

---
 net/mac80211/debugfs_sta.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index c5f3417..e010419 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -154,7 +154,7 @@ static ssize_t sta_agg_status_read(struct file *file, char __user *userbuf,
 	p += scnprintf(p, sizeof(buf) + buf - p, "next dialog_token: %#02x\n",
 			sta->ampdu_mlme.dialog_token_allocator + 1);
 	p += scnprintf(p, sizeof(buf) + buf - p,
-		       "TID\t\tRX active\tDTKN\tSSN\t\tTX\tDTKN\tpending\n");
+		       "TID\t\tRX active\tDTKN\tSSN\t\tTX active\tDTKN\ttimeout\tpending\n");
 
 	for (i = 0; i < STA_TID_NUM; i++) {
 		tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[i]);
@@ -162,14 +162,16 @@ static ssize_t sta_agg_status_read(struct file *file, char __user *userbuf,
 
 		p += scnprintf(p, sizeof(buf) + buf - p, "%02d", i);
 		p += scnprintf(p, sizeof(buf) + buf - p, "\t\t%x", !!tid_rx);
-		p += scnprintf(p, sizeof(buf) + buf - p, "\t%#.2x",
+		p += scnprintf(p, sizeof(buf) + buf - p, "\t\t%#.2x",
 				tid_rx ? tid_rx->dialog_token : 0);
 		p += scnprintf(p, sizeof(buf) + buf - p, "\t%#.3x",
 				tid_rx ? tid_rx->ssn : 0);
 
 		p += scnprintf(p, sizeof(buf) + buf - p, "\t\t%x", !!tid_tx);
-		p += scnprintf(p, sizeof(buf) + buf - p, "\t%#.2x",
+		p += scnprintf(p, sizeof(buf) + buf - p, "\t\t%#.2x",
 				tid_tx ? tid_tx->dialog_token : 0);
+		p += scnprintf(p, sizeof(buf) + buf - p, "\t%d",
+                               tid_tx ? (int)tid_tx->timeout : 0);
 		p += scnprintf(p, sizeof(buf) + buf - p, "\t%03d",
 				tid_tx ? skb_queue_len(&tid_tx->pending) : 0);
 		p += scnprintf(p, sizeof(buf) + buf - p, "\n");
-- 
1.7.4.1

Re: [PATCH 2/3] mac80211: use WLAN_BACK_RECIPIENT instead of hardcoded 0

[Johannes Berg]

On Sun, 2011-11-20 at 21:22 -0500, Nikolay Martynov wrote:
> From: kolya <mar.kolya@gmail.com>

> ---

Missing signature (see Documentation/SubmittingPatches).

>  net/mac80211/agg-rx.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
> index 476b106..449d77b 100644
> --- a/net/mac80211/agg-rx.c
> +++ b/net/mac80211/agg-rx.c
> @@ -73,8 +73,9 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
>  	RCU_INIT_POINTER(sta->ampdu_mlme.tid_rx[tid], NULL);
>  
>  #ifdef CONFIG_MAC80211_HT_DEBUG
> -	printk(KERN_DEBUG "Rx BA session stop requested for %pM tid %u\n",
> -	       sta->sta.addr, tid);
> +	printk(KERN_DEBUG "Rx BA session stop requested for %pM tid %u %s reason: %d\n",
> +	       sta->sta.addr, tid, initiator == WLAN_BACK_RECIPIENT ? "recipient" : "inititator",
> +               (int)reason);

please break lines to 80 cols (and maybe run scripts/checkpatch.pl)

johannes

Re: [PATCH 1/3] mac80211: add tx agg session timer to timeout inactive tids in way similar to rx agg sessions are being timed out

[Johannes Berg]

On Sun, 2011-11-20 at 21:22 -0500, Nikolay Martynov wrote:
> From: kolya <mar.kolya@gmail.com>
> 
> ---

Same comment as on patch 2, but additionally: please make a short &
concise subject line and explain the change in proper text in the commit
log.

> @@ -689,6 +716,8 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u8 tid)
>  	 * more.
>  	 */
>  
> +	del_timer_sync(&tid_tx->session_timer);
> +
>  	ieee80211_agg_splice_packets(local, tid_tx, tid);

This is a deadlock waiting to happen.

> @@ -778,6 +807,10 @@ void ieee80211_process_addba_resp(struct ieee80211_local *local,
>  			ieee80211_agg_tx_operational(local, sta, tid);
>  
>  		sta->ampdu_mlme.addba_req_num[tid] = 0;
> +
> +                if (tid_tx->timeout)
> +                        mod_timer(&tid_tx->session_timer, TU_TO_EXP_TIME(tid_tx->timeout));
> +

line breaks please

johannes

Re: [PATCH 3/3] mac80211: format debugfs agg_status output

[Johannes Berg]

On Sun, 2011-11-20 at 21:22 -0500, Nikolay Martynov wrote:
> From: kolya <mar.kolya@gmail.com>
> 
> ---

Need patch description.

johannes

Re: [PATCH 1/3] mac80211: add tx agg session timer to timeout inactive tids in way similar to rx agg sessions are being timed out

[Johannes Berg]

On Mon, 2011-11-21 at 10:57 -0500, Nikolay Martynov wrote:

> >> @@ -689,6 +716,8 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u8 tid)
> >>        * more.
> >>        */
> >>
> >> +     del_timer_sync(&tid_tx->session_timer);
> >> +
> >>       ieee80211_agg_splice_packets(local, tid_tx, tid);
> >
> > This is a deadlock waiting to happen.

>   I'd really appreciate if you could be a bit more specific on why
> this could lead to deadlock. Thanks!

It's within the spinlock, and you take the spinlock in the timer. So
this can happen:

cpu 1				cpu 2
stop_tx_ba_cb
  spin_lock_bh(&sta->lock);
				start your timer function
  del_timer_sync
    wait for timer to finish
				spin_lock_bh(&sta->lock);

johannes