if_txtimer API to replace (*if_watchdog)()

[Jason Thorpe <thorpej%me.com@localhost>]

Folks --

The legacy (*if_watchdog)() interface has a couple of problems:

1- It does not have any way to represent multiple hardware-managed transmit queues.

2- It's not easy to make MP-safe because it relies on modifying the ifnet structure periodically outside of the normal locking mechanisms.

The wm(4) driver solved the problem in a reasonable way, and to make it easier for the other drivers in the tree to adopt it's strategy, I re-factored it into a new if_txtimer structure and API.

So save typing, I'll paste the relevant section of <net/if.h>:

/*
 * if_txtimer --
 *
 * Transmission timer (to replace the legacy ifnet watchdog timer).
 *
 * The driver should allocate one if_txtimer per hardware-managed
 * transmission queue and initialize it with if_txtimer_init(). The
 * driver should also arrange to have a callout invoked at a regular
 * interval (this may be coindidental with a timer used to check
 * link status).
 *
 * When the driver gives packets to the hardware to transmit, it should
 * arm the timer by calling if_txtimer_arm().  When it is sweeping up
 * completed transmit jobs, it should disarm the timer by calling
 * if_txtimer_disarm() if there are no outstanding jobs remaining.
 *
 * In the periodic callout, the driver should check if the timer has
 * expired by calling if_txtimer_is_expired().  It is not necessary to
 * check if the timer is armed when checking for expiration.  If the
 * timer has is expired, then the transmission has timed out and the
 * driver should take corrective action to recover from the error.
 *
 * If the driver needs to check multiple transmission queues, an
 * optimization is available that avoids repeated calls to fetch
 * the compare time.  In this case, the driver can get the compare
 * time by calling if_txtimer_now() and can check for timer expiration
 * using if_txtimer_is_expired_explicit().
 *
 * The granularity of the if_txtimer is 1 second.
 *
 * Locking: All locking of the if_txtimer is the responsibility of
 * the driver.  The if_txtimer should be protected by the same lock
 * that protects the associated transmission queue.
 */

See the diff for complete details.  Included is a conversion of wm(4) (which uses the if_txtimer_is_expired_explicit() variant), and pcn(4) (which uses the simpler if_txtimer_is_expired() variant).  The diff for pcn(4) looks artificially large because I relocated to pcn_txtimer_check() its sole call site after I renamed it from pcn_watchdog().

My plan is to get the new API in and start updating drivers to use the new mechanism.  The changes will be basically mechanical.  Before I get started on that, I'd like feedback on the proposed API.

Thx.

Index: net/if.c
===================================================================
RCS file: /cvsroot/src/sys/net/if.c,v
retrieving revision 1.468
diff -u -p -r1.468 if.c
--- net/if.c	20 Jan 2020 18:38:18 -0000	1.468
+++ net/if.c	20 Jan 2020 22:39:20 -0000
@@ -3774,6 +3774,91 @@ if_mcast_op(ifnet_t *ifp, const unsigned
 	return rc;
 }
 
+/*
+ * if_txtimer_init --
+ *	Initialize a network interface transmit timer.
+ */
+void
+if_txtimer_init(struct if_txtimer * const txt, unsigned int timeout)
+{
+
+	memset(txt, 0, sizeof(*txt));
+	txt->txt_timeout = timeout;
+}
+
+/*
+ * if_txtimer_arm --
+ *	Arm a network interface transmit timer.
+ */
+void
+if_txtimer_arm(struct if_txtimer * const txt)
+{
+	const time_t current_time = time_uptime;
+
+	txt->txt_armtime = current_time;
+	txt->txt_armed = true;
+}
+
+/*
+ * if_txtimer_disarm --
+ *	Disarm a network interface transmit timer.
+ */
+void
+if_txtimer_disarm(struct if_txtimer * const txt)
+{
+
+	txt->txt_armed = false;
+}
+
+/*
+ * if_txtimer_is_armed --
+ *	Return if a network interface transmit timer is armed.
+ */
+bool
+if_txtimer_is_armed(const struct if_txtimer * const txt)
+{
+
+	return txt->txt_armed;
+}
+
+/*
+ * if_txtimer_now --
+ *	Return the current value of "now" for the purpose of
+ *	checking for transmit timer expiration.
+ */
+time_t
+if_txtimer_now(void)
+{
+
+	return time_uptime;
+}
+
+/*
+ * if_txtimer_is_expired_explicit --
+ *	Return if a network interface transmit timer has expired,
+ *	using an explicit time.
+ */
+bool
+if_txtimer_is_expired_explicit(const struct if_txtimer * const txt,
+			       const time_t current_time)
+{
+
+	return txt->txt_armed &&
+	    (current_time - txt->txt_armtime) > txt->txt_timeout;
+}
+
+/*
+ * if_txtimer_is_expired --
+ *	Return if a network interface transmit timer has expired.
+ */
+bool
+if_txtimer_is_expired(const struct if_txtimer * const txt)
+{
+	const time_t current_time = time_uptime;
+
+	return if_txtimer_is_expired_explicit(txt, current_time);
+}
+
 static void
 sysctl_sndq_setup(struct sysctllog **clog, const char *ifname,
     struct ifaltq *ifq)
Index: net/if.h
===================================================================
RCS file: /cvsroot/src/sys/net/if.h,v
retrieving revision 1.277
diff -u -p -r1.277 if.h
--- net/if.h	19 Sep 2019 06:07:24 -0000	1.277
+++ net/if.h	20 Jan 2020 22:39:20 -0000
@@ -1182,6 +1182,54 @@ bool	ifa_is_destroying(struct ifaddr *);
 void	ifaref(struct ifaddr *);
 void	ifafree(struct ifaddr *);
 
+/*
+ * if_txtimer --
+ *
+ * Transmission timer (to replace the legacy ifnet watchdog timer).
+ *
+ * The driver should allocate one if_txtimer per hardware-managed
+ * transmission queue and initialize it with if_txtimer_init(). The
+ * driver should also arrange to have a callout invoked at a regular
+ * interval (this may be coindidental with a timer used to check
+ * link status).
+ *
+ * When the driver gives packets to the hardware to transmit, it should
+ * arm the timer by calling if_txtimer_arm().  When it is sweeping up
+ * completed transmit jobs, it should disarm the timer by calling
+ * if_txtimer_disarm() if there are no outstanding jobs remaining.
+ *
+ * In the periodic callout, the driver should check if the timer has
+ * expired by calling if_txtimer_is_expired().  It is not necessary to
+ * check if the timer is armed when checking for expiration.  If the
+ * timer has is expired, then the transmission has timed out and the
+ * driver should take corrective action to recover from the error.
+ *
+ * If the driver needs to check multiple transmission queues, an
+ * optimization is available that avoids repeated calls to fetch
+ * the compare time.  In this case, the driver can get the compare
+ * time by calling if_txtimer_now() and can check for timer expiration
+ * using if_txtimer_is_expired_explicit().
+ *
+ * The granularity of the if_txtimer is 1 second.
+ *
+ * Locking: All locking of the if_txtimer is the responsibility of
+ * the driver.  The if_txtimer should be protected by the same lock
+ * that protects the associated transmission queue.
+ */
+struct if_txtimer {
+	time_t		txt_armtime;
+	unsigned int	txt_timeout;
+	bool		txt_armed;
+};
+
+void	if_txtimer_init(struct if_txtimer *, unsigned int);
+void	if_txtimer_arm(struct if_txtimer *);
+void	if_txtimer_disarm(struct if_txtimer *);
+bool	if_txtimer_is_armed(const struct if_txtimer *);
+time_t	if_txtimer_now(void);
+bool	if_txtimer_is_expired_explicit(const struct if_txtimer *, const time_t);
+bool	if_txtimer_is_expired(const struct if_txtimer *);
+
 struct	ifaddr *ifa_ifwithaddr(const struct sockaddr *);
 struct	ifaddr *ifa_ifwithaddr_psref(const struct sockaddr *, struct psref *);
 struct	ifaddr *ifa_ifwithaf(int);
Index: dev/pci/if_pcn.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_pcn.c,v
retrieving revision 1.72
diff -u -p -r1.72 if_pcn.c
--- dev/pci/if_pcn.c	11 Oct 2019 14:22:46 -0000	1.72
+++ dev/pci/if_pcn.c	20 Jan 2020 22:39:21 -0000
@@ -296,6 +296,8 @@ struct pcn_softc {
 	int sc_flags;			/* misc. flags; see below */
 	int sc_swstyle;			/* the software style in use */
 
+	struct if_txtimer sc_txtimer;	/* transmit watchdog timer */
+
 	int sc_txfree;			/* number of free Tx descriptors */
 	int sc_txnext;			/* next ready Tx descriptor */
 
@@ -381,7 +383,6 @@ do {									\
 } while(/*CONSTCOND*/0)
 
 static void	pcn_start(struct ifnet *);
-static void	pcn_watchdog(struct ifnet *);
 static int	pcn_ioctl(struct ifnet *, u_long, void *);
 static int	pcn_init(struct ifnet *);
 static void	pcn_stop(struct ifnet *, int);
@@ -806,9 +807,9 @@ pcn_attach(device_t parent, device_t sel
 	ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
 	ifp->if_ioctl = pcn_ioctl;
 	ifp->if_start = pcn_start;
-	ifp->if_watchdog = pcn_watchdog;
 	ifp->if_init = pcn_init;
 	ifp->if_stop = pcn_stop;
+	if_txtimer_init(&sc->sc_txtimer, 5);
 	IFQ_SET_READY(&ifp->if_snd);
 
 	/* Attach the interface. */
@@ -1140,37 +1141,8 @@ pcn_start(struct ifnet *ifp)
 
 	if (sc->sc_txfree != ofree) {
 		/* Set a watchdog timer in case the chip flakes out. */
-		ifp->if_timer = 5;
-	}
-}
-
-/*
- * pcn_watchdog:	[ifnet interface function]
- *
- *	Watchdog timer handler.
- */
-static void
-pcn_watchdog(struct ifnet *ifp)
-{
-	struct pcn_softc *sc = ifp->if_softc;
-
-	/*
-	 * Since we're not interrupting every packet, sweep
-	 * up before we report an error.
-	 */
-	pcn_txintr(sc);
-
-	if (sc->sc_txfree != PCN_NTXDESC) {
-		printf("%s: device timeout (txfree %d txsfree %d)\n",
-		    device_xname(sc->sc_dev), sc->sc_txfree, sc->sc_txsfree);
-		ifp->if_oerrors++;
-
-		/* Reset the interface. */
-		(void) pcn_init(ifp);
+		if_txtimer_arm(&sc->sc_txtimer);
 	}
-
-	/* Try to get more packets going. */
-	pcn_start(ifp);
 }
 
 /*
@@ -1412,7 +1384,7 @@ pcn_txintr(struct pcn_softc *sc)
 	 * timer.
 	 */
 	if (sc->sc_txsfree == PCN_TXQUEUELEN)
-		ifp->if_timer = 0;
+		if_txtimer_disarm(&sc->sc_txtimer);
 }
 
 /*
@@ -1553,6 +1525,39 @@ pcn_rxintr(struct pcn_softc *sc)
 }
 
 /*
+ * pcn_txtimer_check:
+ *
+ *	Tx timer to check for stalled transmitter.
+ */
+static void
+pcn_txtimer_check(struct pcn_softc * const sc)
+{
+	struct ifnet * const ifp = &sc->sc_ethercom.ec_if;
+
+	if (__predict_true(!if_txtimer_is_expired(&sc->sc_txtimer))) {
+		return;
+	}
+
+	/*
+	 * Since we're not interrupting every packet, sweep
+	 * up before we report an error.
+	 */
+	pcn_txintr(sc);
+
+	if (__predict_false(sc->sc_txfree != PCN_NTXDESC)) {
+		printf("%s: device timeout (txfree %d txsfree %d)\n",
+		    device_xname(sc->sc_dev), sc->sc_txfree, sc->sc_txsfree);
+		ifp->if_oerrors++;
+
+		/* Reset the interface. */
+		(void) pcn_init(ifp);
+	}
+
+	/* Try to get more packets going. */
+	pcn_start(ifp);
+}
+
+/*
  * pcn_tick:
  *
  *	One second timer, used to tick the MII.
@@ -1564,7 +1569,13 @@ pcn_tick(void *arg)
 	int s;
 
 	s = splnet();
-	mii_tick(&sc->sc_mii);
+
+	if (__predict_true(sc->sc_flags & PCN_F_HAS_MII)) {
+		mii_tick(&sc->sc_mii);
+	}
+
+	pcn_txtimer_check(sc);
+
 	splx(s);
 
 	callout_reset(&sc->sc_tick_ch, hz, pcn_tick, sc);
@@ -1809,10 +1820,8 @@ pcn_init(struct ifnet *ifp)
 	/* Enable interrupts and external activity (and ACK IDON). */
 	pcn_csr_write(sc, LE_CSR0, LE_C0_INEA | LE_C0_STRT | LE_C0_IDON);
 
-	if (sc->sc_flags & PCN_F_HAS_MII) {
-		/* Start the one second MII clock. */
-		callout_reset(&sc->sc_tick_ch, hz, pcn_tick, sc);
-	}
+	/* Start the one second MII clock. */
+	callout_reset(&sc->sc_tick_ch, hz, pcn_tick, sc);
 
 	/* ...all done! */
 	ifp->if_flags |= IFF_RUNNING;
@@ -1880,7 +1889,7 @@ pcn_stop(struct ifnet *ifp, int disable)
 
 	/* Mark the interface as down and cancel the watchdog timer. */
 	ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE);
-	ifp->if_timer = 0;
+	if_txtimer_disarm(&sc->sc_txtimer);
 
 	if (disable)
 		pcn_rxdrain(sc);
Index: dev/pci/if_wm.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_wm.c,v
retrieving revision 1.659
diff -u -p -r1.659 if_wm.c
--- dev/pci/if_wm.c	20 Jan 2020 19:45:27 -0000	1.659
+++ dev/pci/if_wm.c	20 Jan 2020 22:39:21 -0000
@@ -370,8 +370,7 @@ struct wm_txqueue {
 
 	bool txq_stopping;
 
-	bool txq_sending;
-	time_t txq_lastsent;
+	struct if_txtimer txq_txtimer;
 
 	uint32_t txq_packets;		/* for AIM */
 	uint32_t txq_bytes;		/* for AIM */
@@ -704,9 +703,9 @@ static bool	wm_suspend(device_t, const p
 static bool	wm_resume(device_t, const pmf_qual_t *);
 static void	wm_watchdog(struct ifnet *);
 static void	wm_watchdog_txq(struct ifnet *, struct wm_txqueue *,
-    uint16_t *);
+				const time_t, uint16_t *);
 static void	wm_watchdog_txq_locked(struct ifnet *, struct wm_txqueue *,
-    uint16_t *);
+				       uint16_t *);
 static void	wm_tick(void *);
 static int	wm_ifflags_cb(struct ethercom *);
 static int	wm_ioctl(struct ifnet *, u_long, void *);
@@ -3148,10 +3147,12 @@ wm_watchdog(struct ifnet *ifp)
 	struct wm_softc *sc = ifp->if_softc;
 	uint16_t hang_queue = 0; /* Max queue number of wm(4) is 82576's 16. */
 
+	const time_t txq_now = if_txtimer_now();
+
 	for (qid = 0; qid < sc->sc_nqueues; qid++) {
 		struct wm_txqueue *txq = &sc->sc_queue[qid].wmq_txq;
 
-		wm_watchdog_txq(ifp, txq, &hang_queue);
+		wm_watchdog_txq(ifp, txq, txq_now, &hang_queue);
 	}
 
 	/* IF any of queues hanged up, reset the interface. */
@@ -3169,14 +3170,13 @@ wm_watchdog(struct ifnet *ifp)
 
 
 static void
-wm_watchdog_txq(struct ifnet *ifp, struct wm_txqueue *txq, uint16_t *hang)
+wm_watchdog_txq(struct ifnet *ifp, struct wm_txqueue *txq,
+		const time_t txq_now, uint16_t *hang)
 {
 
 	mutex_enter(txq->txq_lock);
-	if (txq->txq_sending &&
-	    time_uptime - txq->txq_lastsent > wm_watchdog_timeout)
+	if (if_txtimer_is_expired_explicit(&txq->txq_txtimer, txq_now))
 		wm_watchdog_txq_locked(ifp, txq, hang);
-
 	mutex_exit(txq->txq_lock);
 }
 
@@ -3195,7 +3195,7 @@ wm_watchdog_txq_locked(struct ifnet *ifp
 	 */
 	wm_txeof(txq, UINT_MAX);
 
-	if (txq->txq_sending)
+	if (if_txtimer_is_armed(&txq->txq_txtimer))
 		*hang |= __BIT(wmq->wmq_id);
 
 	if (txq->txq_free == WM_NTXDESC(txq)) {
@@ -6356,7 +6356,7 @@ wm_stop_locked(struct ifnet *ifp, int di
 		struct wm_queue *wmq = &sc->sc_queue[qidx];
 		struct wm_txqueue *txq = &wmq->wmq_txq;
 		mutex_enter(txq->txq_lock);
-		txq->txq_sending = false; /* Ensure watchdog disabled */
+		if_txtimer_disarm(&txq->txq_txtimer);
 		for (i = 0; i < WM_TXQUEUELEN(txq); i++) {
 			txs = &txq->txq_soft[i];
 			if (txs->txs_mbuf != NULL) {
@@ -6769,6 +6769,7 @@ wm_alloc_txrx_queues(struct wm_softc *sc
 		struct wm_txqueue *txq = &sc->sc_queue[i].wmq_txq;
 		txq->txq_sc = sc;
 		txq->txq_lock = mutex_obj_alloc(MUTEX_DEFAULT, IPL_NET);
+		if_txtimer_init(&txq->txq_txtimer, wm_watchdog_timeout);
 
 		error = wm_alloc_tx_descs(sc, txq);
 		if (error)
@@ -7058,7 +7059,7 @@ wm_init_tx_queue(struct wm_softc *sc, st
 	wm_init_tx_buffer(sc, txq);
 
 	txq->txq_flags = 0; /* Clear WM_TXQ_NO_SPACE */
-	txq->txq_sending = false;
+	if_txtimer_disarm(&txq->txq_txtimer);
 }
 
 static void
@@ -7833,8 +7834,7 @@ retry:
 
 	if (txq->txq_free != ofree) {
 		/* Set a watchdog timer in case the chip flakes out. */
-		txq->txq_lastsent = time_uptime;
-		txq->txq_sending = true;
+		if_txtimer_arm(&txq->txq_txtimer);
 	}
 }
 
@@ -8417,8 +8417,7 @@ retry:
 
 	if (sent) {
 		/* Set a watchdog timer in case the chip flakes out. */
-		txq->txq_lastsent = time_uptime;
-		txq->txq_sending = true;
+		if_txtimer_arm(&txq->txq_txtimer);
 	}
 }
 
@@ -8574,7 +8573,7 @@ wm_txeof(struct wm_txqueue *txq, u_int l
 	 * timer.
 	 */
 	if (txq->txq_sfree == WM_TXQUEUELEN(txq))
-		txq->txq_sending = false;
+		if_txtimer_disarm(&txq->txq_txtimer);
 
 	return more;
 }

-- thorpej