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>