[PATCH v2 0/3] mac80211: timeout tx agg sessions in way similar to rx agg sessions

[PATCH v2 0/3] mac80211: timeout tx agg sessions in way similar to rx agg sessions

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

Signed-off-by: Nikolay Martynov <mar.kolya@gmail.com>

Nikolay Martynov (3):
  mac80211: timeout tx agg sessions in way similar to rx agg sessions
  mac80211: trivial: use WLAN_BACK_RECIPIENT instead of hardcoded 0
  mac80211: log reason and initiator when rx agg is stopped

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

-- 
1.7.4.1

[PATCH v2 1/3] mac80211: timeout tx agg sessions in way similar to rx agg sessions

[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.
  This patch implements tx tid timeouting in way very similar to rx tid
timeouting.

Signed-off-by: Nikolay Martynov <mar.kolya@gmail.com>
---
 net/mac80211/agg-tx.c   |   35 ++++++++++++++++++++++++++++++++++-
 net/mac80211/sta_info.h |    2 ++
 net/mac80211/tx.c       |    8 ++++++++
 3 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 39d72cc..a2d9654 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -180,6 +180,7 @@ int ___ieee80211_stop_tx_ba_session(struct sta_info *sta, u16 tid,
 	set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);
 
 	del_timer_sync(&tid_tx->addba_resp_timer);
+	del_timer_sync(&tid_tx->session_timer);
 
 	/*
 	 * After this packets are no longer handed right through
@@ -349,6 +350,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 +441,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;
@@ -778,6 +806,11 @@ 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 6280e8b..ccd34e9 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -88,6 +88,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
@@ -110,6 +111,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 8d31933..7b74f59 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1063,9 +1063,11 @@ static bool ieee80211_tx_prep_agg(struct ieee80211_tx_data *tx,
 				  int tid)
 {
 	bool queued = false;
+	bool reset_agg_timer = false;
 
 	if (test_bit(HT_AGG_STATE_OPERATIONAL, &tid_tx->state)) {
 		info->flags |= IEEE80211_TX_CTL_AMPDU;
+		reset_agg_timer = true;
 	} else if (test_bit(HT_AGG_STATE_WANT_START, &tid_tx->state)) {
 		/*
 		 * nothing -- this aggregation session is being started
@@ -1097,6 +1099,7 @@ 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_agg_timer = true;
 		} else {
 			queued = true;
 			info->control.vif = &tx->sdata->vif;
@@ -1106,6 +1109,11 @@ static bool ieee80211_tx_prep_agg(struct ieee80211_tx_data *tx,
 		spin_unlock(&tx->sta->lock);
 	}
 
+	/* reset session timer */
+	if (reset_agg_timer && tid_tx->timeout)
+		mod_timer(&tid_tx->session_timer,
+			  TU_TO_EXP_TIME(tid_tx->timeout));
+
 	return queued;
 }
 
-- 
1.7.4.1

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

[Nikolay Martynov]

  Use WLAN_BACK_RECIPIENT instead of hardcoded 0 for clarity

Signed-off-by: Nikolay Martynov <mar.kolya@gmail.com>
---
 net/mac80211/agg-rx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 476b106..d4f23d4 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -85,7 +85,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 v2 3/3] mac80211: log reason and initiator when rx agg is stopped

[Nikolay Martynov]

  Add additional debug logging of initiator and reason when rx
aggregation session is stopped

Signed-off-by: Nikolay Martynov <mar.kolya@gmail.com>
---
 net/mac80211/agg-rx.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index d4f23d4..98208b6 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -73,8 +73,11 @@ 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,
-- 
1.7.4.1

Re: [PATCH v2 0/3] mac80211: timeout tx agg sessions in way similar to rx agg sessions

[Johannes Berg]

On Tue, 2011-11-22 at 21:50 -0500, Nikolay Martynov wrote:
> 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.

Looks OK to me. Did you run it through sparse too? :-)

johannes

Re: [PATCH v2 0/3] mac80211: timeout tx agg sessions in way similar to rx agg sessions

[Nikolay Martynov]

Hi,

 Thanks for spending your time looking at my patch and providing feedback!

2011/11/24 Johannes Berg <johannes@sipsolutions.net>:
> On Tue, 2011-11-22 at 21:50 -0500, Nikolay Martynov wrote:
>> 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.
>
> Looks OK to me. Did you run it through sparse too? :-)
>
> johannes
>
>

 Please forgive my lack of experience, but could you please point me
to some docs where I can read about sparse and how to use it? I'd
really appreciate this.

 Also, I one more thing I forgot to mention in original cover letter.
Some time before this patch I've posted patch for ath9k: "[PATCH v3]
ath9k: improve ath_tx_aggr_stop to avoid TID stuck in cleanup state".
I've noticed that bug fixed in ath9k patch is triggered much more
often when this patch to mac80211 applied. Probably because of some
timing issues when TID is being closed from both ends at about same
time.
 What I mean to say is that this patch to mac80211 should probably be
applied after that patch to ath9k to avoid making ath9k less stable.

 And one more thing, if you do not mind.
'ieee80211_stop_tx_ba_session' tests tx_tid->state for
HT_AGG_STATE_STOPPING holding only spinlock on sta->lock.
'___ieee80211_stop_tx_ba_session' sets this bit when spin lock on
sta->lock is not being held. It seems to be that this could lead to
problems if these two functions get called at about same time (which
could easily happen when this patch is applied). Although actual
window when lock is not help is really small. But I think the way it
is currently done could still lead to problems. I think
'set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);' should be moved up
to be covered by spin lock. I can include this as a part of next
version of my patch. I'd really appreciate your thoughts about this.

Thanks!

-- 
Truthfully yours,
Martynov Nikolay.
Email: mar.kolya@gmail.com

Re: [PATCH v2 0/3] mac80211: timeout tx agg sessions in way similar to rx agg sessions

[Johannes Berg]

On Thu, 2011-11-24 at 13:33 -0500, Nikolay Martynov wrote:

> > Looks OK to me. Did you run it through sparse too? :-)
> >
>   Please forgive my lack of experience, but could you please point me
> to some docs where I can read about sparse and how to use it? I'd
> really appreciate this.

Sure, FWIW, I'm just asking because I received lots of patches recently
that later got sparse warnings. I guess you normally run something like

make M=net/mac80211

To run sparse, you add C=1 (just run sparse if cc runs) or C=2 (run
sparse on all files). Obviously you need to install sparse first, your
distribution probably has packages.

>   Also, I one more thing I forgot to mention in original cover letter.
> Some time before this patch I've posted patch for ath9k: "[PATCH v3]
> ath9k: improve ath_tx_aggr_stop to avoid TID stuck in cleanup state".
> I've noticed that bug fixed in ath9k patch is triggered much more
> often when this patch to mac80211 applied. Probably because of some
> timing issues when TID is being closed from both ends at about same
> time.
>   What I mean to say is that this patch to mac80211 should probably be
> applied after that patch to ath9k to avoid making ath9k less stable.

You should send that info to the list not just me, I've added the list
back.

>   And one more thing, if you do not mind.
> 'ieee80211_stop_tx_ba_session' tests tx_tid->state for
> HT_AGG_STATE_STOPPING holding only spinlock on sta->lock.
> '___ieee80211_stop_tx_ba_session' sets this bit when spin lock on
> sta->lock is not being held. It seems to be that this could lead to
> problems if these two functions get called at about same time (which
> could easily happen when this patch is applied). Although actual
> window when lock is not help is really small. But I think the way it
> is currently done could still lead to problems. I think
> 'set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);' should be moved up
> to be covered by spin lock. I can include this as a part of next
> version of my patch. I'd really appreciate your thoughts about this.

Yeah that does indeed seem like an issue -- please send a separate patch
outside of this series and add Cc: stable@vger.kernel.org (to the patch
description not to the actual email, if you're not familiar with the
stable submission rules please check for those)

johannes

Re: [PATCH v2 0/3] mac80211: timeout tx agg sessions in way similar to rx agg sessions

[Johannes Berg]

On Thu, 2011-11-24 at 19:39 +0100, Johannes Berg wrote:

> >   And one more thing, if you do not mind.
> > 'ieee80211_stop_tx_ba_session' tests tx_tid->state for
> > HT_AGG_STATE_STOPPING holding only spinlock on sta->lock.
> > '___ieee80211_stop_tx_ba_session' sets this bit when spin lock on
> > sta->lock is not being held. It seems to be that this could lead to
> > problems if these two functions get called at about same time (which
> > could easily happen when this patch is applied). Although actual
> > window when lock is not help is really small. But I think the way it
> > is currently done could still lead to problems. I think
> > 'set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);' should be moved up
> > to be covered by spin lock. I can include this as a part of next
> > version of my patch. I'd really appreciate your thoughts about this.
> 
> Yeah that does indeed seem like an issue

Actually I think the right way to fix it would be the change below, do
you agree? I think just moving the set_bit doesn't help because other
code might call right into ___ieee80211_stop_tx_ba_session (or with just
two underscores on front)

johannes

--- wireless-testing.orig/net/mac80211/agg-tx.c	2011-11-09 10:07:34.000000000 +0100
+++ wireless-testing/net/mac80211/agg-tx.c	2011-11-24 19:44:51.000000000 +0100
@@ -162,6 +162,12 @@ int ___ieee80211_stop_tx_ba_session(stru
 		return -ENOENT;
 	}
 
+	/* if we're already stopping ignore any new requests to stop */
+	if (test_bit(HT_AGG_STATE_STOPPING, &tid_tx->state)) {
+		spin_unlock_bh(&sta->lock);
+		return -EALREADY;
+	}
+
 	if (test_bit(HT_AGG_STATE_WANT_START, &tid_tx->state)) {
 		/* not even started yet! */
 		ieee80211_assign_tid_tx(sta, tid, NULL);
@@ -170,6 +176,8 @@ int ___ieee80211_stop_tx_ba_session(stru
 		return 0;
 	}
 
+	set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);
+
 	spin_unlock_bh(&sta->lock);
 
 #ifdef CONFIG_MAC80211_HT_DEBUG
@@ -177,8 +185,6 @@ int ___ieee80211_stop_tx_ba_session(stru
 	       sta->sta.addr, tid);
 #endif /* CONFIG_MAC80211_HT_DEBUG */
 
-	set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);
-
 	del_timer_sync(&tid_tx->addba_resp_timer);
 
 	/*

Re: [PATCH v2 0/3] mac80211: timeout tx agg sessions in way similar to rx agg sessions

[Nikolay Martynov]

Hi

2011/11/24 Johannes Berg <johannes@sipsolutions.net>:
> On Thu, 2011-11-24 at 19:39 +0100, Johannes Berg wrote:
>
>> >   And one more thing, if you do not mind.
>> > 'ieee80211_stop_tx_ba_session' tests tx_tid->state for
>> > HT_AGG_STATE_STOPPING holding only spinlock on sta->lock.
>> > '___ieee80211_stop_tx_ba_session' sets this bit when spin lock on
>> > sta->lock is not being held. It seems to be that this could lead to
>> > problems if these two functions get called at about same time (which
>> > could easily happen when this patch is applied). Although actual
>> > window when lock is not help is really small. But I think the way it
>> > is currently done could still lead to problems. I think
>> > 'set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);' should be moved up
>> > to be covered by spin lock. I can include this as a part of next
>> > version of my patch. I'd really appreciate your thoughts about this.
>>
>> Yeah that does indeed seem like an issue
>
> Actually I think the right way to fix it would be the change below, do
> you agree? I think just moving the set_bit doesn't help because other
> code might call right into ___ieee80211_stop_tx_ba_session (or with just
> two underscores on front)
>
> johannes
>
> --- wireless-testing.orig/net/mac80211/agg-tx.c 2011-11-09 10:07:34.000000000 +0100
> +++ wireless-testing/net/mac80211/agg-tx.c      2011-11-24 19:44:51.000000000 +0100
> @@ -162,6 +162,12 @@ int ___ieee80211_stop_tx_ba_session(stru
>                return -ENOENT;
>        }
>
> +       /* if we're already stopping ignore any new requests to stop */
> +       if (test_bit(HT_AGG_STATE_STOPPING, &tid_tx->state)) {
> +               spin_unlock_bh(&sta->lock);
> +               return -EALREADY;
> +       }
> +
>        if (test_bit(HT_AGG_STATE_WANT_START, &tid_tx->state)) {
>                /* not even started yet! */
>                ieee80211_assign_tid_tx(sta, tid, NULL);
> @@ -170,6 +176,8 @@ int ___ieee80211_stop_tx_ba_session(stru
>                return 0;
>        }
>
> +       set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);
> +
>        spin_unlock_bh(&sta->lock);
>
>  #ifdef CONFIG_MAC80211_HT_DEBUG
> @@ -177,8 +185,6 @@ int ___ieee80211_stop_tx_ba_session(stru
>               sta->sta.addr, tid);
>  #endif /* CONFIG_MAC80211_HT_DEBUG */
>
> -       set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);
> -
>        del_timer_sync(&tid_tx->addba_resp_timer);
>
>        /*
>
>
>

Yes, this patch does make sense to me.

-- 
Truthfully yours,
Martynov Nikolay.
Email: mar.kolya@gmail.com

Re: [PATCH v2 0/3] mac80211: timeout tx agg sessions in way similar to rx agg sessions

[Nikolay Martynov]

Hi

2011/11/24 Johannes Berg <johannes@sipsolutions.net>:
> On Thu, 2011-11-24 at 13:33 -0500, Nikolay Martynov wrote:
>
>> > Looks OK to me. Did you run it through sparse too? :-)
>> >
>>   Please forgive my lack of experience, but could you please point me
>> to some docs where I can read about sparse and how to use it? I'd
>> really appreciate this.
>
> Sure, FWIW, I'm just asking because I received lots of patches recently
> that later got sparse warnings. I guess you normally run something like
>
> make M=net/mac80211
>
> To run sparse, you add C=1 (just run sparse if cc runs) or C=2 (run
> sparse on all files). Obviously you need to install sparse first, your
> distribution probably has packages.
>

  I wasn't able to find any sparse warnings introduced by my changes,
everything looks fine.

-- 
Truthfully yours,
Martynov Nikolay.
Email: mar.kolya@gmail.com

Re: [PATCH v2 0/3] mac80211: timeout tx agg sessions in way similar to rx agg sessions

[Johannes Berg]

On Thu, 2011-11-24 at 23:43 -0500, Nikolay Martynov wrote:

> >> > Looks OK to me. Did you run it through sparse too? :-)

>   I wasn't able to find any sparse warnings introduced by my changes,
> everything looks fine.

Great, thanks for checking.

johannes