Skip site navigation (1)Skip section navigation (2) 
Date:      Sun, 14 Oct 2012 20:44:08 +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: r241559 - head/sys/dev/ath
Message-ID:  <201210142044.q9EKi8Kt038996@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: adrian
Date: Sun Oct 14 20:44:08 2012
New Revision: 241559
URL: http://svn.freebsd.org/changeset/base/241559

Log:
  Push the actual TX processing into the ath taskqueue, rather than having
  it run out of multiple concurrent contexts.
  
  Right now the ath(4) TX processing is a bit hairy. Specifically:
  
  * It was running out of ath_start(), which could occur from multiple
    concurrent sending processes (as if_start() can be started from multiple
    sending threads nowdays.. sigh)
  
  * during RX if fast frames are enabled (so not really at the moment, not
    until I fix this particular feature again..)
  
  * during ath_reset() - so anything which calls that
  
  * during ath_tx_proc*() in the ath taskqueue - ie, TX is attempted again
    after TX completion, as there's now hopefully some ath_bufs available.
  
  * Then, the ic_raw_xmit() method can queue raw frames for transmission
    at any time, from any net80211 TX context. Ew.
  
  This has caused packet ordering issues in the past - specifically,
  there's absolutely no guarantee that preemption won't occuring _during_
  ath_start() by the TX completion processing, which will call ath_start()
  again. It's a mess - 802.11 really, really wants things to be in
  sequence or things go all kinds of loopy.
  
  So:
  
  * create a new task struct for TX'ing;
  * make the if_start method simply queue the task on the ath taskqueue;
  * make ath_start() just be called by the new TX task;
  * make ath_tx_kick() just schedule the ath TX task, rather than directly
    calling ath_start().
  
  Now yes, this means that I've taken a step backwards in terms of
  concurrency - TX -and- RX now occur in the same single-task taskqueue.
  But there's nothing stopping me from separating out the TX / TX completion
  code into a separate taskqueue which runs in parallel with the RX path,
  if that ends up being appropriate for some platforms.
  
  This fixes the CCMP/seqno concurrency issues that creep up when you
  transmit large amounts of uni-directional UDP traffic (>200MBit) on a
  FreeBSD STA -> AP, as now there's only one TX context no matter what's
  going on (TX completion->retry/software queue,
  userland->net80211->ath_start(), TX completion -> ath_start());
  but it won't fix any concurrency issues between raw transmitted frames
  and non-raw transmitted frames (eg EAPOL frames on TID 16 and any other
  TID 16 multicast traffic that gets put on the CABQ.)  That is going to
  require a bunch more re-architecture before it's feasible to fix.
  
  In any case, this is a big step towards making the majority of the TX
  path locking irrelevant, as now almost all TX activity occurs in the
  taskqueue.
  
  Phew.

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

Modified: head/sys/dev/ath/if_ath.c
==============================================================================
--- head/sys/dev/ath/if_ath.c	Sun Oct 14 20:31:38 2012	(r241558)
+++ head/sys/dev/ath/if_ath.c	Sun Oct 14 20:44:08 2012	(r241559)
@@ -142,6 +142,7 @@ static void	ath_init(void *);
 static void	ath_stop_locked(struct ifnet *);
 static void	ath_stop(struct ifnet *);
 static int	ath_reset_vap(struct ieee80211vap *, u_long);
+static void	ath_start_queue(struct ifnet *ifp);
 static int	ath_media_change(struct ifnet *);
 static void	ath_watchdog(void *);
 static int	ath_ioctl(struct ifnet *, u_long, caddr_t);
@@ -420,6 +421,7 @@ 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
@@ -531,7 +533,7 @@ ath_attach(u_int16_t devid, struct ath_s
 
 	ifp->if_softc = sc;
 	ifp->if_flags = IFF_SIMPLEX | IFF_BROADCAST | IFF_MULTICAST;
-	ifp->if_start = ath_start;
+	ifp->if_start = ath_start_queue;
 	ifp->if_ioctl = ath_ioctl;
 	ifp->if_init = ath_init;
 	IFQ_SET_MAXLEN(&ifp->if_snd, ifqmaxlen);
@@ -2246,7 +2248,7 @@ ath_reset(struct ifnet *ifp, ATH_RESET_T
 	 * XXX should this be done by the caller, rather than
 	 * ath_reset() ?
 	 */
-	ath_start(ifp);			/* restart xmit */
+	ath_tx_kick(sc);		/* restart xmit */
 	return 0;
 }
 
@@ -2428,17 +2430,19 @@ ath_getbuf(struct ath_softc *sc, ath_buf
 	return bf;
 }
 
-void
-ath_start(struct ifnet *ifp)
+static void
+ath_start_queue(struct ifnet *ifp)
 {
 	struct ath_softc *sc = ifp->if_softc;
-	struct ieee80211_node *ni;
-	struct ath_buf *bf;
-	struct mbuf *m, *next;
-	ath_bufhead frags;
 
-	if ((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0 || sc->sc_invalid)
-		return;
+	ath_tx_kick(sc);
+}
+
+void
+ath_start_task(void *arg, int npending)
+{
+	struct ath_softc *sc = (struct ath_softc *) arg;
+	struct ifnet *ifp = sc->sc_ifp;
 
 	/* XXX is it ok to hold the ATH_LOCK here? */
 	ATH_PCU_LOCK(sc);
@@ -2455,6 +2459,25 @@ ath_start(struct ifnet *ifp)
 	sc->sc_txstart_cnt++;
 	ATH_PCU_UNLOCK(sc);
 
+	ath_start(sc->sc_ifp);
+
+	ATH_PCU_LOCK(sc);
+	sc->sc_txstart_cnt--;
+	ATH_PCU_UNLOCK(sc);
+}
+
+void
+ath_start(struct ifnet *ifp)
+{
+	struct ath_softc *sc = ifp->if_softc;
+	struct ieee80211_node *ni;
+	struct ath_buf *bf;
+	struct mbuf *m, *next;
+	ath_bufhead frags;
+
+	if ((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0 || sc->sc_invalid)
+		return;
+
 	for (;;) {
 		ATH_TXBUF_LOCK(sc);
 		if (sc->sc_txbuf_cnt <= sc->sc_txq_data_minfree) {
@@ -2549,10 +2572,6 @@ ath_start(struct ifnet *ifp)
 
 		sc->sc_wd_timer = 5;
 	}
-
-	ATH_PCU_LOCK(sc);
-	sc->sc_txstart_cnt--;
-	ATH_PCU_UNLOCK(sc);
 }
 
 static int

Modified: head/sys/dev/ath/if_ath_misc.h
==============================================================================
--- head/sys/dev/ath/if_ath_misc.h	Sun Oct 14 20:31:38 2012	(r241558)
+++ head/sys/dev/ath/if_ath_misc.h	Sun Oct 14 20:44:08 2012	(r241559)
@@ -115,12 +115,13 @@ extern	int ath_stoptxdma(struct ath_soft
  * we can kill this.
  */
 extern void ath_start(struct ifnet *ifp);
+extern	void ath_start_task(void *arg, int npending);
 
 static inline void
 ath_tx_kick(struct ath_softc *sc)
 {
 
-	ath_start(sc->sc_ifp);
+	taskqueue_enqueue(sc->sc_tq, &sc->sc_txsndtask);
 }
 
 #endif

Modified: head/sys/dev/ath/if_athvar.h
==============================================================================
--- head/sys/dev/ath/if_athvar.h	Sun Oct 14 20:31:38 2012	(r241558)
+++ head/sys/dev/ath/if_athvar.h	Sun Oct 14 20:44:08 2012	(r241559)
@@ -632,6 +632,7 @@ 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 */

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