kern/168649: commit references a PR

dfilter service dfilter at FreeBSD.ORG
Mon Jun 4 22:10:13 UTC 2012

The following reply was made to PR kern/168649; it has been noted by GNATS.

From: dfilter at FreeBSD.ORG (dfilter service)
To: bug-followup at
Subject: Re: kern/168649: commit references a PR
Date: Mon,  4 Jun 2012 22:01:27 +0000 (UTC)

 Author: adrian
 Date: Mon Jun  4 22:01:12 2012
 New Revision: 236583
   Migrate the TX path to a taskqueue for now, until a better way of
   implementing parallel TX and TX/RX completion can be done without
   simply abusing long-held locks.
   Right now, multiple concurrent ath_start() entries can result in
   frames being dequeued out of order.  Well, they're dequeued in order
   fine, but if there's any preemption or race between CPUs between:
   * removing the frame from the ifnet, and
   * calling and runningath_tx_start(), until the frame is placed on a
     software or hardware TXQ
   Then although dequeueing the frame is in-order, queueing it to the hardware
   may be out of order.
   This is solved in a lot of other drivers by just holding a TX lock over
   a rather long period of time.  This lets them continue to direct dispatch
   without races between dequeue and hardware queue.
   Note to observers: if_transmit() doesn't necessarily solve this.
   It removes the ifnet from the main path, but the same issue exists if
   there's some intermediary queue (eg a bufring, which as an aside also
   may pull in ifnet when you're using ALTQ.)
   So, until I can sit down and code up a much better way of doing parallel
   TX, I'm going to leave the TX path using a deferred taskqueue task.
   What I will likely head towards is doing a direct dispatch to hardware
   or software via if_transmit(), but it'll require some driver changes to
   allow queues to be made without using the really large ath_buf / ath_desc
   * Look at how feasible it'll be to just do direct dispatch to
     ath_tx_start() from if_transmit(), avoiding doing _any_ intermediary
     serialisation into a global queue.  This may break ALTQ for example,
     so I have to be delicate.
   * It's quite likely that I should break up ath_tx_start() so it
     deposits frames onto the software queues first, and then only fill
     in the 802.11 fields when it's being queued to the hardware.
     That will make the if_transmit() -> software queue path very
     quick and lightweight.
   * This has some very bad behaviour when using ACPI and Cx states.
     I'll do some subsequent analysis using KTR and schedgraph and file
     a follow-up PR or two.
   PR:		kern/168649
 Modified: head/sys/dev/ath/if_ath.c
 --- head/sys/dev/ath/if_ath.c	Mon Jun  4 21:34:49 2012	(r236582)
 +++ head/sys/dev/ath/if_ath.c	Mon Jun  4 22:01:12 2012	(r236583)
 @@ -373,6 +373,7 @@ ath_attach(u_int16_t devid, struct ath_s
  		"%s taskq", ifp->if_xname);
  	TASK_INIT(&sc->sc_rxtask, 0, ath_rx_tasklet, sc);
 +	TASK_INIT(&sc->sc_txstarttask, 0, ath_tx_tasklet, sc);
  	TASK_INIT(&sc->sc_bmisstask, 0, ath_bmiss_proc, sc);
  	TASK_INIT(&sc->sc_bstucktask,0, ath_bstuck_proc, sc);
  	TASK_INIT(&sc->sc_resettask,0, ath_reset_proc, sc);
 @@ -2325,6 +2326,15 @@ void
  ath_start(struct ifnet *ifp)
  	struct ath_softc *sc = ifp->if_softc;
 +	taskqueue_enqueue(sc->sc_tq, &sc->sc_txstarttask);
 +ath_tx_tasklet(void *arg, int npending)
 +	struct ath_softc *sc = (struct ath_softc *) arg;
 +	struct ifnet *ifp = sc->sc_ifp;
  	struct ieee80211_node *ni;
  	struct ath_buf *bf;
  	struct mbuf *m, *next;
 @@ -3499,7 +3509,8 @@ ath_tx_proc_q0(void *arg, int npending)
 -	ath_start(ifp);
 +	// ath_start(ifp);
 +	ath_tx_tasklet(sc, 1);
 @@ -3549,7 +3560,8 @@ ath_tx_proc_q0123(void *arg, int npendin
 -	ath_start(ifp);
 +	//ath_start(ifp);
 +	ath_tx_tasklet(sc, 1);
 @@ -3592,7 +3604,8 @@ ath_tx_proc(void *arg, int npending)
 -	ath_start(ifp);
 +	//ath_start(ifp);
 +	ath_tx_tasklet(sc, 1);
  #undef	TXQACTIVE
 Modified: head/sys/dev/ath/if_ath_misc.h
 --- head/sys/dev/ath/if_ath_misc.h	Mon Jun  4 21:34:49 2012	(r236582)
 +++ head/sys/dev/ath/if_ath_misc.h	Mon Jun  4 22:01:12 2012	(r236583)
 @@ -83,6 +83,7 @@ extern void ath_setslottime(struct ath_s
   * we can kill this.
  extern void ath_start(struct ifnet *ifp);
 +extern void ath_tx_tasklet(void *arg, int npending);
 Modified: head/sys/dev/ath/if_ath_rx.c
 --- head/sys/dev/ath/if_ath_rx.c	Mon Jun  4 21:34:49 2012	(r236582)
 +++ head/sys/dev/ath/if_ath_rx.c	Mon Jun  4 22:01:12 2012	(r236583)
 @@ -899,7 +899,8 @@ rx_proc_next:
  		ieee80211_ff_age_all(ic, 100);
  		if (!IFQ_IS_EMPTY(&ifp->if_snd))
 -			ath_start(ifp);
 +			ath_tx_tasklet(sc, 1);
 +			//ath_start(ifp);
  #undef PA2DESC
 Modified: head/sys/dev/ath/if_athvar.h
 --- head/sys/dev/ath/if_athvar.h	Mon Jun  4 21:34:49 2012	(r236582)
 +++ head/sys/dev/ath/if_athvar.h	Mon Jun  4 22:01:12 2012	(r236583)
 @@ -476,6 +476,7 @@ struct ath_softc {
  	struct mbuf		*sc_rxpending;	/* pending receive data */
  	u_int32_t		*sc_rxlink;	/* link ptr in last RX desc */
  	struct task		sc_rxtask;	/* rx int processing */
 +	struct task		sc_txstarttask;	/* ath_start() processing */
  	u_int8_t		sc_defant;	/* current default antenna */
  	u_int8_t		sc_rxotherant;	/* rx's on non-default antenna*/
  	u_int64_t		sc_lastrx;	/* tsf at last rx'd frame */
 svn-src-all at mailing list
 To unsubscribe, send any mail to "svn-src-all-unsubscribe at"

More information about the freebsd-bugs mailing list