Bug 166357

Summary:	[ath] 802.11n TX stall when the first frame in the BAW is in the software queue
Product:	Base System	Reporter:	Adrian Chadd <adrian>
Component:	wireless	Assignee:	freebsd-wireless (Nobody) <wireless>
Status:	Closed FIXED
Severity:	Affects Only Me	CC:	emaste, gonzo
Priority:	Normal	Flags:	bugmeister: mfc-stable10? bugmeister: mfc-stable9? bugmeister: mfc-stable8?
Version:	10.0-CURRENT
Hardware:	Any
OS:	Any

Description Adrian Chadd freebsd_committer

2012-03-23 21:10:06 UTC

I'm seeing TX stalls after a bunch of bidirectional traffic in a lossy 802.11n environment.

Every single time, I see:

* the TID isn't scheduled;
* The software queue for that TID has the first and last seqno in the BAW in the software queue (along with other frames here and there);
* .. i _think_ the HW transmission has failed, so the frames appear again in the SWQ;
* .. but it's thus not being rescheduled.

I'll post further information soon.

Comment 1 Adrian Chadd freebsd_committer

2012-03-23 21:14:27 UTC

Responsible Changed
From-To: freebsd-bugs->freebsd-wireless

reassign to -wireless.

Comment 2 dfilter service freebsd_committer

2012-03-26 00:50:44 UTC

Author: adrian
Date: Sun Mar 25 23:50:34 2012
New Revision: 233480
URL: http://svn.freebsd.org/changeset/base/233480

Log:
  Add some more debugging to try and nail down exactly what's going on when
  I see traffic stalls.
  
  It turns out that the bug isn't because the first and last frame in the
  BAW is in the software queue.  It is more likely that it's because
  the first frame in the BAW is still in the software queue and thus there's
  no more room to allocate and do subsequent TX.
  
  PR:		kern/166357

Modified:
  head/sys/dev/ath/if_ath_tx.c

Modified: head/sys/dev/ath/if_ath_tx.c
==============================================================================
--- head/sys/dev/ath/if_ath_tx.c	Sun Mar 25 21:54:36 2012	(r233479)
+++ head/sys/dev/ath/if_ath_tx.c	Sun Mar 25 23:50:34 2012	(r233480)
@@ -2357,6 +2357,7 @@ ath_tx_xmit_aggr(struct ath_softc *sc, s
 	/* paused? queue */
 	if (tid->paused) {
 		ATH_TXQ_INSERT_TAIL(tid, bf, bf_list);
+		/* XXX don't sched - we're paused! */
 		return;
 	}
 
@@ -2647,13 +2648,19 @@ ath_tx_tid_drain(struct ath_softc *sc, s
 		if (t == 0) {
 			device_printf(sc->sc_dev,
 			    "%s: node %p: bf=%p: addbaw=%d, dobaw=%d, "
-			    "seqno_assign=%d, seqno_required=%d, seqno=%d\n",
+			    "seqno_assign=%d, seqno_required=%d, seqno=%d, retry=%d\n",
 			    __func__, ni, bf,
 			    bf->bf_state.bfs_addedbaw,
 			    bf->bf_state.bfs_dobaw,
 			    bf->bf_state.bfs_need_seqno,
 			    bf->bf_state.bfs_seqno_assigned,
-			    SEQNO(bf->bf_state.bfs_seqno));
+			    SEQNO(bf->bf_state.bfs_seqno),
+			    bf->bf_state.bfs_retries);
+			device_printf(sc->sc_dev,
+			    "%s: node %p: bf=%p: tid txq_depth=%d hwq_depth=%d\n",
+			    __func__, ni, bf,
+			    tid->axq_depth,
+			    tid->hwq_depth);
 			device_printf(sc->sc_dev,
 			    "%s: node %p: bf=%p: tid %d: txq_depth=%d, "
 			    "txq_aggr_depth=%d, sched=%d, paused=%d, "
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 3 dfilter service freebsd_committer

2012-03-26 17:05:38 UTC

Author: adrian
Date: Mon Mar 26 16:05:19 2012
New Revision: 233514
URL: http://svn.freebsd.org/changeset/base/233514

Log:
  Use the assigned sequence number when checking if a retried packet is
  within the BAW.
  
  This regression was introduced in ane earlier commit by me to fix the
  BAW seqno allocation-but-not-insertion-into-BAW race.  Since it was only
  ever using the to-be allocated sequence number, any frame retries
  with the first frame in the BAW still in the software queue would
  have constantly failed, as ni_txseqs[tid] would always be outside
  the BAW.
  
  TODO:
  
  * Extract out the mostly common code here in the agg and non-agg ADDBA
    case and stuff it into a single function.
  
  PR:		kern/166357

Modified:
  head/sys/dev/ath/if_ath_tx.c
  head/sys/dev/ath/if_ath_tx_ht.c

Modified: head/sys/dev/ath/if_ath_tx.c
==============================================================================
--- head/sys/dev/ath/if_ath_tx.c	Mon Mar 26 15:38:17 2012	(r233513)
+++ head/sys/dev/ath/if_ath_tx.c	Mon Mar 26 16:05:19 2012	(r233514)
@@ -2348,7 +2348,6 @@ ath_tx_xmit_aggr(struct ath_softc *sc, s
 	struct ath_tid *tid = &an->an_tid[bf->bf_state.bfs_tid];
 	struct ath_txq *txq = bf->bf_state.bfs_txq;
 	struct ieee80211_tx_ampdu *tap;
-	int seqno;
 
 	ATH_TXQ_LOCK_ASSERT(txq);
 
@@ -2384,13 +2383,31 @@ ath_tx_xmit_aggr(struct ath_softc *sc, s
 	 * the TIDs that map to it.  Ugh.
 	 */
 	if (bf->bf_state.bfs_dobaw) {
-		if (! BAW_WITHIN(tap->txa_start, tap->txa_wnd,
-		    ni->ni_txseqs[bf->bf_state.bfs_tid])) {
+		ieee80211_seq seqno;
+
+		/*
+		 * If the sequence number is allocated, use it.
+		 * Otherwise, use the sequence number we WOULD
+		 * allocate.
+		 */
+		if (bf->bf_state.bfs_seqno_assigned)
+			seqno = SEQNO(bf->bf_state.bfs_seqno);
+		else
+			seqno = ni->ni_txseqs[bf->bf_state.bfs_tid];
+
+		/*
+		 * Check whether either the currently allocated
+		 * sequence number _OR_ the to-be allocated
+		 * sequence number is inside the BAW.
+		 */
+		if (! BAW_WITHIN(tap->txa_start, tap->txa_wnd, seqno)) {
 			ATH_TXQ_INSERT_TAIL(tid, bf, bf_list);
 			ath_tx_tid_sched(sc, tid);
 			return;
 		}
 		if (! bf->bf_state.bfs_seqno_assigned) {
+			int seqno;
+
 			seqno = ath_tx_tid_seqno_assign(sc, ni, bf, bf->bf_m);
 			if (seqno < 0) {
 				device_printf(sc->sc_dev,

Modified: head/sys/dev/ath/if_ath_tx_ht.c
==============================================================================
--- head/sys/dev/ath/if_ath_tx_ht.c	Mon Mar 26 15:38:17 2012	(r233513)
+++ head/sys/dev/ath/if_ath_tx_ht.c	Mon Mar 26 16:05:19 2012	(r233514)
@@ -652,7 +652,6 @@ ath_tx_form_aggr(struct ath_softc *sc, s
 	int status = ATH_AGGR_DONE;
 	int prev_frames = 0;	/* XXX for AR5416 burst, not done here */
 	int prev_al = 0;	/* XXX also for AR5416 burst */
-	int seqno;
 
 	ATH_TXQ_LOCK_ASSERT(sc->sc_ac2q[tid->ac]);
 
@@ -747,13 +746,32 @@ ath_tx_form_aggr(struct ath_softc *sc, s
 		 * see ath_tx_xmit_aggr() for more info.
 		 */
 		if (bf->bf_state.bfs_dobaw) {
+			ieee80211_seq seqno;
+
+			/*
+			 * If the sequence number is allocated, use it.
+			 * Otherwise, use the sequence number we WOULD
+			 * allocate.
+			 */
+			if (bf->bf_state.bfs_seqno_assigned)
+				seqno = SEQNO(bf->bf_state.bfs_seqno);
+			else
+				seqno = ni->ni_txseqs[bf->bf_state.bfs_tid];
+
+			/*
+			 * Check whether either the currently allocated
+			 * sequence number _OR_ the to-be allocated
+			 * sequence number is inside the BAW.
+			 */
 			if (! BAW_WITHIN(tap->txa_start, tap->txa_wnd,
-			    ni->ni_txseqs[bf->bf_state.bfs_tid])) {
+			    seqno)) {
 				status = ATH_AGGR_BAW_CLOSED;
 				break;
 			}
+
 			/* XXX check for bfs_need_seqno? */
 			if (! bf->bf_state.bfs_seqno_assigned) {
+				int seqno;
 				seqno = ath_tx_tid_seqno_assign(sc, ni, bf, bf->bf_m);
 				if (seqno < 0) {
 					device_printf(sc->sc_dev,
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 4 Adrian Chadd freebsd_committer

2012-04-02 17:38:30 UTC

State Changed
From-To: open->patched

Fix

Comment 5 Ed Maste freebsd_committer

2018-10-04 00:44:05 UTC

Adrian is this fixed now?

Comment 6 Oleksandr Tymoshenko freebsd_committer

2019-01-20 01:17:46 UTC

There was a commit referencing this bug, but it's still not closed and has been inactive for some time. Closing as fixed. Please re-open it if the issue hasn't been completely resolved.