[RFC/RFT 0/2] mac80211/ath10k: fix Rx reordering

[RFC/RFT 0/2] mac80211/ath10k: fix Rx reordering

[Michal Kazior]

Hi,

I'm posting these 2 patches together in a single
patchset for easier review/testing for now. Once
review goes well I can split up the patchset and
send patches separately if it is desired so.

Recently Denton pointed out ath10k fails to
perform Rx reordering and this causes performance
issues in some cases.

Due to ath10k design I had to come up with a patch
for mac80211 to allow Rx reordering offloading via
existing Rx BA sessions code.

I've tested this pretty lightly so far. With the
patchset `iperf -u` reports 0* out-of-order
frames. Without the patch I'm seeing 400+ per
second (the particular number isn't relevant as it
depends on throughput and environment). It seems
to improve single-threaded TCP performance as
well. All test traffic is station -> ath10k AP.

* There are a few out-of-rder frames in the first
second after Rx tid is set up. I suspect this is
because ath10k is forced to start Rx BA sesion
with an arbitrary start_seq_num. 

Note: This is based on https://github.com/kvalo/ath
30a165cb84793fa3896ad93497e11eed651b1813.


Michal Kazior (2):
  mac80211: add support for Rx reordering offloading
  ath10k: fix Rx aggregation reordering

 drivers/net/wireless/ath/ath10k/htt_rx.c |  92 +++++++++++++++++++++++++-
 drivers/net/wireless/ath/ath10k/mac.c    |  30 +++++++++
 drivers/net/wireless/ath/ath10k/txrx.c   |   3 +-
 drivers/net/wireless/ath/ath10k/txrx.h   |   1 +
 include/net/mac80211.h                   |  40 ++++++++++++
 net/mac80211/agg-rx.c                    | 108 ++++++++++++++++++++++++-------
 net/mac80211/ieee80211_i.h               |  16 +++++
 net/mac80211/iface.c                     |  25 +++++++
 8 files changed, 287 insertions(+), 28 deletions(-)

-- 
1.8.5.3

[RFC/RFT 1/2] mac80211: add support for Rx reordering offloading

[Michal Kazior]

Some drivers may be performing most of Tx/Rx
aggregation on their own (e.g. in firmware)
including AddBa/DelBa negotiations but may
otherwise require Rx reordering assistance.

The patch exports 2 new functions for establishing
Rx aggregation sessions in assumption device
driver has taken care of the necessary
negotiations.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 include/net/mac80211.h     |  40 +++++++++++++++++
 net/mac80211/agg-rx.c      | 108 +++++++++++++++++++++++++++++++++++----------
 net/mac80211/ieee80211_i.h |  16 +++++++
 net/mac80211/iface.c       |  25 +++++++++++
 4 files changed, 166 insertions(+), 23 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 421b6ec..e7362f0 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -4524,6 +4524,46 @@ void ieee80211_stop_rx_ba_session(struct ieee80211_vif *vif, u16 ba_rx_bitmap,
  */
 void ieee80211_send_bar(struct ieee80211_vif *vif, u8 *ra, u16 tid, u16 ssn);
 
+/**
+ * ieee80211_start_rx_ba_session_offl - start a Rx BA session
+ *
+ * Some device drivers may offload part of the Rx aggregation flow including
+ * AddBa/DelBa negotiation but may otherwise be incapable of full Rx
+ * reordering.
+ *
+ * Create structures responsible for reordering so device drivers may call here
+ * when they complete AddBa negotiation.
+ *
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback
+ * @addr: station mac address
+ * @dialog_token:
+ * @timeout: session timeout (in TU)
+ * @start_seq_num: starting frame sequence number
+ * @tid: the rx tid
+ * @buf_size: max number of frames in reorder buffer
+ */
+void ieee80211_start_rx_ba_session_offl(struct ieee80211_vif *vif,
+					const u8 *addr, u8 dialog_token,
+					u16 timeout, u16 start_seq_num,
+					u16 tid, u16 buf_size);
+
+/**
+ * ieee80211_stop_rx_ba_session_offl - stop a Rx BA session
+ *
+ * Some device drivers may offload part of the Rx aggregation flow including
+ * AddBa/DelBa negotiation but may otherwise be incapable of full Rx
+ * reordering.
+ *
+ * Destroy structures responsible for reordering so device drivers may call here
+ * when they complete DelBa negotiation.
+ *
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback
+ * @addr: station mac address
+ * @tid: the rx tid
+ */
+void ieee80211_stop_rx_ba_session_offl(struct ieee80211_vif *vif,
+				       const u8 *addr, u16 tid);
+
 /* Rate control API */
 
 /**
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 31bf258..dbf90f5 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -224,28 +224,15 @@ static void ieee80211_send_addba_resp(struct ieee80211_sub_if_data *sdata, u8 *d
 	ieee80211_tx_skb(sdata, skb);
 }
 
-void ieee80211_process_addba_request(struct ieee80211_local *local,
-				     struct sta_info *sta,
-				     struct ieee80211_mgmt *mgmt,
-				     size_t len)
+void __ieee80211_start_rx_ba_session(struct sta_info *sta,
+				     u8 dialog_token, u16 timeout,
+				     u16 start_seq_num, u16 ba_policy, u16 tid,
+				     u16 buf_size, bool tx)
 {
+	struct ieee80211_local *local = sta->sdata->local;
 	struct tid_ampdu_rx *tid_agg_rx;
-	u16 capab, tid, timeout, ba_policy, buf_size, start_seq_num, status;
-	u8 dialog_token;
 	int ret = -EOPNOTSUPP;
-
-	/* extract session parameters from addba request frame */
-	dialog_token = mgmt->u.action.u.addba_req.dialog_token;
-	timeout = le16_to_cpu(mgmt->u.action.u.addba_req.timeout);
-	start_seq_num =
-		le16_to_cpu(mgmt->u.action.u.addba_req.start_seq_num) >> 4;
-
-	capab = le16_to_cpu(mgmt->u.action.u.addba_req.capab);
-	ba_policy = (capab & IEEE80211_ADDBA_PARAM_POLICY_MASK) >> 1;
-	tid = (capab & IEEE80211_ADDBA_PARAM_TID_MASK) >> 2;
-	buf_size = (capab & IEEE80211_ADDBA_PARAM_BUF_SIZE_MASK) >> 6;
-
-	status = WLAN_STATUS_REQUEST_DECLINED;
+	u16 status = WLAN_STATUS_REQUEST_DECLINED;
 
 	if (test_sta_flag(sta, WLAN_STA_BLOCK_BA)) {
 		ht_dbg(sta->sdata,
@@ -264,7 +251,7 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
 		status = WLAN_STATUS_INVALID_QOS_PARAM;
 		ht_dbg_ratelimited(sta->sdata,
 				   "AddBA Req with bad params from %pM on tid %u. policy %d, buffer size %d\n",
-				   mgmt->sa, tid, ba_policy, buf_size);
+				   sta->sta.addr, tid, ba_policy, buf_size);
 		goto end_no_lock;
 	}
 	/* determine default buffer size */
@@ -281,7 +268,7 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
 	if (sta->ampdu_mlme.tid_rx[tid]) {
 		ht_dbg_ratelimited(sta->sdata,
 				   "unexpected AddBA Req from %pM on tid %u\n",
-				   mgmt->sa, tid);
+				   sta->sta.addr, tid);
 
 		/* delete existing Rx BA session on the same tid */
 		___ieee80211_stop_rx_ba_session(sta, tid, WLAN_BACK_RECIPIENT,
@@ -350,6 +337,81 @@ end:
 	mutex_unlock(&sta->ampdu_mlme.mtx);
 
 end_no_lock:
-	ieee80211_send_addba_resp(sta->sdata, sta->sta.addr, tid,
-				  dialog_token, status, 1, buf_size, timeout);
+	if (tx)
+		ieee80211_send_addba_resp(sta->sdata, sta->sta.addr, tid,
+					  dialog_token, status, 1, buf_size,
+					  timeout);
+}
+
+void ieee80211_process_addba_request(struct ieee80211_local *local,
+				     struct sta_info *sta,
+				     struct ieee80211_mgmt *mgmt,
+				     size_t len)
+{
+	u16 capab, tid, timeout, ba_policy, buf_size, start_seq_num;
+	u8 dialog_token;
+
+	/* extract session parameters from addba request frame */
+	dialog_token = mgmt->u.action.u.addba_req.dialog_token;
+	timeout = le16_to_cpu(mgmt->u.action.u.addba_req.timeout);
+	start_seq_num =
+		le16_to_cpu(mgmt->u.action.u.addba_req.start_seq_num) >> 4;
+
+	capab = le16_to_cpu(mgmt->u.action.u.addba_req.capab);
+	ba_policy = (capab & IEEE80211_ADDBA_PARAM_POLICY_MASK) >> 1;
+	tid = (capab & IEEE80211_ADDBA_PARAM_TID_MASK) >> 2;
+	buf_size = (capab & IEEE80211_ADDBA_PARAM_BUF_SIZE_MASK) >> 6;
+
+	__ieee80211_start_rx_ba_session(sta, dialog_token, timeout,
+					start_seq_num, ba_policy, tid,
+					buf_size, true);
+}
+
+void ieee80211_start_rx_ba_session_offl(struct ieee80211_vif *vif,
+					const u8 *addr, u8 dialog_token,
+					u16 timeout, u16 start_seq_num,
+					u16 tid, u16 buf_size)
+{
+	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+	struct ieee80211_local *local = sdata->local;
+	struct ieee80211_rx_agg *rx_agg;
+	struct sk_buff *skb = dev_alloc_skb(0);
+
+	if (unlikely(!skb))
+		return;
+
+	rx_agg = (struct ieee80211_rx_agg *) &skb->cb;
+	memcpy(&rx_agg->addr, addr, ETH_ALEN);
+	rx_agg->dialog_token = dialog_token;
+	rx_agg->timeout = timeout;
+	rx_agg->start_seq_num = start_seq_num;
+	rx_agg->ba_policy = 1;
+	rx_agg->tid = tid;
+	rx_agg->buf_size = buf_size;
+
+	skb->pkt_type = IEEE80211_SDATA_QUEUE_RX_AGG_START;
+	skb_queue_tail(&sdata->skb_queue, skb);
+	ieee80211_queue_work(&local->hw, &sdata->work);
+}
+EXPORT_SYMBOL(ieee80211_start_rx_ba_session_offl);
+
+void ieee80211_stop_rx_ba_session_offl(struct ieee80211_vif *vif,
+				       const u8 *addr, u16 tid)
+{
+	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+	struct ieee80211_local *local = sdata->local;
+	struct ieee80211_rx_agg *rx_agg;
+	struct sk_buff *skb = dev_alloc_skb(0);
+
+	if (unlikely(!skb))
+		return;
+
+	rx_agg = (struct ieee80211_rx_agg *) &skb->cb;
+	memcpy(&rx_agg->addr, addr, ETH_ALEN);
+	rx_agg->tid = tid;
+
+	skb->pkt_type = IEEE80211_SDATA_QUEUE_RX_AGG_STOP;
+	skb_queue_tail(&sdata->skb_queue, skb);
+	ieee80211_queue_work(&local->hw, &sdata->work);
 }
+EXPORT_SYMBOL(ieee80211_stop_rx_ba_session_offl);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index ac9836e..ae35bf1 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -892,10 +892,22 @@ ieee80211_vif_get_shift(struct ieee80211_vif *vif)
 	return shift;
 }
 
+struct ieee80211_rx_agg {
+	u8 addr[ETH_ALEN];
+	u8 dialog_token;
+	u16 timeout;
+	u16 start_seq_num;
+	u16 ba_policy;
+	u16 tid;
+	u16 buf_size;
+};
+
 enum sdata_queue_type {
 	IEEE80211_SDATA_QUEUE_TYPE_FRAME	= 0,
 	IEEE80211_SDATA_QUEUE_AGG_START		= 1,
 	IEEE80211_SDATA_QUEUE_AGG_STOP		= 2,
+	IEEE80211_SDATA_QUEUE_RX_AGG_START	= 3,
+	IEEE80211_SDATA_QUEUE_RX_AGG_STOP	= 4,
 };
 
 enum {
@@ -1540,6 +1552,10 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
 				     u16 initiator, u16 reason, bool stop);
 void __ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
 				    u16 initiator, u16 reason, bool stop);
+void __ieee80211_start_rx_ba_session(struct sta_info *sta,
+				     u8 dialog_token, u16 timeout,
+				     u16 start_seq_num, u16 ba_policy, u16 tid,
+				     u16 buf_size, bool tx);
 void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta,
 					 enum ieee80211_agg_stop_reason reason);
 void ieee80211_process_delba(struct ieee80211_sub_if_data *sdata,
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 388b863..3d9b135 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1139,6 +1139,7 @@ static void ieee80211_iface_work(struct work_struct *work)
 	struct sk_buff *skb;
 	struct sta_info *sta;
 	struct ieee80211_ra_tid *ra_tid;
+	struct ieee80211_rx_agg *rx_agg;
 
 	if (!ieee80211_sdata_running(sdata))
 		return;
@@ -1166,6 +1167,30 @@ static void ieee80211_iface_work(struct work_struct *work)
 			ra_tid = (void *)&skb->cb;
 			ieee80211_stop_tx_ba_cb(&sdata->vif, ra_tid->ra,
 						ra_tid->tid);
+		} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_START) {
+			rx_agg = (void *)&skb->cb;
+			mutex_lock(&local->sta_mtx);
+			sta = sta_info_get_bss(sdata, rx_agg->addr);
+			if (sta)
+				__ieee80211_start_rx_ba_session(sta,
+							rx_agg->dialog_token,
+							rx_agg->timeout,
+							rx_agg->start_seq_num,
+							rx_agg->ba_policy,
+							rx_agg->tid,
+							rx_agg->buf_size,
+							false);
+			mutex_unlock(&local->sta_mtx);
+		} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_STOP) {
+			rx_agg = (void *)&skb->cb;
+			mutex_lock(&local->sta_mtx);
+			sta = sta_info_get_bss(sdata, rx_agg->addr);
+			if (sta)
+				__ieee80211_stop_rx_ba_session(sta,
+							rx_agg->tid,
+							WLAN_BACK_RECIPIENT, 0,
+							false);
+			mutex_unlock(&local->sta_mtx);
 		} else if (ieee80211_is_action(mgmt->frame_control) &&
 			   mgmt->u.action.category == WLAN_CATEGORY_BACK) {
 			int len = skb->len;
-- 
1.8.5.3

Re: [RFC/RFT 1/2] mac80211: add support for Rx reordering offloading

[Johannes Berg]

On Fri, 2014-06-27 at 11:06 +0200, Michal Kazior wrote:

> +/**
> + * ieee80211_start_rx_ba_session_offl - start a Rx BA session
> + *
> + * Some device drivers may offload part of the Rx aggregation flow including
> + * AddBa/DelBa negotiation but may otherwise be incapable of full Rx
> + * reordering.
> + *
> + * Create structures responsible for reordering so device drivers may call here
> + * when they complete AddBa negotiation.
> + *
> + * @vif: &struct ieee80211_vif pointer from the add_interface callback
> + * @addr: station mac address
> + * @dialog_token:
> + * @timeout: session timeout (in TU)

Why would you need the dialog token (and why no docs?) and timeout?

> + * @start_seq_num: starting frame sequence number
> + * @tid: the rx tid

> + * @buf_size: max number of frames in reorder buffer

The buf_size also isn't really needed, is it? We are allowed to use a
bigger buffer, I believe?

> +void ieee80211_start_rx_ba_session_offl(struct ieee80211_vif *vif,
> +					const u8 *addr, u8 dialog_token,
> +					u16 timeout, u16 start_seq_num,
> +					u16 tid, u16 buf_size)
> +{
> +	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> +	struct ieee80211_local *local = sdata->local;
> +	struct ieee80211_rx_agg *rx_agg;
> +	struct sk_buff *skb = dev_alloc_skb(0);
> +
> +	if (unlikely(!skb))
> +		return;
> +
> +	rx_agg = (struct ieee80211_rx_agg *) &skb->cb;
> +	memcpy(&rx_agg->addr, addr, ETH_ALEN);
> +	rx_agg->dialog_token = dialog_token;
> +	rx_agg->timeout = timeout;
> +	rx_agg->start_seq_num = start_seq_num;
> +	rx_agg->ba_policy = 1;
> +	rx_agg->tid = tid;
> +	rx_agg->buf_size = buf_size;
> +
> +	skb->pkt_type = IEEE80211_SDATA_QUEUE_RX_AGG_START;
> +	skb_queue_tail(&sdata->skb_queue, skb);
> +	ieee80211_queue_work(&local->hw, &sdata->work);

This seems problematic, since packets might be received immediately.

On the teardown path it should probably also be invalidated immediately,
no?

Then again, we have the same problem already? Hmm.

johannes

Re: [RFC/RFT 1/2] mac80211: add support for Rx reordering offloading

[Michal Kazior]

On 8 July 2014 09:10, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2014-06-27 at 11:06 +0200, Michal Kazior wrote:
>
>> +/**
>> + * ieee80211_start_rx_ba_session_offl - start a Rx BA session
>> + *
>> + * Some device drivers may offload part of the Rx aggregation flow including
>> + * AddBa/DelBa negotiation but may otherwise be incapable of full Rx
>> + * reordering.
>> + *
>> + * Create structures responsible for reordering so device drivers may call here
>> + * when they complete AddBa negotiation.
>> + *
>> + * @vif: &struct ieee80211_vif pointer from the add_interface callback
>> + * @addr: station mac address
>> + * @dialog_token:
>> + * @timeout: session timeout (in TU)
>
> Why would you need the dialog token (and why no docs?) and timeout?

Good point. I'll remove both.


>> + * @start_seq_num: starting frame sequence number
>> + * @tid: the rx tid
>
>> + * @buf_size: max number of frames in reorder buffer
>
> The buf_size also isn't really needed, is it? We are allowed to use a
> bigger buffer, I believe?

Good point. I'll use the max buffer size macro.


>> +void ieee80211_start_rx_ba_session_offl(struct ieee80211_vif *vif,
>> +                                     const u8 *addr, u8 dialog_token,
>> +                                     u16 timeout, u16 start_seq_num,
>> +                                     u16 tid, u16 buf_size)
>> +{
>> +     struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
>> +     struct ieee80211_local *local = sdata->local;
>> +     struct ieee80211_rx_agg *rx_agg;
>> +     struct sk_buff *skb = dev_alloc_skb(0);
>> +
>> +     if (unlikely(!skb))
>> +             return;
>> +
>> +     rx_agg = (struct ieee80211_rx_agg *) &skb->cb;
>> +     memcpy(&rx_agg->addr, addr, ETH_ALEN);
>> +     rx_agg->dialog_token = dialog_token;
>> +     rx_agg->timeout = timeout;
>> +     rx_agg->start_seq_num = start_seq_num;
>> +     rx_agg->ba_policy = 1;
>> +     rx_agg->tid = tid;
>> +     rx_agg->buf_size = buf_size;
>> +
>> +     skb->pkt_type = IEEE80211_SDATA_QUEUE_RX_AGG_START;
>> +     skb_queue_tail(&sdata->skb_queue, skb);
>> +     ieee80211_queue_work(&local->hw, &sdata->work);
>
> This seems problematic, since packets might be received immediately.
>
> On the teardown path it should probably also be invalidated immediately,
> no?
>
> Then again, we have the same problem already? Hmm.

Yeah. You have to go through the iface worker so you can race with
data frames coming in in ieee80211_rx() either way.

You'd have to make ampdu_action() atomic, replace sta->ampdu_mlme.mtx
with a spinlock and move some del_timer_sync() around - at least
that's what I've found after taking a quick look.


Michał

[RFC/RFT 2/2] ath10k: fix Rx aggregation reordering

[Michal Kazior]

Firmware doesn't perform Rx reordering so it is
left to the host driver to do that.

Use mac80211 to perform reordering instead of
re-inventing the wheel.

This fixes TCP throughput issues in some
environments (notably Windows stations connecting
to ath10k AP).

Reported-By: Denton Gentry <denton.gentry@gmail.com>
Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 92 ++++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/mac.c    | 30 +++++++++++
 drivers/net/wireless/ath/ath10k/txrx.c   |  3 +-
 drivers/net/wireless/ath/ath10k/txrx.h   |  1 +
 4 files changed, 121 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 6c102b1..57d5a97 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -21,6 +21,7 @@
 #include "txrx.h"
 #include "debug.h"
 #include "trace.h"
+#include "mac.h"
 
 #include <linux/log2.h>
 
@@ -1533,10 +1534,95 @@ void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
 	case HTT_T2H_MSG_TYPE_STATS_CONF:
 		trace_ath10k_htt_stats(skb->data, skb->len);
 		break;
+	case HTT_T2H_MSG_TYPE_RX_ADDBA: {
+		struct ath10k *ar = htt->ar;
+		struct htt_rx_addba *ev = &resp->rx_addba;
+		struct ath10k_peer *peer;
+		struct ath10k_vif *arvif;
+		u16 info0, tid, peer_id;
+
+		info0 = __le32_to_cpu(ev->info0);
+		tid = MS(info0, HTT_RX_BA_INFO0_TID);
+		peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
+
+		ath10k_dbg(ATH10K_DBG_HTT,
+			   "htt rx addba tid %hu peer_id %hu size %hhu\n",
+			   tid, peer_id, ev->window_size);
+
+		spin_lock_bh(&ar->data_lock);
+		peer = ath10k_peer_find_by_id(ar, peer_id);
+		if (!peer) {
+			ath10k_warn("received addba event for invalid peer_id: %hu\n",
+				    peer_id);
+			spin_unlock_bh(&ar->data_lock);
+			break;
+		}
+
+		arvif = ath10k_get_arvif(ar, peer->vdev_id);
+		if (!arvif) {
+			ath10k_warn("received addba event for invalid vdev_id: %u\n",
+				    peer->vdev_id);
+			spin_unlock_bh(&ar->data_lock);
+			break;
+		}
+
+		ath10k_dbg(ATH10K_DBG_HTT,
+			   "htt rx start rx ba session sta %pM tid %hu size %hhu\n",
+			   peer->addr, tid, ev->window_size);
+
+		ieee80211_start_rx_ba_session_offl(arvif->vif, peer->addr, 0,
+						   0, 0, tid, ev->window_size);
+		spin_unlock_bh(&ar->data_lock);
+		break;
+	}
+	case HTT_T2H_MSG_TYPE_RX_DELBA: {
+		struct ath10k *ar = htt->ar;
+		struct htt_rx_addba *ev = &resp->rx_addba;
+		struct ath10k_peer *peer;
+		struct ath10k_vif *arvif;
+		u16 info0, tid, peer_id;
+
+		info0 = __le32_to_cpu(ev->info0);
+		tid = MS(info0, HTT_RX_BA_INFO0_TID);
+		peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
+
+		ath10k_dbg(ATH10K_DBG_HTT,
+			   "htt rx delba tid %hu peer_id %hu\n",
+			   tid, peer_id);
+
+		spin_lock_bh(&ar->data_lock);
+		peer = ath10k_peer_find_by_id(ar, peer_id);
+		if (!peer) {
+			ath10k_warn("received addba event for invalid peer_id: %hu\n",
+				    peer_id);
+			spin_unlock_bh(&ar->data_lock);
+			break;
+		}
+
+		arvif = ath10k_get_arvif(ar, peer->vdev_id);
+		if (!arvif) {
+			ath10k_warn("received addba event for invalid vdev_id: %u\n",
+				    peer->vdev_id);
+			spin_unlock_bh(&ar->data_lock);
+			break;
+		}
+
+		ath10k_dbg(ATH10K_DBG_HTT,
+			   "htt rx stop rx ba session sta %pM tid %hu\n",
+			   peer->addr, tid);
+
+		ieee80211_stop_rx_ba_session_offl(arvif->vif, peer->addr, tid);
+		spin_unlock_bh(&ar->data_lock);
+
+		break;
+	}
+	case HTT_T2H_MSG_TYPE_RX_FLUSH: {
+		/* Ignore this event because mac80211 takes care of Rx
+		 * aggregation reordering.
+		 */
+		break;
+	}
 	case HTT_T2H_MSG_TYPE_TX_INSPECT_IND:
-	case HTT_T2H_MSG_TYPE_RX_ADDBA:
-	case HTT_T2H_MSG_TYPE_RX_DELBA:
-	case HTT_T2H_MSG_TYPE_RX_FLUSH:
 	default:
 		ath10k_dbg(ATH10K_DBG_HTT, "htt event (%d) not handled\n",
 			   resp->hdr.msg_type);
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index a210800..766fead 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4330,6 +4330,35 @@ static u64 ath10k_get_tsf(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
 	return 0;
 }
 
+int ath10k_ampdu_action(struct ieee80211_hw *hw,
+			struct ieee80211_vif *vif,
+			enum ieee80211_ampdu_mlme_action action,
+			struct ieee80211_sta *sta, u16 tid, u16 *ssn,
+			u8 buf_size)
+{
+	ath10k_dbg(ATH10K_DBG_MAC, "mac ampdu action %d\n", action);
+
+	switch (action) {
+	case IEEE80211_AMPDU_RX_START:
+	case IEEE80211_AMPDU_RX_STOP:
+		/* HTT AddBa/DelBa events trigger mac80211 Rx BA session
+		 * creation/removal. Do we need to verify this?
+		 */
+		return 0;
+	case IEEE80211_AMPDU_TX_START:
+	case IEEE80211_AMPDU_TX_STOP_CONT:
+	case IEEE80211_AMPDU_TX_STOP_FLUSH:
+	case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
+	case IEEE80211_AMPDU_TX_OPERATIONAL:
+		/* Firmware offloads Tx aggregation entirely so deny mac80211
+		 * Tx aggregation requests.
+		 */
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static const struct ieee80211_ops ath10k_ops = {
 	.tx				= ath10k_tx,
 	.start				= ath10k_start,
@@ -4357,6 +4386,7 @@ static const struct ieee80211_ops ath10k_ops = {
 	.set_bitrate_mask		= ath10k_set_bitrate_mask,
 	.sta_rc_update			= ath10k_sta_rc_update,
 	.get_tsf			= ath10k_get_tsf,
+	.ampdu_action			= ath10k_ampdu_action,
 #ifdef CONFIG_PM
 	.suspend			= ath10k_suspend,
 	.resume				= ath10k_resume,
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index 82669a7..f4fa22d 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -119,8 +119,7 @@ struct ath10k_peer *ath10k_peer_find(struct ath10k *ar, int vdev_id,
 	return NULL;
 }
 
-static struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k *ar,
-						  int peer_id)
+struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k *ar, int peer_id)
 {
 	struct ath10k_peer *peer;
 
diff --git a/drivers/net/wireless/ath/ath10k/txrx.h b/drivers/net/wireless/ath/ath10k/txrx.h
index aee3e20..a90e09f 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.h
+++ b/drivers/net/wireless/ath/ath10k/txrx.h
@@ -24,6 +24,7 @@ void ath10k_txrx_tx_unref(struct ath10k_htt *htt,
 
 struct ath10k_peer *ath10k_peer_find(struct ath10k *ar, int vdev_id,
 				     const u8 *addr);
+struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k *ar, int peer_id);
 int ath10k_wait_for_peer_created(struct ath10k *ar, int vdev_id,
 				 const u8 *addr);
 int ath10k_wait_for_peer_deleted(struct ath10k *ar, int vdev_id,
-- 
1.8.5.3

Re: [RFC/RFT 2/2] ath10k: fix Rx aggregation reordering

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> Firmware doesn't perform Rx reordering so it is
> left to the host driver to do that.
>
> Use mac80211 to perform reordering instead of
> re-inventing the wheel.
>
> This fixes TCP throughput issues in some
> environments (notably Windows stations connecting
> to ath10k AP).
>
> Reported-By: Denton Gentry <denton.gentry@gmail.com>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
> @@ -21,6 +21,7 @@
>  #include "txrx.h"
>  #include "debug.h"
>  #include "trace.h"
> +#include "mac.h"

Why this? At least from a quick search I didn't see any reason to add this.

>  
>  #include <linux/log2.h>
>  
> @@ -1533,10 +1534,95 @@ void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
>  	case HTT_T2H_MSG_TYPE_STATS_CONF:
>  		trace_ath10k_htt_stats(skb->data, skb->len);
>  		break;
> +	case HTT_T2H_MSG_TYPE_RX_ADDBA: {
> +		struct ath10k *ar = htt->ar;
> +		struct htt_rx_addba *ev = &resp->rx_addba;
> +		struct ath10k_peer *peer;
> +		struct ath10k_vif *arvif;
> +		u16 info0, tid, peer_id;
> +
> +		info0 = __le32_to_cpu(ev->info0);
> +		tid = MS(info0, HTT_RX_BA_INFO0_TID);
> +		peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);

This event handling is pretty long. I think it would be better to have a
separate function for this event.

> +	case HTT_T2H_MSG_TYPE_RX_DELBA: {
> +		struct ath10k *ar = htt->ar;
> +		struct htt_rx_addba *ev = &resp->rx_addba;
> +		struct ath10k_peer *peer;
> +		struct ath10k_vif *arvif;
> +		u16 info0, tid, peer_id;

And the same for this event.

-- 
Kalle Valo

Re: [RFC/RFT 2/2] ath10k: fix Rx aggregation reordering

[Michal Kazior]

On 27 June 2014 16:17, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> Firmware doesn't perform Rx reordering so it is
>> left to the host driver to do that.
>>
>> Use mac80211 to perform reordering instead of
>> re-inventing the wheel.
>>
>> This fixes TCP throughput issues in some
>> environments (notably Windows stations connecting
>> to ath10k AP).
>>
>> Reported-By: Denton Gentry <denton.gentry@gmail.com>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> [...]
>
>> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
>> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
>> @@ -21,6 +21,7 @@
>>  #include "txrx.h"
>>  #include "debug.h"
>>  #include "trace.h"
>> +#include "mac.h"
>
> Why this? At least from a quick search I didn't see any reason to add this.

ath10k_get_arvif()


>>
>>  #include <linux/log2.h>
>>
>> @@ -1533,10 +1534,95 @@ void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
>>       case HTT_T2H_MSG_TYPE_STATS_CONF:
>>               trace_ath10k_htt_stats(skb->data, skb->len);
>>               break;
>> +     case HTT_T2H_MSG_TYPE_RX_ADDBA: {
>> +             struct ath10k *ar = htt->ar;
>> +             struct htt_rx_addba *ev = &resp->rx_addba;
>> +             struct ath10k_peer *peer;
>> +             struct ath10k_vif *arvif;
>> +             u16 info0, tid, peer_id;
>> +
>> +             info0 = __le32_to_cpu(ev->info0);
>> +             tid = MS(info0, HTT_RX_BA_INFO0_TID);
>> +             peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
>
> This event handling is pretty long. I think it would be better to have a
> separate function for this event.
>
>> +     case HTT_T2H_MSG_TYPE_RX_DELBA: {
>> +             struct ath10k *ar = htt->ar;
>> +             struct htt_rx_addba *ev = &resp->rx_addba;
>> +             struct ath10k_peer *peer;
>> +             struct ath10k_vif *arvif;
>> +             u16 info0, tid, peer_id;
>
> And the same for this event.

Good idea. I'll do that.


Michał

Re: [RFC/RFT 2/2] ath10k: fix Rx aggregation reordering

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 27 June 2014 16:17, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> @@ -21,6 +21,7 @@
>>>  #include "txrx.h"
>>>  #include "debug.h"
>>>  #include "trace.h"
>>> +#include "mac.h"
>>
>> Why this? At least from a quick search I didn't see any reason to add this.
>
> ath10k_get_arvif()

Ah, ok. BTW, in the future we should rename it to ath10k_mac_get_arvif()
to make it clear it's coming from mac.c.

-- 
Kalle Valo

Re: [RFC/RFT 0/2] mac80211/ath10k: fix Rx reordering

[Denton Gentry]

With these two changes the iperf throughput of the Windows STA
increased to 283 Mbps, up from ~45 Mbps prior to these changes.

Everything else is as constant as I can hold it, no other code changes
and in the same physical location. It is an open air office
environment, the noise environment can vary day to day.

On Fri, Jun 27, 2014 at 2:06 AM, Michal Kazior <michal.kazior@tieto.com> wrote:
> Hi,
>
> I'm posting these 2 patches together in a single
> patchset for easier review/testing for now. Once
> review goes well I can split up the patchset and
> send patches separately if it is desired so.
>
> Recently Denton pointed out ath10k fails to
> perform Rx reordering and this causes performance
> issues in some cases.
>
> Due to ath10k design I had to come up with a patch
> for mac80211 to allow Rx reordering offloading via
> existing Rx BA sessions code.
>
> I've tested this pretty lightly so far. With the
> patchset `iperf -u` reports 0* out-of-order
> frames. Without the patch I'm seeing 400+ per
> second (the particular number isn't relevant as it
> depends on throughput and environment). It seems
> to improve single-threaded TCP performance as
> well. All test traffic is station -> ath10k AP.
>
> * There are a few out-of-rder frames in the first
> second after Rx tid is set up. I suspect this is
> because ath10k is forced to start Rx BA sesion
> with an arbitrary start_seq_num.
>
> Note: This is based on https://github.com/kvalo/ath
> 30a165cb84793fa3896ad93497e11eed651b1813.
>
>
> Michal Kazior (2):
>   mac80211: add support for Rx reordering offloading
>   ath10k: fix Rx aggregation reordering
>
>  drivers/net/wireless/ath/ath10k/htt_rx.c |  92 +++++++++++++++++++++++++-
>  drivers/net/wireless/ath/ath10k/mac.c    |  30 +++++++++
>  drivers/net/wireless/ath/ath10k/txrx.c   |   3 +-
>  drivers/net/wireless/ath/ath10k/txrx.h   |   1 +
>  include/net/mac80211.h                   |  40 ++++++++++++
>  net/mac80211/agg-rx.c                    | 108 ++++++++++++++++++++++++-------
>  net/mac80211/ieee80211_i.h               |  16 +++++
>  net/mac80211/iface.c                     |  25 +++++++
>  8 files changed, 287 insertions(+), 28 deletions(-)
>
> --
> 1.8.5.3
>