From owner-svn-src-all@FreeBSD.ORG Tue Nov 8 18:10:11 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D4F76106566C; Tue, 8 Nov 2011 18:10:11 +0000 (UTC) (envelope-from adrian@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:4f8:fff6::2c]) by mx1.freebsd.org (Postfix) with ESMTP id B9A328FC14; Tue, 8 Nov 2011 18:10:11 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.4/8.14.4) with ESMTP id pA8IA4Qg042494; Tue, 8 Nov 2011 18:10:04 GMT (envelope-from adrian@svn.freebsd.org) Received: (from adrian@localhost) by svn.freebsd.org (8.14.4/8.14.4/Submit) id pA8IA4ZX042490; Tue, 8 Nov 2011 18:10:04 GMT (envelope-from adrian@svn.freebsd.org) Message-Id: <201111081810.pA8IA4ZX042490@svn.freebsd.org> From: Adrian Chadd Date: Tue, 8 Nov 2011 18:10:04 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r227346 - head/sys/dev/ath X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Nov 2011 18:10:11 -0000 Author: adrian Date: Tue Nov 8 18:10:04 2011 New Revision: 227346 URL: http://svn.freebsd.org/changeset/base/227346 Log: Merge in some fixes from the if_ath_tx branch. * Close down some of the kickpcu races, where the interrupt handler can and will run concurrently with the taskqueue. * Close down the TXQ active/completed race between the interrupt handler and the concurrently running tx completion taskqueue function. * Add some tx and rx interrupt count tracking, for debugging. * Fix the kickpcu logic in ath_rx_proc() to not simply drain and restart the TX queue - instead, assume the hardware isn't (too) confused and just restart RX DMA. This may break on previous chipsets, so if it does I'll add a HAL flag and conditionally handle this (ie, for broken chipsets, I'll just restore the "stop PCU / flush things / restart PCU" logic.) * Misc stuff Sponsored by: Hobnob, Inc. Modified: head/sys/dev/ath/if_ath.c head/sys/dev/ath/if_ath_tx.h head/sys/dev/ath/if_athvar.h Modified: head/sys/dev/ath/if_ath.c ============================================================================== --- head/sys/dev/ath/if_ath.c Tue Nov 8 17:23:43 2011 (r227345) +++ head/sys/dev/ath/if_ath.c Tue Nov 8 18:10:04 2011 (r227346) @@ -1304,6 +1304,7 @@ ath_intr(void *arg) struct ifnet *ifp = sc->sc_ifp; struct ath_hal *ah = sc->sc_ah; HAL_INT status = 0; + uint32_t txqs; if (sc->sc_invalid) { /* @@ -1377,7 +1378,7 @@ ath_intr(void *arg) } } if (status & HAL_INT_RXEOL) { - int imask = sc->sc_imask; + int imask; /* * NB: the hardware should re-read the link when * RXE bit is written, but it doesn't work at @@ -1393,26 +1394,55 @@ ath_intr(void *arg) * by a call to ath_reset() somehow, the * interrupt mask will be correctly reprogrammed. */ + ATH_LOCK(sc); + imask = sc->sc_imask; imask &= ~(HAL_INT_RXEOL | HAL_INT_RXORN); ath_hal_intrset(ah, imask); /* + * Only blank sc_rxlink if we've not yet kicked + * the PCU. + * + * This isn't entirely correct - the correct solution + * would be to have a PCU lock and engage that for + * the duration of the PCU fiddling; which would include + * running the RX process. Otherwise we could end up + * messing up the RX descriptor chain and making the + * RX desc list much shorter. + */ + if (! sc->sc_kickpcu) + sc->sc_rxlink = NULL; + sc->sc_kickpcu = 1; + ATH_UNLOCK(sc); + /* * Enqueue an RX proc, to handled whatever * is in the RX queue. * This will then kick the PCU. */ taskqueue_enqueue(sc->sc_tq, &sc->sc_rxtask); - sc->sc_rxlink = NULL; - sc->sc_kickpcu = 1; } if (status & HAL_INT_TXURN) { sc->sc_stats.ast_txurn++; /* bump tx trigger level */ ath_hal_updatetxtriglevel(ah, AH_TRUE); } - if (status & HAL_INT_RX) + if (status & HAL_INT_RX) { + sc->sc_stats.ast_rx_intr++; taskqueue_enqueue(sc->sc_tq, &sc->sc_rxtask); - if (status & HAL_INT_TX) + } + if (status & HAL_INT_TX) { + sc->sc_stats.ast_tx_intr++; + /* + * Grab all the currently set bits in the HAL txq bitmap + * and blank them. This is the only place we should be + * doing this. + */ + ATH_LOCK(sc); + txqs = 0xffffffff; + ath_hal_gettxintrtxqs(sc->sc_ah, &txqs); + sc->sc_txq_active |= txqs; + ATH_UNLOCK(sc); taskqueue_enqueue(sc->sc_tq, &sc->sc_txtask); + } if (status & HAL_INT_BMISS) { sc->sc_stats.ast_bmiss++; taskqueue_enqueue(sc->sc_tq, &sc->sc_bmisstask); @@ -1433,7 +1463,16 @@ ath_intr(void *arg) * clear whatever condition caused the interrupt. */ ath_hal_mibevent(ah, &sc->sc_halstats); - ath_hal_intrset(ah, sc->sc_imask); + /* + * Don't reset the interrupt if we've just + * kicked the PCU, or we may get a nested + * RXEOL before the rxproc has had a chance + * to run. + */ + ATH_LOCK(sc); + if (sc->sc_kickpcu == 0) + ath_hal_intrset(ah, sc->sc_imask); + ATH_UNLOCK(sc); } if (status & HAL_INT_RXORN) { /* NB: hal marks HAL_INT_FATAL when RXORN is fatal */ @@ -1609,6 +1648,13 @@ ath_init(void *arg) sc->sc_beacons = 0; /* + * Initial aggregation settings. + */ + sc->sc_hwq_limit = ATH_AGGR_MIN_QDEPTH; + sc->sc_tid_hwq_lo = ATH_AGGR_SCHED_LOW; + sc->sc_tid_hwq_hi = ATH_AGGR_SCHED_HIGH; + + /* * Setup the hardware after reset: the key cache * is filled as needed and the receive engine is * set going. Frame transmit is handled entirely @@ -3478,6 +3524,7 @@ ath_rx_proc(void *arg, int npending) HAL_STATUS status; int16_t nf; u_int64_t tsf; + int npkts = 0; DPRINTF(sc, ATH_DEBUG_RX_PROC, "%s: pending %u\n", __func__, npending); ngood = 0; @@ -3537,6 +3584,7 @@ ath_rx_proc(void *arg, int npending) break; TAILQ_REMOVE(&sc->sc_rxbuf, bf, bf_list); + npkts++; /* These aren't specifically errors */ if (rs->rs_flags & HAL_RX_GI) @@ -3823,17 +3871,20 @@ rx_next: * been an RXEOL condition. */ if (sc->sc_kickpcu) { - sc->sc_kickpcu = 0; - ath_stoprecv(sc); - sc->sc_imask |= (HAL_INT_RXEOL | HAL_INT_RXORN); - if (ath_startrecv(sc) != 0) { - if_printf(ifp, - "%s: couldn't restart RX after RXEOL; resetting\n", - __func__); - ath_reset(ifp); - return; - } + device_printf(sc->sc_dev, "%s: kickpcu; handled %d packets\n", + __func__, npkts); + + /* XXX rxslink? */ + bf = TAILQ_FIRST(&sc->sc_rxbuf); + ath_hal_putrxbuf(ah, bf->bf_daddr); + ath_hal_rxena(ah); /* enable recv descriptors */ + ath_mode_init(sc); /* set filters, etc. */ + ath_hal_startpcurecv(ah); /* re-enable PCU/DMA engine */ + + ATH_LOCK(sc); ath_hal_intrset(ah, sc->sc_imask); + sc->sc_kickpcu = 0; + ATH_UNLOCK(sc); } if ((ifp->if_drv_flags & IFF_DRV_OACTIVE) == 0) { @@ -4218,13 +4269,7 @@ ath_tx_processq(struct ath_softc *sc, st return nacked; } -static __inline int -txqactive(struct ath_hal *ah, int qnum) -{ - u_int32_t txqs = 1<sc_ifp; + uint32_t txqs; - if (txqactive(sc->sc_ah, 0) && ath_tx_processq(sc, &sc->sc_txq[0])) + ATH_LOCK(sc); + txqs = sc->sc_txq_active; + sc->sc_txq_active &= ~txqs; + ATH_UNLOCK(sc); + + if (TXQACTIVE(txqs, 0) && ath_tx_processq(sc, &sc->sc_txq[0])) + /* XXX why is lastrx updated in tx code? */ sc->sc_lastrx = ath_hal_gettsf64(sc->sc_ah); - if (txqactive(sc->sc_ah, sc->sc_cabq->axq_qnum)) + if (TXQACTIVE(txqs, sc->sc_cabq->axq_qnum)) ath_tx_processq(sc, sc->sc_cabq); ifp->if_drv_flags &= ~IFF_DRV_OACTIVE; sc->sc_wd_timer = 0; @@ -4259,20 +4311,26 @@ ath_tx_proc_q0123(void *arg, int npendin struct ath_softc *sc = arg; struct ifnet *ifp = sc->sc_ifp; int nacked; + uint32_t txqs; + + ATH_LOCK(sc); + txqs = sc->sc_txq_active; + sc->sc_txq_active &= ~txqs; + ATH_UNLOCK(sc); /* * Process each active queue. */ nacked = 0; - if (txqactive(sc->sc_ah, 0)) + if (TXQACTIVE(txqs, 0)) nacked += ath_tx_processq(sc, &sc->sc_txq[0]); - if (txqactive(sc->sc_ah, 1)) + if (TXQACTIVE(txqs, 1)) nacked += ath_tx_processq(sc, &sc->sc_txq[1]); - if (txqactive(sc->sc_ah, 2)) + if (TXQACTIVE(txqs, 2)) nacked += ath_tx_processq(sc, &sc->sc_txq[2]); - if (txqactive(sc->sc_ah, 3)) + if (TXQACTIVE(txqs, 3)) nacked += ath_tx_processq(sc, &sc->sc_txq[3]); - if (txqactive(sc->sc_ah, sc->sc_cabq->axq_qnum)) + if (TXQACTIVE(txqs, sc->sc_cabq->axq_qnum)) ath_tx_processq(sc, sc->sc_cabq); if (nacked) sc->sc_lastrx = ath_hal_gettsf64(sc->sc_ah); @@ -4295,13 +4353,19 @@ ath_tx_proc(void *arg, int npending) struct ath_softc *sc = arg; struct ifnet *ifp = sc->sc_ifp; int i, nacked; + uint32_t txqs; + + ATH_LOCK(sc); + txqs = sc->sc_txq_active; + sc->sc_txq_active &= ~txqs; + ATH_UNLOCK(sc); /* * Process each active queue. */ nacked = 0; for (i = 0; i < HAL_NUM_TX_QUEUES; i++) - if (ATH_TXQ_SETUP(sc, i) && txqactive(sc->sc_ah, i)) + if (ATH_TXQ_SETUP(sc, i) && TXQACTIVE(txqs, i)) nacked += ath_tx_processq(sc, &sc->sc_txq[i]); if (nacked) sc->sc_lastrx = ath_hal_gettsf64(sc->sc_ah); Modified: head/sys/dev/ath/if_ath_tx.h ============================================================================== --- head/sys/dev/ath/if_ath_tx.h Tue Nov 8 17:23:43 2011 (r227345) +++ head/sys/dev/ath/if_ath_tx.h Tue Nov 8 18:10:04 2011 (r227346) @@ -31,6 +31,24 @@ #ifndef __IF_ATH_TX_H__ #define __IF_ATH_TX_H__ +/* + * How 'busy' to try and keep the hardware txq + */ +#define ATH_AGGR_MIN_QDEPTH 2 + +/* + * Watermark for scheduling TIDs in order to maximise aggregation. + * + * If hwq_depth is greater than this, don't schedule the TID + * for packet scheduling - the hardware is already busy servicing + * this TID. + * + * If hwq_depth is less than this, schedule the TID for packet + * scheduling in the completion handler. + */ +#define ATH_AGGR_SCHED_HIGH 4 +#define ATH_AGGR_SCHED_LOW 2 + extern void ath_freetx(struct mbuf *m); extern void ath_txfrag_cleanup(struct ath_softc *sc, ath_bufhead *frags, struct ieee80211_node *ni); Modified: head/sys/dev/ath/if_athvar.h ============================================================================== --- head/sys/dev/ath/if_athvar.h Tue Nov 8 17:23:43 2011 (r227345) +++ head/sys/dev/ath/if_athvar.h Tue Nov 8 18:10:04 2011 (r227346) @@ -312,7 +312,6 @@ struct ath_txq { TAILQ_REMOVE(&(_tq)->axq_q, _elm, _field); \ (_tq)->axq_depth--; \ } while (0) -/* NB: this does not do the "head empty check" that STAILQ_LAST does */ #define ATH_TXQ_LAST(_tq, _field) TAILQ_LAST(&(_tq)->axq_q, _field) struct ath_vap { @@ -397,7 +396,6 @@ struct ath_softc { sc_setcca : 1,/* set/clr CCA with TDMA */ sc_resetcal : 1,/* reset cal state next trip */ sc_rxslink : 1,/* do self-linked final descriptor */ - sc_kickpcu : 1,/* kick PCU RX on next RX proc */ sc_rxtsf32 : 1;/* RX dec TSF is 32 bits */ uint32_t sc_eerd; /* regdomain from EEPROM */ uint32_t sc_eecc; /* country code from EEPROM */ @@ -424,7 +422,19 @@ struct ath_softc { u_int sc_fftxqmin; /* min frames before staging */ u_int sc_fftxqmax; /* max frames before drop */ u_int sc_txantenna; /* tx antenna (fixed or auto) */ + HAL_INT sc_imask; /* interrupt mask copy */ + /* + * These are modified in the interrupt handler as well as + * the task queues and other contexts. Thus these must be + * protected by a mutex, or they could clash. + * + * For now, access to these is behind the ATH_LOCK, + * just to save time. + */ + uint32_t sc_txq_active; /* bitmap of active TXQs */ + uint32_t sc_kickpcu; /* whether to kick the PCU */ + u_int sc_keymax; /* size of key cache */ u_int8_t sc_keymap[ATH_KEYBYTES];/* key use bit map */