Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 6 Aug 2011 04:49:25 +0000 (UTC)
From:      Adrian Chadd <adrian@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-user@freebsd.org
Subject:   svn commit: r224669 - user/adrian/if_ath_tx/sys/dev/ath
Message-ID:  <201108060449.p764nP87035521@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: adrian
Date: Sat Aug  6 04:49:25 2011
New Revision: 224669
URL: http://svn.freebsd.org/changeset/base/224669

Log:
  Now that aggregation and BAW tracking are in place, a whole lot of bad
  assumptions (on my part, unfortunately) have crept up.
  
  * Add some more debugging.
  * Store away the original packet priority in ath_buf
  * The TID is being incorrectly assigned. The TID should be 16 for
    non-QoS packets, regardless of what the priority is.
    The trouble is, the rest of the code assumes all packets in the
    given TID are destined for the same hardware txq - ie, they all
    have the same WME access class/priority value.
  
  For example, if a packet has priority 0 (via M_WME_GETAC()) and
  is a QoS enabled packet, it'll get a TID of 0. When aggregation
  is enabled, these packets will also be tossed into the aggregation
  state. Fine.
  
  If a packet has priority 0 but isn't a QoS enabled packet, it'll also
  get a TID of 0. This breaks things - non-QoS packets get thrown
  into the aggregation state and suddenly everything is unhappy.
  
  If I put non-QoS packets into TID 16, then I'm in the unhappy
  situation that the packet priority may be non-zero (well, it may
  be non-constant) and things suddenly crash.
  
  This actually occurs in ieee80211_mgmt_output():
  
  	ieee80211_send_setup(ni, m,
  	     IEEE80211_FC0_TYPE_MGT | type, IEEE80211_NONQOS_TID,
  	     vap->iv_myaddr, ni->ni_macaddr, ni->ni_bssid);
  ...
  	M_WME_SETAC(m, params->ibp_pri);
  
  So before I can continue, I likely need to correctly handle
  TID=16 (non-QoS frames) which can have differing AC/PRI, and thus
  different hardware TX queues. This influences what the locking
  requirements are.

Modified:
  user/adrian/if_ath_tx/sys/dev/ath/if_ath_tx.c
  user/adrian/if_ath_tx/sys/dev/ath/if_athvar.h

Modified: user/adrian/if_ath_tx/sys/dev/ath/if_ath_tx.c
==============================================================================
--- user/adrian/if_ath_tx/sys/dev/ath/if_ath_tx.c	Sat Aug  6 03:40:33 2011	(r224668)
+++ user/adrian/if_ath_tx/sys/dev/ath/if_ath_tx.c	Sat Aug  6 04:49:25 2011	(r224669)
@@ -967,10 +967,8 @@ ath_tx_start(struct ath_softc *sc, struc
 	is_ampdu_pending = ath_tx_ampdu_pending(sc, ATH_NODE(ni), tid);
 	is_ampdu = is_ampdu_tx | is_ampdu_pending;
 
-#if 0
 	DPRINTF(sc, ATH_DEBUG_SW_TX, "%s: tid=%d, ac=%d, is_ampdu=%d\n",
 	    __func__, tid, pri, is_ampdu);
-#endif
 
 	/* Multicast frames go onto the software multicast queue */
 	if (ismcast)
@@ -991,13 +989,11 @@ ath_tx_start(struct ath_softc *sc, struc
 	 */
 	bf->bf_state.bfs_seqno = M_SEQNO_GET(m0) << IEEE80211_SEQ_SEQ_SHIFT;
 
-#if 0
 	/* Is ampdu pending? fetch the seqno and print it out */
 	if (is_ampdu_pending)
-		device_printf(sc->sc_dev,
+		DPRINTF(sc, ATH_DEBUG_SW_TX,
 		    "%s: tid %d: ampdu pending, seqno %d\n",
 		    __func__, tid, M_SEQNO_GET(m0));
-#endif
 
 	/* This also sets up the DMA map */
 	r = ath_tx_normal_setup(sc, ni, bf, m0);
@@ -1564,6 +1560,8 @@ ath_tx_tid_seqno_assign(struct ath_softc
 	wh = mtod(m0, struct ieee80211_frame *);
 	pri = M_WME_GETAC(m0);			/* honor classification */
 	tid = WME_AC_TO_TID(pri);
+	DPRINTF(sc, ATH_DEBUG_SW_TX, "%s: pri=%d, tid=%d, qos has seq=%d\n",
+	    __func__, pri, tid, IEEE80211_QOS_HAS_SEQ(wh));
 
 	/* Does the packet require a sequence number? */
 	if (! IEEE80211_QOS_HAS_SEQ(wh))
@@ -1576,6 +1574,7 @@ ath_tx_tid_seqno_assign(struct ath_softc
 	M_SEQNO_SET(m0, seqno);
 
 	/* Return so caller can do something with it if needed */
+	DPRINTF(sc, ATH_DEBUG_SW_TX, "%s:  -> seqno=%d\n", __func__, seqno);
 	return seqno;
 }
 
@@ -1600,11 +1599,34 @@ ath_tx_swq(struct ath_softc *sc, struct 
 	wh = mtod(m0, struct ieee80211_frame *);
 	pri = M_WME_GETAC(m0);			/* honor classification */
 	tid = WME_AC_TO_TID(pri);
+#if 0
+	/*
+	 * XXX This is correct!
+	 *
+	 * It however breaks the rest of the code, which currently assumes
+	 * a constant mapping from TID->WME AC->hardware queue.
+	 * Non-QoS frames will have a TID of 16. They may end up with a TXQ
+	 * that differs to what the classification states?
+	 *
+	 * I'll have to go back over the code and audit exactly what's going
+	 * on before I can flip this on.
+	 *
+	 * With this off, non-QoS traffic in pri=0 gets thorwn into tid=0,
+	 * which means it gets punted to the aggregation code.
+	 * This breaks things.
+	 */
+	if (! IEEE80211_QOS_HAS_SEQ(wh))
+		tid = IEEE80211_NONQOS_TID;
+#endif
 	atid = &an->an_tid[tid];
 
+	DPRINTF(sc, ATH_DEBUG_SW_TX, "%s: pri=%d, tid=%d, qos=%d\n",
+	    __func__, pri, tid, IEEE80211_QOS_HAS_SEQ(wh));
+
 	/* Set local packet state, used to queue packets to hardware */
 	bf->bf_state.bfs_tid = tid;
 	bf->bf_state.bfs_txq = txq;
+	bf->bf_state.bfs_pri = pri;
 
 	/* Queue frame to the tail of the software queue */
 	ATH_TXQ_INSERT_TAIL(atid, bf, bf_list);
@@ -1828,12 +1850,21 @@ ath_tx_tid_hw_queue_aggr(struct ath_soft
 	struct ath_tid *atid = &an->an_tid[tid];
 	struct ieee80211_tx_ampdu *tap;
 
+	DPRINTF(sc, ATH_DEBUG_SW_TX, "%s: tid=%d\n", __func__, tid);
+
+	tap = ath_tx_get_tx_tid(an, tid);
+
 	for (;;) {
                 bf = STAILQ_FIRST(&atid->axq_q);
 		if (bf == NULL) {
 			break;
 		}
-		tap = ath_tx_get_tx_tid(an, tid);
+		if (bf->bf_state.bfs_tid != tid)
+			device_printf(sc->sc_dev, "%s: TID: tid=%d, ac=%d, bf tid=%d\n",
+			    __func__, tid, TID_TO_WME_AC(tid), bf->bf_state.bfs_tid);
+		if (sc->sc_ac2q[TID_TO_WME_AC(tid)] != bf->bf_state.bfs_txq)
+			device_printf(sc->sc_dev, "%s: TXQ: tid=%d, ac=%d, bf tid=%d\n",
+			    __func__, tid, TID_TO_WME_AC(tid), bf->bf_state.bfs_tid);
 
 		/* XXX check if seqno is outside of BAW, if so don't queue it */
 		if (! BAW_WITHIN(tap->txa_start, tap->txa_wnd,
@@ -1998,7 +2029,6 @@ ath_tx_ampdu_pending(struct ath_softc *s
 	return !! (tap->txa_flags & IEEE80211_AGGR_XCHGPEND);
 }
 
-
 /*
  * Is AMPDU-TX pending for the given TID?
  */

Modified: user/adrian/if_ath_tx/sys/dev/ath/if_athvar.h
==============================================================================
--- user/adrian/if_ath_tx/sys/dev/ath/if_athvar.h	Sat Aug  6 03:40:33 2011	(r224668)
+++ user/adrian/if_ath_tx/sys/dev/ath/if_athvar.h	Sat Aug  6 04:49:25 2011	(r224669)
@@ -175,6 +175,7 @@ struct ath_buf {
 		int bfs_seqno;		/* sequence number of this packet */
 		int bfs_retries;	/* retry count */
 		uint16_t bfs_tid;	/* packet TID (or TID_MAX for no QoS) */
+		uint16_t bfs_pri;	/* packet AC priority */
 		struct ath_txq *bfs_txq;	/* eventual dest hardware TXQ */
 		uint16_t bfs_al;	/* length of aggregate */
 		uint16_t bfs_pktdur;	/* packet duration (at current rate?) */



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201108060449.p764nP87035521>