Date: Sun, 29 Jan 2017 19:15:47 -0800 From: Adrian Chadd <adrian@freebsd.org> To: "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r312972 - head/sys/net80211 Message-ID: <CAJ-Vmo=5cVUS%2BuKEgHb87Ftt2G39LiV90cmXX-UJ1cOX05ZY5w@mail.gmail.com> In-Reply-To: <201701300111.v0U1BV6L081269@repo.freebsd.org> References: <201701300111.v0U1BV6L081269@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
... and obviously between writing the initial review message and the commit, I also decided to disable multicast QoS frame generation for now. It just makes things less terrible. At some point IBSS/hostap mode should grow "how many stations are QoS/non-QoS" tracking so this can be dynamic and multicast QoS can occur. (ANd for whenever I implement 802.11aa extensions, I'll need it to work..) -adrian On 29 January 2017 at 17:11, Adrian Chadd <adrian@freebsd.org> wrote: > Author: adrian > Date: Mon Jan 30 01:11:30 2017 > New Revision: 312972 > URL: https://svnweb.freebsd.org/changeset/base/312972 > > Log: > [net80211] address seqno allocation for group addressed frames > > After some digging and looking at packet traces, it looks like the > sequence number allocation being done by net80211 doesn't meet > 802.11-2012. > > Specifically, group addressed frames (broadcast, multicast) have > sequence numbers allocated from a separate pool, even if they're > QoS frames. > > This patch starts to try and address this, both on transmit and > receive. > > * When receiving, don't throw away multicast frames for now. > It's sub-optimal, but until we correctly track group addressed > frames via another TID counter, this is the best we can do. > > * When doing A-MPDU checks, don't include group addressed frames > in the sequence number checks. > > * When transmitting, don't allocate group frame sequence numbers > from the TID, instead use the NONQOS TID for allocation. > > This may fix iwn(4) 11n because I /think/ this was one of the > handful of places where ni_txseqs[] was being assigned /outside/ > of the driver itself. > > This however doesn't completely fix things - notably the way that > TID assignment versus WME assignment for driver hardware queues > will mess up multicast ordering. For example, if all multicast > QoS frames come from one sequence number space but they're > expected to obey the QoS value assigned, they'll end up in > different queues in the hardware and go out in different > orders. > > I can't fix that right now and indeed fixing it will require some > pretty heavy lifting of both the WME<->TID QoS assignment, as well > as figuring out what the correct way for drivers to behave. > > For example, both iwn(4) and ath(4) shouldn't put QoS multicast > traffic into the same output queue as aggregate traffic, because > the sequence numbers are all wrong. So perhaps the correct thing > to do there is ignore the WME/TID for QoS traffic and map it all > to the best effort queue or something, and ensure it doesn't > muck up the TID/blockack window tracking. However, I'm /pretty/ > sure that is still going to happen. > > .. maybe I should disable multicast QoS frames in general as well, > but I don't know what that'll do for whatever the current state > of 802.11s mesh support is. > > Tested: > > * STA mode, ath10k NIC > * AP mode, AR9344/AR9580 AP > * iperf tcp/udp tests with concurrent multicast QoS traffic. > > Before this, iperfs would fail pretty quickly because the sending > AP would start sending out QoS multicast frames that would be > out of order from the rest of the TID traffic, causing the blockack > window to get way, way out of sync. > > This now doesn't occur. > > TODO: > > * verify which QoS frames SHOULD be tagged as M_AMPDU_MPDU. > For example, QoS NULL frames shouldn't be tagged! > > Reviewed by: avos > Differential Revision: https://reviews.freebsd.org/D9357 > > Modified: > head/sys/net80211/ieee80211_ht.c > head/sys/net80211/ieee80211_input.h > head/sys/net80211/ieee80211_output.c > > Modified: head/sys/net80211/ieee80211_ht.c > ============================================================================== > --- head/sys/net80211/ieee80211_ht.c Sun Jan 29 22:38:13 2017 (r312971) > +++ head/sys/net80211/ieee80211_ht.c Mon Jan 30 01:11:30 2017 (r312972) > @@ -827,6 +827,16 @@ ieee80211_ampdu_reorder(struct ieee80211 > */ > return PROCESS; > } > + > + /* > + * 802.11-2012 9.3.2.10 - Duplicate detection and recovery. > + * > + * Multicast QoS data frames are checked against a different > + * counter, not the per-TID counter. > + */ > + if (IEEE80211_IS_MULTICAST(wh->i_addr1)) > + return PROCESS; > + > if (IEEE80211_IS_DSTODS(wh)) > tid = ((struct ieee80211_qosframe_addr4 *)wh)->i_qos[0]; > else > > Modified: head/sys/net80211/ieee80211_input.h > ============================================================================== > --- head/sys/net80211/ieee80211_input.h Sun Jan 29 22:38:13 2017 (r312971) > +++ head/sys/net80211/ieee80211_input.h Mon Jan 30 01:11:30 2017 (r312972) > @@ -149,6 +149,12 @@ ishtinfooui(const uint8_t *frm) > * (as the seqnum wraps), handle that special case so packets aren't > * incorrectly dropped - ie, if the next packet is sequence number 0 > * but a retransmit since the initial packet didn't make it. > + * > + * XXX TODO: handle sequence number space wrapping with dropped frames; > + * especially in high interference conditions under high traffic load > + * The RX AMPDU reorder code also needs it. > + * > + * XXX TODO: update for 802.11-2012 9.3.2.10 Duplicate Detection and Recovery. > */ > static __inline int > ieee80211_check_rxseq(struct ieee80211_node *ni, struct ieee80211_frame *wh, > @@ -175,6 +181,13 @@ ieee80211_check_rxseq(struct ieee80211_n > if (! IEEE80211_HAS_SEQ(type, subtype)) > return 1; > > + /* > + * Always allow multicast frames for now - QoS (any TID) > + * or not. > + */ > + if (IEEE80211_IS_MULTICAST(wh->i_addr1)) > + return 1; > + > tid = ieee80211_gettid(wh); > > /* > > Modified: head/sys/net80211/ieee80211_output.c > ============================================================================== > --- head/sys/net80211/ieee80211_output.c Sun Jan 29 22:38:13 2017 (r312971) > +++ head/sys/net80211/ieee80211_output.c Mon Jan 30 01:11:30 2017 (r312972) > @@ -122,9 +122,7 @@ ieee80211_vap_pkt_send_dest(struct ieee8 > { > struct ieee80211com *ic = vap->iv_ic; > struct ifnet *ifp = vap->iv_ifp; > -#ifdef IEEE80211_SUPPORT_SUPERG > int mcast; > -#endif > > if ((ni->ni_flags & IEEE80211_NODE_PWR_MGT) && > (m->m_flags & M_PWR_SAV) == 0) { > @@ -164,9 +162,7 @@ ieee80211_vap_pkt_send_dest(struct ieee8 > * interface it (might have been) received on. > */ > m->m_pkthdr.rcvif = (void *)ni; > -#ifdef IEEE80211_SUPPORT_SUPERG > mcast = (m->m_flags & (M_MCAST | M_BCAST)) ? 1: 0; > -#endif > > BPF_MTAP(ifp, m); /* 802.3 tx */ > > @@ -181,10 +177,15 @@ ieee80211_vap_pkt_send_dest(struct ieee8 > * The default ic_ampdu_enable routine handles staggering > * ADDBA requests in case the receiver NAK's us or we are > * otherwise unable to establish a BA stream. > + * > + * Don't treat group-addressed frames as candidates for aggregation; > + * net80211 doesn't support 802.11aa-2012 and so group addressed > + * frames will always have sequence numbers allocated from the NON_QOS > + * TID. > */ > if ((ni->ni_flags & IEEE80211_NODE_AMPDU_TX) && > (vap->iv_flags_ht & IEEE80211_FHT_AMPDU_TX)) { > - if ((m->m_flags & M_EAPOL) == 0) { > + if ((m->m_flags & M_EAPOL) == 0 && (! mcast)) { > int tid = WME_AC_TO_TID(M_WME_GETAC(m)); > struct ieee80211_tx_ampdu *tap = &ni->ni_tx_ampdu[tid]; > > @@ -776,12 +777,20 @@ ieee80211_send_setup( > * requiring the TX lock. > */ > tap = &ni->ni_tx_ampdu[tid]; > - if (tid != IEEE80211_NONQOS_TID && IEEE80211_AMPDU_RUNNING(tap)) > + if (tid != IEEE80211_NONQOS_TID && IEEE80211_AMPDU_RUNNING(tap)) { > m->m_flags |= M_AMPDU_MPDU; > - else { > + } else { > if (IEEE80211_HAS_SEQ(type & IEEE80211_FC0_TYPE_MASK, > type & IEEE80211_FC0_SUBTYPE_MASK)) > - seqno = ni->ni_txseqs[tid]++; > + /* > + * 802.11-2012 9.3.2.10 - QoS multicast frames > + * come out of a different seqno space. > + */ > + if (IEEE80211_IS_MULTICAST(wh->i_addr1)) { > + seqno = ni->ni_txseqs[IEEE80211_NONQOS_TID]++; > + } else { > + seqno = ni->ni_txseqs[tid]++; > + } > else > seqno = 0; > > @@ -1239,7 +1248,7 @@ ieee80211_encap(struct ieee80211vap *vap > struct ieee80211_frame *wh; > struct ieee80211_key *key; > struct llc *llc; > - int hdrsize, hdrspace, datalen, addqos, txfrag, is4addr; > + int hdrsize, hdrspace, datalen, addqos, txfrag, is4addr, is_mcast; > ieee80211_seq seqno; > int meshhdrsize, meshae; > uint8_t *qos; > @@ -1247,6 +1256,8 @@ ieee80211_encap(struct ieee80211vap *vap > > IEEE80211_TX_LOCK_ASSERT(ic); > > + is_mcast = !! (m->m_flags & (M_MCAST | M_BCAST)); > + > /* > * Copy existing Ethernet header to a safe place. The > * rest of the code assumes it's ok to strip it when > @@ -1291,11 +1302,19 @@ ieee80211_encap(struct ieee80211vap *vap > * ap's require all data frames to be QoS-encapsulated > * once negotiated in which case we'll need to make this > * configurable. > - * NB: mesh data frames are QoS. > + * > + * Don't send multicast QoS frames. > + * Technically multicast frames can be QoS if all stations in the > + * BSS are also QoS. > + * > + * NB: mesh data frames are QoS, including multicast frames. > */ > - addqos = ((ni->ni_flags & (IEEE80211_NODE_QOS|IEEE80211_NODE_HT)) || > + addqos = > + (((is_mcast == 0) && (ni->ni_flags & > + (IEEE80211_NODE_QOS|IEEE80211_NODE_HT))) || > (vap->iv_opmode == IEEE80211_M_MBSS)) && > (m->m_flags & M_EAPOL) == 0; > + > if (addqos) > hdrsize = sizeof(struct ieee80211_qosframe); > else > @@ -1560,6 +1579,22 @@ ieee80211_encap(struct ieee80211vap *vap > */ > if ((m->m_flags & M_AMPDU_MPDU) == 0) { > /* > + * 802.11-2012 9.3.2.10 - > + * > + * If this is a multicast frame then we need > + * to ensure that the sequence number comes from > + * a separate seqno space and not the TID space. > + * > + * Otherwise multicast frames may actually cause > + * holes in the TX blockack window space and > + * upset various things. > + */ > + if (IEEE80211_IS_MULTICAST(wh->i_addr1)) > + seqno = ni->ni_txseqs[IEEE80211_NONQOS_TID]++; > + else > + seqno = ni->ni_txseqs[tid]++; > + > + /* > * NB: don't assign a sequence # to potential > * aggregates; we expect this happens at the > * point the frame comes off any aggregation q >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-Vmo=5cVUS%2BuKEgHb87Ftt2G39LiV90cmXX-UJ1cOX05ZY5w>