Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 11 Jun 2012 07:50:09 GMT
From:      dfilter@FreeBSD.ORG (dfilter service)
To:        freebsd-wireless@FreeBSD.org
Subject:   Re: kern/166190: commit references a PR
Message-ID:  <201206110750.q5B7o9hS037528@freefall.freebsd.org>

next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/166190; it has been noted by GNATS.

From: dfilter@FreeBSD.ORG (dfilter service)
To: bug-followup@FreeBSD.org
Cc:  
Subject: Re: kern/166190: commit references a PR
Date: Mon, 11 Jun 2012 07:44:32 +0000 (UTC)

 Author: adrian
 Date: Mon Jun 11 07:44:16 2012
 New Revision: 236880
 URL: http://svn.freebsd.org/changeset/base/236880
 
 Log:
   Wrap the whole (software) TX path from ifnet dequeue to software queue
   (or direct dispatch) behind the TXQ lock (which, remember, is doubling
   as the TID lock too for now.)
   
   This ensures that:
   
    (a) the sequence number and the CCMP PN allocation is done together;
    (b) overlapping transmit paths don't interleave frames, so we don't
        end up with the original issue that triggered kern/166190.
   
        Ie, that we don't end up with seqno A, B in thread 1, C, D in
        thread 2, and they being queued to the software queue as "A C D B"
        or similar, leading to the BAW stalls.
   
   This has been tested:
   
   * both STA and AP modes with INVARIANTS and WITNESS;
   * TCP and UDP TX;
   * both STA->AP and AP->STA.
   
   STA is a Routerstation Pro (single CPU MIPS) and the AP is a dual-core
   Centrino.
   
   PR:		kern/166190
 
 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	Mon Jun 11 07:35:24 2012	(r236879)
 +++ head/sys/dev/ath/if_ath_tx.c	Mon Jun 11 07:44:16 2012	(r236880)
 @@ -1171,6 +1171,15 @@ ath_tx_normal_setup(struct ath_softc *sc
  	struct ath_node *an;
  	u_int pri;
  
 +	/*
 +	 * To ensure that both sequence numbers and the CCMP PN handling
 +	 * is "correct", make sure that the relevant TID queue is locked.
 +	 * Otherwise the CCMP PN and seqno may appear out of order, causing
 +	 * re-ordered frames to have out of order CCMP PN's, resulting
 +	 * in many, many frame drops.
 +	 */
 +	ATH_TXQ_LOCK_ASSERT(txq);
 +
  	wh = mtod(m0, struct ieee80211_frame *);
  	iswep = wh->i_fc[1] & IEEE80211_FC1_WEP;
  	ismcast = IEEE80211_IS_MULTICAST(wh->i_addr1);
 @@ -1506,11 +1515,18 @@ ath_tx_start(struct ath_softc *sc, struc
  	/* XXX should just bzero the bf_state? */
  	bf->bf_state.bfs_dobaw = 0;
  
 +	/*
 +	 * Acquire the TXQ lock early, so both the encap and seqno
 +	 * are allocated together.
 +	 */
 +	ATH_TXQ_LOCK(txq);
 +
  	/* A-MPDU TX? Manually set sequence number */
 -	/* Don't do it whilst pending; the net80211 layer still assigns them */
 -	/* XXX do we need locking here? */
 +	/*
 +	 * Don't do it whilst pending; the net80211 layer still
 +	 * assigns them.
 +	 */
  	if (is_ampdu_tx) {
 -		ATH_TXQ_LOCK(txq);
  		/*
  		 * Always call; this function will
  		 * handle making sure that null data frames
 @@ -1526,7 +1542,6 @@ ath_tx_start(struct ath_softc *sc, struc
  		    subtype != IEEE80211_FC0_SUBTYPE_QOS_NULL) {
  			bf->bf_state.bfs_dobaw = 1;
  		}
 -		ATH_TXQ_UNLOCK(txq);
  	}
  
  	/*
 @@ -1545,7 +1560,7 @@ ath_tx_start(struct ath_softc *sc, struc
  	r = ath_tx_normal_setup(sc, ni, bf, m0, txq);
  
  	if (r != 0)
 -		return r;
 +		goto done;
  
  	/* At this point m0 could have changed! */
  	m0 = bf->bf_m;
 @@ -1570,16 +1585,12 @@ ath_tx_start(struct ath_softc *sc, struc
  	if (txq == &avp->av_mcastq) {
  		DPRINTF(sc, ATH_DEBUG_SW_TX,
  		    "%s: bf=%p: mcastq: TX'ing\n", __func__, bf);
 -		ATH_TXQ_LOCK(txq);
  		ath_tx_xmit_normal(sc, txq, bf);
 -		ATH_TXQ_UNLOCK(txq);
  	} else if (type == IEEE80211_FC0_TYPE_CTL &&
  		    subtype == IEEE80211_FC0_SUBTYPE_BAR) {
  		DPRINTF(sc, ATH_DEBUG_SW_TX,
  		    "%s: BAR: TX'ing direct\n", __func__);
 -		ATH_TXQ_LOCK(txq);
  		ath_tx_xmit_normal(sc, txq, bf);
 -		ATH_TXQ_UNLOCK(txq);
  	} else {
  		/* add to software queue */
  		DPRINTF(sc, ATH_DEBUG_SW_TX,
 @@ -1591,10 +1602,10 @@ ath_tx_start(struct ath_softc *sc, struc
  	 * For now, since there's no software queue,
  	 * direct-dispatch to the hardware.
  	 */
 -	ATH_TXQ_LOCK(txq);
  	ath_tx_xmit_normal(sc, txq, bf);
 -	ATH_TXQ_UNLOCK(txq);
  #endif
 +done:
 +	ATH_TXQ_UNLOCK(txq);
  
  	return 0;
  }
 @@ -1630,10 +1641,29 @@ ath_tx_raw_start(struct ath_softc *sc, s
  	/* XXX honor IEEE80211_BPF_DATAPAD */
  	pktlen = m0->m_pkthdr.len - (hdrlen & 3) + IEEE80211_CRC_LEN;
  
 -
  	DPRINTF(sc, ATH_DEBUG_SW_TX, "%s: ismcast=%d\n",
  	    __func__, ismcast);
  
 +	pri = params->ibp_pri & 3;
 +	/* Override pri if the frame isn't a QoS one */
 +	if (! IEEE80211_QOS_HAS_SEQ(wh))
 +		pri = ath_tx_getac(sc, m0);
 +
 +	/* XXX If it's an ADDBA, override the correct queue */
 +	do_override = ath_tx_action_frame_override_queue(sc, ni, m0, &o_tid);
 +
 +	/* Map ADDBA to the correct priority */
 +	if (do_override) {
 +#if 0
 +		device_printf(sc->sc_dev,
 +		    "%s: overriding tid %d pri %d -> %d\n",
 +		    __func__, o_tid, pri, TID_TO_WME_AC(o_tid));
 +#endif
 +		pri = TID_TO_WME_AC(o_tid);
 +	}
 +
 +	ATH_TXQ_LOCK(sc->sc_ac2q[pri]);
 +
  	/* Handle encryption twiddling if needed */
  	if (! ath_tx_tag_crypto(sc, ni,
  	    m0, params->ibp_flags & IEEE80211_BPF_CRYPTO, 0,
 @@ -1688,11 +1718,6 @@ ath_tx_raw_start(struct ath_softc *sc, s
  	if (flags & (HAL_TXDESC_RTSENA|HAL_TXDESC_CTSENA))
  		bf->bf_state.bfs_ctsrate0 = params->ibp_ctsrate;
  
 -	pri = params->ibp_pri & 3;
 -	/* Override pri if the frame isn't a QoS one */
 -	if (! IEEE80211_QOS_HAS_SEQ(wh))
 -		pri = ath_tx_getac(sc, m0);
 -
  	/*
  	 * NB: we mark all packets as type PSPOLL so the h/w won't
  	 * set the sequence number, duration, etc.
 @@ -1774,19 +1799,6 @@ ath_tx_raw_start(struct ath_softc *sc, s
  
  	/* NB: no buffered multicast in power save support */
  
 -	/* XXX If it's an ADDBA, override the correct queue */
 -	do_override = ath_tx_action_frame_override_queue(sc, ni, m0, &o_tid);
 -
 -	/* Map ADDBA to the correct priority */
 -	if (do_override) {
 -#if 0
 -		device_printf(sc->sc_dev,
 -		    "%s: overriding tid %d pri %d -> %d\n",
 -		    __func__, o_tid, pri, TID_TO_WME_AC(o_tid));
 -#endif
 -		pri = TID_TO_WME_AC(o_tid);
 -	}
 -
  	/*
  	 * If we're overiding the ADDBA destination, dump directly
  	 * into the hardware queue, right after any pending
 @@ -1796,13 +1808,12 @@ ath_tx_raw_start(struct ath_softc *sc, s
  	    __func__, do_override);
  
  	if (do_override) {
 -		ATH_TXQ_LOCK(sc->sc_ac2q[pri]);
  		ath_tx_xmit_normal(sc, sc->sc_ac2q[pri], bf);
 -		ATH_TXQ_UNLOCK(sc->sc_ac2q[pri]);
  	} else {
  		/* Queue to software queue */
  		ath_tx_swq(sc, ni, sc->sc_ac2q[pri], bf);
  	}
 +	ATH_TXQ_UNLOCK(sc->sc_ac2q[pri]);
  
  	return 0;
  }
 @@ -2032,6 +2043,15 @@ ath_tx_addto_baw(struct ath_softc *sc, s
  	if (bf->bf_state.bfs_isretried)
  		return;
  
 +	if (! bf->bf_state.bfs_dobaw) {
 +		device_printf(sc->sc_dev,
 +		    "%s: dobaw=0, seqno=%d, window %d:%d\n",
 +		    __func__,
 +		    SEQNO(bf->bf_state.bfs_seqno),
 +		    tap->txa_start,
 +		    tap->txa_wnd);
 +	}
 +
  	tap = ath_tx_get_tx_tid(an, tid->tid);
  
  	if (bf->bf_state.bfs_addedbaw)
 @@ -2043,6 +2063,20 @@ ath_tx_addto_baw(struct ath_softc *sc, s
  		    tid->baw_tail);
  
  	/*
 +	 * Verify that the given sequence number is not outside of the
 +	 * BAW.  Complain loudly if that's the case.
 +	 */
 +	if (! BAW_WITHIN(tap->txa_start, tap->txa_wnd,
 +	    SEQNO(bf->bf_state.bfs_seqno))) {
 +		device_printf(sc->sc_dev,
 +		    "%s: bf=%p: outside of BAW?? tid=%d, seqno %d; window %d:%d; "
 +		    "baw head=%d tail=%d\n",
 +		    __func__, bf, tid->tid, SEQNO(bf->bf_state.bfs_seqno),
 +		    tap->txa_start, tap->txa_wnd, tid->baw_head,
 +		    tid->baw_tail);
 +	}
 +
 +	/*
  	 * ni->ni_txseqs[] is the currently allocated seqno.
  	 * the txa state contains the current baw start.
  	 */
 @@ -2265,6 +2299,8 @@ ath_tx_tid_seqno_assign(struct ath_softc
  	if (! IEEE80211_QOS_HAS_SEQ(wh))
  		return -1;
  
 +	ATH_TID_LOCK_ASSERT(sc, &(ATH_NODE(ni)->an_tid[tid]));
 +
  	/*
  	 * Is it a QOS NULL Data frame? Give it a sequence number from
  	 * the default TID (IEEE80211_NONQOS_TID.)
 @@ -2276,6 +2312,7 @@ ath_tx_tid_seqno_assign(struct ath_softc
  	 */
  	subtype = wh->i_fc[0] & IEEE80211_FC0_SUBTYPE_MASK;
  	if (subtype == IEEE80211_FC0_SUBTYPE_QOS_NULL) {
 +		/* 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 {
 @@ -2369,6 +2406,8 @@ ath_tx_swq(struct ath_softc *sc, struct 
  	int pri, tid;
  	struct mbuf *m0 = bf->bf_m;
  
 +	ATH_TXQ_LOCK_ASSERT(txq);
 +
  	/* Fetch the TID - non-QoS frames get assigned to TID 16 */
  	wh = mtod(m0, struct ieee80211_frame *);
  	pri = ath_tx_getac(sc, m0);
 @@ -2391,7 +2430,6 @@ ath_tx_swq(struct ath_softc *sc, struct 
  	 * If the TID is paused or the traffic it outside BAW, software
  	 * queue it.
  	 */
 -	ATH_TXQ_LOCK(txq);
  	if (atid->paused) {
  		/* TID is paused, queue */
  		DPRINTF(sc, ATH_DEBUG_SW_TX, "%s: paused\n", __func__);
 @@ -2439,7 +2477,6 @@ ath_tx_swq(struct ath_softc *sc, struct 
  		ATH_TXQ_INSERT_TAIL(atid, bf, bf_list);
  		ath_tx_tid_sched(sc, atid);
  	}
 -	ATH_TXQ_UNLOCK(txq);
  }
  
  /*
 _______________________________________________
 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"
 



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