[PATCH] mac80211: retry sending failed BAR frames later instead of tearing down aggr

[PATCH] mac80211: retry sending failed BAR frames later instead of tearing down aggr

[Felix Fietkau]

Unfortunately failed BAR tx attempts happen more frequently than I
expected, and the resulting aggregation teardowns cause performance
issues, as the aggregation session does not always get re-established
properly.
Instead of tearing down the entire aggr session, we can simply store the
SSN of the last failed BAR tx attempt, wait for the first successful
tx status event, and then send another BAR with the same SSN.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
Cc: Helmut Schaa <helmut.schaa@googlemail.com>
---
 net/mac80211/sta_info.h |    5 +++++
 net/mac80211/status.c   |   37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index e9eb565..56a3d38 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -86,6 +86,8 @@ enum ieee80211_sta_info_flags {
  * @stop_initiator: initiator of a session stop
  * @tx_stop: TX DelBA frame when stopping
  * @buf_size: reorder buffer size at receiver
+ * @failed_bar_ssn: ssn of the last failed BAR tx attempt
+ * @bar_pending: BAR needs to be re-sent
  *
  * This structure's lifetime is managed by RCU, assignments to
  * the array holding it must hold the aggregation mutex.
@@ -106,6 +108,9 @@ struct tid_ampdu_tx {
 	u8 stop_initiator;
 	bool tx_stop;
 	u8 buf_size;
+
+	u16 failed_bar_ssn;
+	bool bar_pending;
 };
 
 /**
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index e51bd2a..ba405bc 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -127,12 +127,32 @@ static void ieee80211_handle_filtered_frame(struct ieee80211_local *local,
 	dev_kfree_skb(skb);
 }
 
+static void ieee80211_check_pending_bar(struct sta_info *sta, u8 *addr, u8 tid)
+{
+	struct tid_ampdu_tx *tid_tx;
+
+	tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[tid]);
+	if (!tid_tx || !tid_tx->bar_pending)
+		return;
+
+	tid_tx->bar_pending = false;
+	ieee80211_send_bar(sta->sdata, addr, tid, tid_tx->failed_bar_ssn);
+}
+
 static void ieee80211_frame_acked(struct sta_info *sta, struct sk_buff *skb)
 {
 	struct ieee80211_mgmt *mgmt = (void *) skb->data;
 	struct ieee80211_local *local = sta->local;
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
 
+	if (ieee80211_is_data_qos(mgmt->frame_control)) {
+		struct ieee80211_hdr *hdr = (void *) skb->data;
+		u8 *qc = ieee80211_get_qos_ctl(hdr);
+		u16 tid = qc[0] & 0xf;
+
+		ieee80211_check_pending_bar(sta, hdr->addr1, tid);
+	}
+
 	if (ieee80211_is_action(mgmt->frame_control) &&
 	    sdata->vif.type == NL80211_IFTYPE_STATION &&
 	    mgmt->u.action.category == WLAN_CATEGORY_HT &&
@@ -161,6 +181,18 @@ static void ieee80211_frame_acked(struct sta_info *sta, struct sk_buff *skb)
 	}
 }
 
+static void ieee80211_set_bar_pending(struct sta_info *sta, u8 tid, u16 ssn)
+{
+	struct tid_ampdu_tx *tid_tx;
+
+	tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[tid]);
+	if (!tid_tx)
+		return;
+
+	tid_tx->failed_bar_ssn = ssn;
+	tid_tx->bar_pending = true;
+}
+
 /*
  * Use a static threshold for now, best value to be determined
  * by testing ...
@@ -254,10 +286,13 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
 			 */
 			bar = (struct ieee80211_bar *) skb->data;
 			if (!(bar->control & IEEE80211_BAR_CTRL_MULTI_TID)) {
+				u16 ssn = le16_to_cpu(bar->start_seq_num);
+
 				tid = (bar->control &
 				       IEEE80211_BAR_CTRL_TID_INFO_MASK) >>
 				      IEEE80211_BAR_CTRL_TID_INFO_SHIFT;
-				ieee80211_stop_tx_ba_session(&sta->sta, tid);
+
+				ieee80211_set_bar_pending(sta, tid, ssn);
 			}
 		}
 
-- 
1.7.3.2

Re: [PATCH] mac80211: retry sending failed BAR frames later instead of tearing down aggr

[Helmut Schaa]

On Sun, Aug 28, 2011 at 9:11 PM, Felix Fietkau <nbd@openwrt.org> wrote:
> Unfortunately failed BAR tx attempts happen more frequently than I
> expected, and the resulting aggregation teardowns cause performance
> issues, as the aggregation session does not always get re-established
> properly.
> Instead of tearing down the entire aggr session, we can simply store the
> SSN of the last failed BAR tx attempt, wait for the first successful
> tx status event, and then send another BAR with the same SSN.
>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> Cc: Helmut Schaa <helmut.schaa@googlemail.com>

[...]

>  static void ieee80211_frame_acked(struct sta_info *sta, struct sk_buff *skb)
>  {
>        struct ieee80211_mgmt *mgmt = (void *) skb->data;
>        struct ieee80211_local *local = sta->local;
>        struct ieee80211_sub_if_data *sdata = sta->sdata;
>
> +       if (ieee80211_is_data_qos(mgmt->frame_control)) {
> +               struct ieee80211_hdr *hdr = (void *) skb->data;
> +               u8 *qc = ieee80211_get_qos_ctl(hdr);
> +               u16 tid = qc[0] & 0xf;
> +
> +               ieee80211_check_pending_bar(sta, hdr->addr1, tid);
> +       }
> +

This code will not be run for injected QoS frames. Is this intended?
Otherwise looks good to me.

Helmut

Re: [PATCH] mac80211: retry sending failed BAR frames later instead of tearing down aggr

[Felix Fietkau]

On 2011-08-29 8:58 AM, Helmut Schaa wrote:
> On Sun, Aug 28, 2011 at 9:11 PM, Felix Fietkau<nbd@openwrt.org>  wrote:
>>  Unfortunately failed BAR tx attempts happen more frequently than I
>>  expected, and the resulting aggregation teardowns cause performance
>>  issues, as the aggregation session does not always get re-established
>>  properly.
>>  Instead of tearing down the entire aggr session, we can simply store the
>>  SSN of the last failed BAR tx attempt, wait for the first successful
>>  tx status event, and then send another BAR with the same SSN.
>>
>>  Signed-off-by: Felix Fietkau<nbd@openwrt.org>
>>  Cc: Helmut Schaa<helmut.schaa@googlemail.com>
>
> [...]
>
>>    static void ieee80211_frame_acked(struct sta_info *sta, struct sk_buff *skb)
>>    {
>>          struct ieee80211_mgmt *mgmt = (void *) skb->data;
>>          struct ieee80211_local *local = sta->local;
>>          struct ieee80211_sub_if_data *sdata = sta->sdata;
>>
>>  +       if (ieee80211_is_data_qos(mgmt->frame_control)) {
>>  +               struct ieee80211_hdr *hdr = (void *) skb->data;
>>  +               u8 *qc = ieee80211_get_qos_ctl(hdr);
>>  +               u16 tid = qc[0]&  0xf;
>>  +
>>  +               ieee80211_check_pending_bar(sta, hdr->addr1, tid);
>>  +       }
>>  +
>
> This code will not be run for injected QoS frames. Is this intended?
> Otherwise looks good to me.
I didn't consider it, but I think it's fine. I don't expect any 
aggregated injected traffic under normal circumstances anyway.

- Felix

Re: [PATCH] mac80211: retry sending failed BAR frames later instead of tearing down aggr

[Johannes Berg]

On Sun, 2011-08-28 at 21:11 +0200, Felix Fietkau wrote:
> Unfortunately failed BAR tx attempts happen more frequently than I
> expected, and the resulting aggregation teardowns cause performance
> issues, as the aggregation session does not always get re-established
> properly.

I find this curious. How can tons of traffic go through, but BAR frames
fail? These are unicast and should be retried a bunch...

> Instead of tearing down the entire aggr session, we can simply store the
> SSN of the last failed BAR tx attempt, wait for the first successful
> tx status event, and then send another BAR with the same SSN.

So what if it keeps failing? I think eventually we'd want to kill the
session?

johannes

Re: [PATCH] mac80211: retry sending failed BAR frames later instead of tearing down aggr

[Helmut Schaa]

On Wed, Aug 31, 2011 at 2:15 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Sun, 2011-08-28 at 21:11 +0200, Felix Fietkau wrote:
>> Unfortunately failed BAR tx attempts happen more frequently than I
>> expected, and the resulting aggregation teardowns cause performance
>> issues, as the aggregation session does not always get re-established
>> properly.

Felix, so this also happens with ath9k (not only rt2800pci)?

> I find this curious. How can tons of traffic go through, but BAR frames
> fail? These are unicast and should be retried a bunch...

As I said before I could only reproduce this very seldom and the failure
was always just a temporary one. Only one or two AMPDUs failed for
whatever reason and the following BAR as well. Afterwards traffic was
going through again.

Helmut

Re: [PATCH] mac80211: retry sending failed BAR frames later instead of tearing down aggr

[Felix Fietkau]

On 2011-08-31 2:15 PM, Johannes Berg wrote:
> On Sun, 2011-08-28 at 21:11 +0200, Felix Fietkau wrote:
>>  Unfortunately failed BAR tx attempts happen more frequently than I
>>  expected, and the resulting aggregation teardowns cause performance
>>  issues, as the aggregation session does not always get re-established
>>  properly.
>
> I find this curious. How can tons of traffic go through, but BAR frames
> fail? These are unicast and should be retried a bunch...
I think it mostly happens when the client goes to powersave (triggered 
by background scans, etc).

>>  Instead of tearing down the entire aggr session, we can simply store the
>>  SSN of the last failed BAR tx attempt, wait for the first successful
>>  tx status event, and then send another BAR with the same SSN.
>
> So what if it keeps failing? I think eventually we'd want to kill the
> session?
If it keeps failing, then the connection is probably so bad that it 
doesn't matter if we kill the session or not.

- Felix

Re: [PATCH] mac80211: retry sending failed BAR frames later instead of tearing down aggr

[Felix Fietkau]

On 2011-08-31 2:31 PM, Helmut Schaa wrote:
> On Wed, Aug 31, 2011 at 2:15 PM, Johannes Berg
> <johannes@sipsolutions.net>  wrote:
>>  On Sun, 2011-08-28 at 21:11 +0200, Felix Fietkau wrote:
>>>  Unfortunately failed BAR tx attempts happen more frequently than I
>>>  expected, and the resulting aggregation teardowns cause performance
>>>  issues, as the aggregation session does not always get re-established
>>>  properly.
>
> Felix, so this also happens with ath9k (not only rt2800pci)?
Right. I think it can happen with any driver if the client goes into 
powersave during heavy traffic.

- Felix

Re: [PATCH] mac80211: retry sending failed BAR frames later instead of tearing down aggr

[Johannes Berg]

On Wed, 2011-08-31 at 16:47 +0200, Felix Fietkau wrote:
> On 2011-08-31 2:15 PM, Johannes Berg wrote:
> > On Sun, 2011-08-28 at 21:11 +0200, Felix Fietkau wrote:
> >>  Unfortunately failed BAR tx attempts happen more frequently than I
> >>  expected, and the resulting aggregation teardowns cause performance
> >>  issues, as the aggregation session does not always get re-established
> >>  properly.
> >
> > I find this curious. How can tons of traffic go through, but BAR frames
> > fail? These are unicast and should be retried a bunch...

> I think it mostly happens when the client goes to powersave (triggered 
> by background scans, etc).

Huh? That frame should be buffered properly.

> >>  Instead of tearing down the entire aggr session, we can simply store the
> >>  SSN of the last failed BAR tx attempt, wait for the first successful
> >>  tx status event, and then send another BAR with the same SSN.
> >
> > So what if it keeps failing? I think eventually we'd want to kill the
> > session?

> If it keeps failing, then the connection is probably so bad that it 
> doesn't matter if we kill the session or not.

Hm, yeah, I guess it wouldn't really matter.

johannes