Date: Sun, 19 Mar 2017 05:00:14 +0000 (UTC) From: Adrian Chadd <adrian@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r315531 - head/sys/dev/ath Message-ID: <201703190500.v2J50EVW031044@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: adrian Date: Sun Mar 19 05:00:14 2017 New Revision: 315531 URL: https://svnweb.freebsd.org/changeset/base/315531 Log: [ath] prepare for "correct" group (bcast/mcast) address frame handling and software/hardware queue TID mapping. When I initially did this 11n TX work in days of yonder, my 802.11 standards clue was ... not as finely tuned. One of the things in 802.11-2012 (which I guess technically was after I did this work, but I'm sure it was like this in the previous rev?) is that among other traffic classes, three things are important: * group addressed frames should be default non-QoS, even if they're QoS frames, and * group addressed frames should have a seqno out of a different space than the per-TID QoS one; and because of this * group addressed frames, being non-QoS, should never be in the Block-ACK window for TX. Now, net80211 and now this code cheats by using the non-QOS TID, but ideally we'd introduce a separate seqno space just for multicast/group traffic for TX and RX comparison. Later extensions (eg reliable multicast / multimedia) express what one should do when doing multicast traffic in a TID. Now, technically we /could/ do group traffic as QoS traffic and throw it into a per-TID seqno space, but this definitely introduces ordering issues when you take into account things like CABQ behaviour. (Ie, if some traffic in the TID goes into the CABQ and some doesn't, because it's doing a split of multicast and non-multicast traffic, then you have seqno ordering issues.) So, until someone implements 802.11vv reliable multicast / multimedia extensions, group traffic is non-QoS. Next, software/hardware queue TID mapping. In the past I believed the WME tagging of frames because well, net80211 had a habit of tagging things like management traffic with it. But, then we also map QoS traffic categories to TIDs as well. So, we should obey the TID! But! then it put some management traffic into higher WME categories too, as those frames don't have QoS TIDs. But! It'd do things like put things like QoS action frames into higher WME categories, when they should be kept in-order with the rest of the traffic for that TID. So! Given all of this, the ath(4) driver does overrides to not trust the WME category. I .. am undoing some of this. Now, the TID has a 1:1 mapping to the hardware queue. The TID is the primary source of truth now for all QoS traffic. The WME is only used for non-QoS traffic. This now means that any TID traffic queued should be consistently queued regardless of WME, so things like the "TX finished, do more TX" that is occuring right now for transmit handling should be "better". The consistent {TID, WME} -> hardware queue mapping is important for transmit completion. It's used to schedule more traffic for that particular TID, because that {many TID}:{1 TXQ} mapping in ath_tx_tid_sched() is used for driving completion. Ie, when the hardware queue completes, it'll walk that list of scheduled TIDs attached to that TXQ. The eventual aim is to get ready for some other features around putting some data into other hardware queues (eg for better PS-POLL support, uAPSD, support, correct-er TDMA support, etc) which requires that I tidy all of this up in preparation for then introducing further TID scheduling that isn't linked to a hardware TXQ (likely a per-WME, per-TID driver queue, and a per-node driver queue) to enable that. Tested: * AR9380, STA mode * AR9380, AR9580, AP mode 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 19 04:03:39 2017 (r315530) +++ head/sys/dev/ath/if_ath_tx.c Sun Mar 19 05:00:14 2017 (r315531) @@ -174,21 +174,22 @@ ath_tx_is_11n(struct ath_softc *sc) /* * Obtain the current TID from the given frame. * - * Non-QoS frames need to go into TID 16 (IEEE80211_NONQOS_TID.) - * This has implications for which AC/priority the packet is placed - * in. + * Non-QoS frames get mapped to a TID so frames consistently + * go on a sensible queue. */ static int ath_tx_gettid(struct ath_softc *sc, const struct mbuf *m0) { const struct ieee80211_frame *wh; - int pri = M_WME_GETAC(m0); wh = mtod(m0, const struct ieee80211_frame *); + + /* Non-QoS: map frame to a TID queue for software queueing */ if (! IEEE80211_QOS_HAS_SEQ(wh)) - return IEEE80211_NONQOS_TID; - else - return WME_AC_TO_TID(pri); + return (WME_AC_TO_TID(M_WME_GETAC(m0))); + + /* QoS - fetch the TID from the header, ignore mbuf WME */ + return (ieee80211_gettid(wh)); } static void @@ -211,30 +212,42 @@ ath_tx_set_retry(struct ath_softc *sc, s * Determine what the correct AC queue for the given frame * should be. * - * This code assumes that the TIDs map consistently to - * the underlying hardware (or software) ath_txq. - * Since the sender may try to set an AC which is - * arbitrary, non-QoS TIDs may end up being put on - * completely different ACs. There's no way to put a - * TID into multiple ath_txq's for scheduling, so - * for now we override the AC/TXQ selection and set - * non-QOS TID frames into the BE queue. - * - * This may be completely incorrect - specifically, - * some management frames may end up out of order - * compared to the QoS traffic they're controlling. - * I'll look into this later. + * For QoS frames, obey the TID. That way things like + * management frames that are related to a given TID + * are thus serialised with the rest of the TID traffic, + * regardless of net80211 overriding priority. + * + * For non-QoS frames, return the mbuf WMI priority. + * + * This has implications that higher priority non-QoS traffic + * may end up being scheduled before other non-QoS traffic, + * leading to out-of-sequence packets being emitted. + * + * (It'd be nice to log/count this so we can see if it + * really is a problem.) + * + * TODO: maybe we should throw multicast traffic, QoS or + * otherwise, into a separate TX queue? */ static int ath_tx_getac(struct ath_softc *sc, const struct mbuf *m0) { const struct ieee80211_frame *wh; - int pri = M_WME_GETAC(m0); + wh = mtod(m0, const struct ieee80211_frame *); + + /* + * QoS data frame (sequence number or otherwise) - + * return hardware queue mapping for the underlying + * TID. + */ if (IEEE80211_QOS_HAS_SEQ(wh)) - return pri; + return TID_TO_WME_AC(ieee80211_gettid(wh)); - return ATH_NONQOS_TID_AC; + /* + * Otherwise - return mbuf QoS pri. + */ + return (M_WME_GETAC(m0)); } void @@ -1550,6 +1563,8 @@ ath_tx_normal_setup(struct ath_softc *sc const HAL_RATE_TABLE *rt; HAL_BOOL shortPreamble; struct ath_node *an; + + /* XXX TODO: this pri is only used for non-QoS check, right? */ u_int pri; /* @@ -1618,7 +1633,8 @@ ath_tx_normal_setup(struct ath_softc *sc //flags = HAL_TXDESC_CLRDMASK; /* XXX needed for crypto errs */ flags = 0; ismrr = 0; /* default no multi-rate retry*/ - pri = M_WME_GETAC(m0); /* honor classification */ + + pri = ath_tx_getac(sc, m0); /* honor classification */ /* XXX use txparams instead of fixed values */ /* * Calculate Atheros packet type from IEEE80211 packet header, @@ -1899,7 +1915,18 @@ ath_tx_start(struct ath_softc *sc, struc * Determine the target hardware queue. * * For multicast frames, the txq gets overridden appropriately - * depending upon the state of PS. + * depending upon the state of PS. If powersave is enabled + * then they get added to the cabq for later transmit. + * + * The "fun" issue here is that group addressed frames should + * have the sequence number from a different pool, rather than + * the per-TID pool. That means that even QoS group addressed + * frames will have a sequence number from that global value, + * which means if we transmit different group addressed frames + * at different traffic priorities, the sequence numbers will + * all be out of whack. So - chances are, the right thing + * to do here is to always put group addressed frames into the BE + * queue, and ignore the TID for queue selection. * * For any other frame, we do a TID/QoS lookup inside the frame * to see what the TID should be. If it's a non-QoS frame, the @@ -1999,21 +2026,26 @@ ath_tx_start(struct ath_softc *sc, struc /* * Don't do it whilst pending; the net80211 layer still * assigns them. + * + * Don't assign A-MPDU sequence numbers to group address + * frames; they come from a different sequence number space. */ - if (is_ampdu_tx) { + if (is_ampdu_tx && (! IEEE80211_IS_MULTICAST(wh->i_addr1))) { /* * Always call; this function will * handle making sure that null data frames - * don't get a sequence number from the current - * TID and thus mess with the BAW. + * and group-addressed frames don't get a sequence number + * from the current TID and thus mess with the BAW. */ seqno = ath_tx_tid_seqno_assign(sc, ni, bf, m0); /* - * Don't add QoS NULL frames to the BAW. + * Don't add QoS NULL frames and group-addressed frames + * to the BAW. */ if (IEEE80211_QOS_HAS_SEQ(wh) && - subtype != IEEE80211_FC0_SUBTYPE_QOS_NULL) { + (! IEEE80211_IS_MULTICAST(wh->i_addr1)) && + (subtype != IEEE80211_FC0_SUBTYPE_QOS_NULL)) { bf->bf_state.bfs_dobaw = 1; } } @@ -2030,7 +2062,7 @@ ath_tx_start(struct ath_softc *sc, struc "%s: tid %d: ampdu pending, seqno %d\n", __func__, tid, M_SEQNO_GET(m0)); - /* This also sets up the DMA map */ + /* This also sets up the DMA map; crypto; frame parameters, etc */ r = ath_tx_normal_setup(sc, ni, bf, m0, txq); if (r != 0) @@ -2148,7 +2180,7 @@ ath_tx_raw_start(struct ath_softc *sc, s /* Map ADDBA to the correct priority */ if (do_override) { -#if 0 +#if 1 DPRINTF(sc, ATH_DEBUG_XMIT, "%s: overriding tid %d pri %d -> %d\n", __func__, o_tid, pri, TID_TO_WME_AC(o_tid)); @@ -2156,6 +2188,14 @@ ath_tx_raw_start(struct ath_softc *sc, s pri = TID_TO_WME_AC(o_tid); } + /* + * "pri" is the hardware queue to transmit on. + * + * Look at the description in ath_tx_start() to understand + * what needs to be "fixed" here so we just use the TID + * for QoS frames. + */ + /* Handle encryption twiddling if needed */ if (! ath_tx_tag_crypto(sc, ni, m0, params->ibp_flags & IEEE80211_BPF_CRYPTO, 0, @@ -2931,22 +2971,25 @@ ath_tx_tid_unsched(struct ath_softc *sc, * Assign a sequence number manually to the given frame. * * This should only be called for A-MPDU TX frames. + * + * Note: for group addressed frames, the sequence number + * should be from NONQOS_TID, and net80211 should have + * already assigned it for us. */ static ieee80211_seq ath_tx_tid_seqno_assign(struct ath_softc *sc, struct ieee80211_node *ni, struct ath_buf *bf, struct mbuf *m0) { struct ieee80211_frame *wh; - int tid, pri; + int tid; ieee80211_seq seqno; uint8_t subtype; - /* TID lookup */ 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)); + tid = ieee80211_gettid(wh); + + DPRINTF(sc, ATH_DEBUG_SW_TX, "%s: tid=%d, qos has seq=%d\n", + __func__, tid, IEEE80211_QOS_HAS_SEQ(wh)); /* XXX Is it a control frame? Ignore */ @@ -2970,6 +3013,13 @@ ath_tx_tid_seqno_assign(struct ath_softc /* XXX no locking for this TID? This is a bit of a problem. */ seqno = ni->ni_txseqs[IEEE80211_NONQOS_TID]; INCR(ni->ni_txseqs[IEEE80211_NONQOS_TID], IEEE80211_SEQ_RANGE); + } else if (IEEE80211_IS_MULTICAST(wh->i_addr1)) { + /* + * group addressed frames get a sequence number from + * a different sequence number space. + */ + seqno = ni->ni_txseqs[IEEE80211_NONQOS_TID]; + INCR(ni->ni_txseqs[IEEE80211_NONQOS_TID], IEEE80211_SEQ_RANGE); } else { /* Manually assign sequence number */ seqno = ni->ni_txseqs[tid];
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201703190500.v2J50EVW031044>