Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 31 Oct 2012 06:27:59 +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: r242391 - head/sys/dev/ath
Message-ID:  <201210310627.q9V6Rx3p067218@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: adrian
Date: Wed Oct 31 06:27:58 2012
New Revision: 242391
URL: http://svn.freebsd.org/changeset/base/242391

Log:
  I give up - introduce a TX lock to serialise TX operations.
  
  I've tried serialising TX using queues and such but unfortunately
  due to how this interacts with the locking going on elsewhere in the
  networking stack, the TX task gets delayed, resulting in quite a
  noticable throughput loss:
  
  * baseline TCP for 2x2 11n HT40 is ~ 170mbit/sec;
  * TCP for TX task in the ath taskq, with the RX also going on - 80mbit/sec;
  * TCP for TX task in a separate, second taskq - 100mbit/sec.
  
  So for now I'm going with the Linux wireless stack approach - lock tx
  early.  The linux code does in the wireless stack, before the 802.11
  state stuff happens and before it's punted to the driver.
  But TX locking needs to also occur at the driver layer as the TX
  completion code _also_ begins to drain the ifnet TX queue.
  
  Whilst I'm here, add some KTR traces for the TX path.
  
  Note:
  
  * This really should be done at the net80211 layer (as well, at least.)
    But that'll have to wait for a little more thought to happen.

Modified:
  head/sys/dev/ath/if_ath.c
  head/sys/dev/ath/if_ath_ahb.c
  head/sys/dev/ath/if_ath_misc.h
  head/sys/dev/ath/if_ath_pci.c
  head/sys/dev/ath/if_ath_tx.c
  head/sys/dev/ath/if_athvar.h

Modified: head/sys/dev/ath/if_ath.c
==============================================================================
--- head/sys/dev/ath/if_ath.c	Wed Oct 31 04:44:32 2012	(r242390)
+++ head/sys/dev/ath/if_ath.c	Wed Oct 31 06:27:58 2012	(r242391)
@@ -424,7 +424,6 @@ ath_attach(u_int16_t devid, struct ath_s
 	TASK_INIT(&sc->sc_resettask,0, ath_reset_proc, sc);
 	TASK_INIT(&sc->sc_txqtask,0, ath_txq_sched_tasklet, sc);
 	TASK_INIT(&sc->sc_fataltask,0, ath_fatal_proc, sc);
-	TASK_INIT(&sc->sc_txsndtask, 0, ath_start_task, sc);
 
 	/*
 	 * Allocate hardware transmit queues: one queue for
@@ -2447,7 +2446,9 @@ ath_start_queue(struct ifnet *ifp)
 {
 	struct ath_softc *sc = ifp->if_softc;
 
+	ATH_KTR(sc, ATH_KTR_TX, 0, "ath_start_queue: start");
 	ath_tx_kick(sc);
+	ATH_KTR(sc, ATH_KTR_TX, 0, "ath_start_queue: finished");
 }
 
 void
@@ -2456,6 +2457,8 @@ ath_start_task(void *arg, int npending)
 	struct ath_softc *sc = (struct ath_softc *) arg;
 	struct ifnet *ifp = sc->sc_ifp;
 
+	ATH_KTR(sc, ATH_KTR_TX, 0, "ath_start_task: start");
+
 	/* XXX is it ok to hold the ATH_LOCK here? */
 	ATH_PCU_LOCK(sc);
 	if (sc->sc_inreset_cnt > 0) {
@@ -2466,6 +2469,7 @@ ath_start_task(void *arg, int npending)
 		sc->sc_stats.ast_tx_qstop++;
 		ifp->if_drv_flags |= IFF_DRV_OACTIVE;
 		IF_UNLOCK(&ifp->if_snd);
+		ATH_KTR(sc, ATH_KTR_TX, 0, "ath_start_task: OACTIVE, finish");
 		return;
 	}
 	sc->sc_txstart_cnt++;
@@ -2476,6 +2480,7 @@ ath_start_task(void *arg, int npending)
 	ATH_PCU_LOCK(sc);
 	sc->sc_txstart_cnt--;
 	ATH_PCU_UNLOCK(sc);
+	ATH_KTR(sc, ATH_KTR_TX, 0, "ath_start_task: finished");
 }
 
 void
@@ -2486,10 +2491,13 @@ ath_start(struct ifnet *ifp)
 	struct ath_buf *bf;
 	struct mbuf *m, *next;
 	ath_bufhead frags;
+	int npkts = 0;
 
 	if ((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0 || sc->sc_invalid)
 		return;
 
+	ATH_KTR(sc, ATH_KTR_TX, 0, "ath_start: called");
+
 	for (;;) {
 		ATH_TXBUF_LOCK(sc);
 		if (sc->sc_txbuf_cnt <= sc->sc_txq_data_minfree) {
@@ -2517,6 +2525,7 @@ ath_start(struct ifnet *ifp)
 			break;
 		}
 		ni = (struct ieee80211_node *) m->m_pkthdr.rcvif;
+		npkts ++;
 		/*
 		 * Check for fragmentation.  If this frame
 		 * has been broken up verify we have enough
@@ -2590,6 +2599,7 @@ ath_start(struct ifnet *ifp)
 
 		sc->sc_wd_timer = 5;
 	}
+	ATH_KTR(sc, ATH_KTR_TX, 1, "ath_start: finished; npkts=%d", npkts);
 }
 
 static int

Modified: head/sys/dev/ath/if_ath_ahb.c
==============================================================================
--- head/sys/dev/ath/if_ath_ahb.c	Wed Oct 31 04:44:32 2012	(r242390)
+++ head/sys/dev/ath/if_ath_ahb.c	Wed Oct 31 06:27:58 2012	(r242391)
@@ -194,6 +194,7 @@ ath_ahb_attach(device_t dev)
 	ATH_LOCK_INIT(sc);
 	ATH_PCU_LOCK_INIT(sc);
 	ATH_RX_LOCK_INIT(sc);
+	ATH_TX_LOCK_INIT(sc);
 	ATH_TXSTATUS_LOCK_INIT(sc);
 
 	error = ath_attach(AR9130_DEVID, sc);
@@ -202,6 +203,7 @@ ath_ahb_attach(device_t dev)
 
 	ATH_TXSTATUS_LOCK_DESTROY(sc);
 	ATH_RX_LOCK_DESTROY(sc);
+	ATH_TX_LOCK_DESTROY(sc);
 	ATH_PCU_LOCK_DESTROY(sc);
 	ATH_LOCK_DESTROY(sc);
 	bus_dma_tag_destroy(sc->sc_dmat);
@@ -244,6 +246,7 @@ ath_ahb_detach(device_t dev)
 
 	ATH_TXSTATUS_LOCK_DESTROY(sc);
 	ATH_RX_LOCK_DESTROY(sc);
+	ATH_TX_LOCK_DESTROY(sc);
 	ATH_PCU_LOCK_DESTROY(sc);
 	ATH_LOCK_DESTROY(sc);
 

Modified: head/sys/dev/ath/if_ath_misc.h
==============================================================================
--- head/sys/dev/ath/if_ath_misc.h	Wed Oct 31 04:44:32 2012	(r242390)
+++ head/sys/dev/ath/if_ath_misc.h	Wed Oct 31 06:27:58 2012	(r242391)
@@ -124,7 +124,9 @@ static inline void
 ath_tx_kick(struct ath_softc *sc)
 {
 
-	taskqueue_enqueue(sc->sc_tq, &sc->sc_txsndtask);
+	ATH_TX_LOCK(sc);
+	ath_start(sc->sc_ifp);
+	ATH_TX_UNLOCK(sc);
 }
 
 #endif

Modified: head/sys/dev/ath/if_ath_pci.c
==============================================================================
--- head/sys/dev/ath/if_ath_pci.c	Wed Oct 31 04:44:32 2012	(r242390)
+++ head/sys/dev/ath/if_ath_pci.c	Wed Oct 31 06:27:58 2012	(r242391)
@@ -250,6 +250,7 @@ ath_pci_attach(device_t dev)
 	ATH_LOCK_INIT(sc);
 	ATH_PCU_LOCK_INIT(sc);
 	ATH_RX_LOCK_INIT(sc);
+	ATH_TX_LOCK_INIT(sc);
 	ATH_TXSTATUS_LOCK_INIT(sc);
 
 	error = ath_attach(pci_get_device(dev), sc);
@@ -259,6 +260,7 @@ ath_pci_attach(device_t dev)
 	ATH_TXSTATUS_LOCK_DESTROY(sc);
 	ATH_PCU_LOCK_DESTROY(sc);
 	ATH_RX_LOCK_DESTROY(sc);
+	ATH_TX_LOCK_DESTROY(sc);
 	ATH_LOCK_DESTROY(sc);
 	bus_dma_tag_destroy(sc->sc_dmat);
 bad3:
@@ -300,6 +302,7 @@ ath_pci_detach(device_t dev)
 	ATH_TXSTATUS_LOCK_DESTROY(sc);
 	ATH_PCU_LOCK_DESTROY(sc);
 	ATH_RX_LOCK_DESTROY(sc);
+	ATH_TX_LOCK_DESTROY(sc);
 	ATH_LOCK_DESTROY(sc);
 
 	return (0);

Modified: head/sys/dev/ath/if_ath_tx.c
==============================================================================
--- head/sys/dev/ath/if_ath_tx.c	Wed Oct 31 04:44:32 2012	(r242390)
+++ head/sys/dev/ath/if_ath_tx.c	Wed Oct 31 06:27:58 2012	(r242391)
@@ -2153,6 +2153,8 @@ ath_raw_xmit(struct ieee80211_node *ni, 
 	sc->sc_txstart_cnt++;
 	ATH_PCU_UNLOCK(sc);
 
+	ATH_TX_LOCK(sc);
+
 	if ((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0 || sc->sc_invalid) {
 		DPRINTF(sc, ATH_DEBUG_XMIT, "%s: discard frame, %s", __func__,
 		    (ifp->if_drv_flags & IFF_DRV_RUNNING) == 0 ?
@@ -2230,6 +2232,8 @@ ath_raw_xmit(struct ieee80211_node *ni, 
 	sc->sc_txstart_cnt--;
 	ATH_PCU_UNLOCK(sc);
 
+	ATH_TX_UNLOCK(sc);
+
 	return 0;
 bad2:
 	ATH_KTR(sc, ATH_KTR_TX, 3, "ath_raw_xmit: bad2: m=%p, params=%p, "
@@ -2241,6 +2245,9 @@ bad2:
 	ath_returnbuf_head(sc, bf);
 	ATH_TXBUF_UNLOCK(sc);
 bad:
+
+	ATH_TX_UNLOCK(sc);
+
 	ATH_PCU_LOCK(sc);
 	sc->sc_txstart_cnt--;
 	ATH_PCU_UNLOCK(sc);

Modified: head/sys/dev/ath/if_athvar.h
==============================================================================
--- head/sys/dev/ath/if_athvar.h	Wed Oct 31 04:44:32 2012	(r242390)
+++ head/sys/dev/ath/if_athvar.h	Wed Oct 31 06:27:58 2012	(r242391)
@@ -531,6 +531,8 @@ struct ath_softc {
 	char			sc_pcu_mtx_name[32];
 	struct mtx		sc_rx_mtx;	/* RX access mutex */
 	char			sc_rx_mtx_name[32];
+	struct mtx		sc_tx_mtx;	/* TX access mutex */
+	char			sc_tx_mtx_name[32];
 	struct taskqueue	*sc_tq;		/* private task queue */
 	struct ath_hal		*sc_ah;		/* Atheros HAL */
 	struct ath_ratectrl	*sc_rc;		/* tx rate control support */
@@ -663,7 +665,6 @@ struct ath_softc {
 	struct ath_txq		*sc_ac2q[5];	/* WME AC -> h/w q map */ 
 	struct task		sc_txtask;	/* tx int processing */
 	struct task		sc_txqtask;	/* tx proc processing */
-	struct task		sc_txsndtask;	/* tx send processing */
 
 	struct ath_descdma	sc_txcompdma;	/* TX EDMA completion */
 	struct mtx		sc_txcomplock;	/* TX EDMA completion lock */
@@ -779,6 +780,28 @@ struct ath_softc {
 #define	ATH_UNLOCK_ASSERT(_sc)	mtx_assert(&(_sc)->sc_mtx, MA_NOTOWNED)
 
 /*
+ * The TX lock is non-reentrant and serialises the TX send operations.
+ * (ath_start(), ath_raw_xmit().)  It doesn't yet serialise the TX
+ * completion operations; thus it can't be used (yet!) to protect
+ * hardware / software TXQ operations.
+ */
+#define	ATH_TX_LOCK_INIT(_sc) do {\
+	snprintf((_sc)->sc_tx_mtx_name,				\
+	    sizeof((_sc)->sc_tx_mtx_name),				\
+	    "%s TX lock",						\
+	    device_get_nameunit((_sc)->sc_dev));			\
+	mtx_init(&(_sc)->sc_tx_mtx, (_sc)->sc_tx_mtx_name,		\
+		 NULL, MTX_DEF);					\
+	} while (0)
+#define	ATH_TX_LOCK_DESTROY(_sc)	mtx_destroy(&(_sc)->sc_tx_mtx)
+#define	ATH_TX_LOCK(_sc)		mtx_lock(&(_sc)->sc_tx_mtx)
+#define	ATH_TX_UNLOCK(_sc)		mtx_unlock(&(_sc)->sc_tx_mtx)
+#define	ATH_TX_LOCK_ASSERT(_sc)	mtx_assert(&(_sc)->sc_tx_mtx,	\
+		MA_OWNED)
+#define	ATH_TX_UNLOCK_ASSERT(_sc)	mtx_assert(&(_sc)->sc_tx_mtx,	\
+		MA_NOTOWNED)
+
+/*
  * The PCU lock is non-recursive and should be treated as a spinlock.
  * Although currently the interrupt code is run in netisr context and
  * doesn't require this, this may change in the future.



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