rework the if_start mpsafe serialisation so it can serialise arbitrary work

work is represented by struct task.

the start routine is now wrapped by a task which is serialised by the
infrastructure. if_start_barrier has been renamed to ifq_barrier and
is now implemented as a task that gets serialised with the start
routine.

this also adds an ifq_restart() function. it serialises a call to
ifq_clr_oactive and calls the start routine again. it exists to
avoid a race that kettenis@ identified in between when a start
routine discovers theres no space left on a ring, and when it calls
ifq_set_oactive. if the txeof side of the driver empties the ring
and calls ifq_clr_oactive in between the above calls in start, the
queue will be marked oactive and the stack will never call the start
routine again.

by serialising the ifq_set_oactive call in the start routine and
ifq_clr_oactive calls we avoid that race.

tested on various nics
ok mpi@

4 files changed, 126 insertions, 36 deletions

diff --git a/sys/net/if.c b/sys/net/if.c
index 417f6fc1706..76646a2e799 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if.c,v 1.424 2015/12/08 10:18:56 mpi Exp $	*/
+/*	$OpenBSD: if.c,v 1.425 2015/12/09 03:22:39 dlg Exp $	*/
 /*	$NetBSD: if.c,v 1.35 1996/05/07 05:26:04 thorpej Exp $	*/
 
 /*
@@ -153,7 +153,6 @@ void	if_input_process(void *);
 void	ifa_print_all(void);
 #endif
 
-void	if_start_mpsafe(struct ifnet *ifp);
 void	if_start_locked(struct ifnet *ifp);
 
 /*
@@ -511,7 +510,7 @@ if_attach_common(struct ifnet *ifp)
 	TAILQ_INIT(&ifp->if_addrlist);
 	TAILQ_INIT(&ifp->if_maddrlist);
 
-	ifq_init(&ifp->if_snd);
+	ifq_init(&ifp->if_snd, ifp);
 
 	ifp->if_addrhooks = malloc(sizeof(*ifp->if_addrhooks),
 	    M_TEMP, M_WAITOK);
@@ -539,7 +538,7 @@ void
 if_start(struct ifnet *ifp)
 {
 	if (ISSET(ifp->if_xflags, IFXF_MPSAFE))
-		if_start_mpsafe(ifp);
+		ifq_start(&ifp->if_snd);
 	else
 		if_start_locked(ifp);
 }
diff --git a/sys/net/if_var.h b/sys/net/if_var.h
index c0bd1ef7459..d98abe02300 100644
--- a/sys/net/if_var.h
+++ b/sys/net/if_var.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_var.h,v 1.66 2015/12/08 10:14:58 dlg Exp $	*/
+/*	$OpenBSD: if_var.h,v 1.67 2015/12/09 03:22:39 dlg Exp $	*/
 /*	$NetBSD: if.h,v 1.23 1996/05/07 02:40:27 thorpej Exp $	*/
 
 /*
@@ -42,6 +42,7 @@
 #include <sys/mbuf.h>
 #include <sys/srp.h>
 #include <sys/refcnt.h>
+#include <sys/task.h>
 #include <sys/time.h>
 
 #include <net/ifq.h>
diff --git a/sys/net/ifq.c b/sys/net/ifq.c
index 51d41dac4d9..93e5adb09ae 100644
--- a/sys/net/ifq.c
+++ b/sys/net/ifq.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ifq.c,v 1.1 2015/12/08 10:06:12 dlg Exp $ */
+/*	$OpenBSD: ifq.c,v 1.2 2015/12/09 03:22:39 dlg Exp $ */
 
 /*
  * Copyright (c) 2015 David Gwynne <dlg@openbsd.org>
@@ -64,6 +64,12 @@ struct priq {
  * ifqueue serialiser
  */
 
+void	ifq_start_task(void *);
+void	ifq_restart_task(void *);
+void	ifq_barrier_task(void *);
+
+#define TASK_ONQUEUE 0x1
+
 static inline unsigned int
 ifq_enter(struct ifqueue *ifq)
 {
@@ -73,57 +79,108 @@ ifq_enter(struct ifqueue *ifq)
 static inline unsigned int
 ifq_leave(struct ifqueue *ifq)
 {
-	if (atomic_cas_uint(&ifq->ifq_serializer, 1, 0) == 1)
-		return (1);
+	return (atomic_cas_uint(&ifq->ifq_serializer, 1, 0) == 1);
+}
+
+static inline int
+ifq_next_task(struct ifqueue *ifq, struct task *work)
+{
+	struct task *t;
+	int rv = 0;
 
 	ifq->ifq_serializer = 1;
 
-	return (0);
+	mtx_enter(&ifq->ifq_task_mtx);
+	t = TAILQ_FIRST(&ifq->ifq_task_list);
+	if (t != NULL) {
+		TAILQ_REMOVE(&ifq->ifq_task_list, t, t_entry);
+		CLR(t->t_flags, TASK_ONQUEUE);
+
+		*work = *t; /* copy to caller to avoid races */
+
+		rv = 1;
+	}
+	mtx_leave(&ifq->ifq_task_mtx);
+
+	return (rv);
 }
 
 void
-if_start_mpsafe(struct ifnet *ifp)
+ifq_serialize(struct ifqueue *ifq, struct task *t)
 {
-	struct ifqueue *ifq = &ifp->if_snd;
+	struct task work;
+
+	if (ISSET(t->t_flags, TASK_ONQUEUE))
+		return;
+
+	mtx_enter(&ifq->ifq_task_mtx);
+	if (!ISSET(t->t_flags, TASK_ONQUEUE)) {
+		SET(t->t_flags, TASK_ONQUEUE);
+		TAILQ_INSERT_TAIL(&ifq->ifq_task_list, t, t_entry);
+	}
+	mtx_leave(&ifq->ifq_task_mtx);
 
 	if (!ifq_enter(ifq))
 		return;
 
 	do {
-		if (__predict_false(!ISSET(ifp->if_flags, IFF_RUNNING))) {
-			ifq->ifq_serializer = 0;
-			wakeup_one(&ifq->ifq_serializer);
-			return;
-		}
+		while (ifq_next_task(ifq, &work))
+			(*work.t_func)(work.t_arg);
 
-		if (ifq_empty(ifq) || ifq_is_oactive(ifq))
-			continue;
+	} while (!ifq_leave(ifq));
+}
 
-		ifp->if_start(ifp);
+void
+ifq_start_task(void *p)
+{
+	struct ifqueue *ifq = p;
+	struct ifnet *ifp = ifq->ifq_if;
 
-	} while (!ifq_leave(ifq));
+	if (!ISSET(ifp->if_flags, IFF_RUNNING) ||
+	    ifq_empty(ifq) || ifq_is_oactive(ifq))
+		return;
+
+	ifp->if_start(ifp);
 }
 
 void
-if_start_barrier(struct ifnet *ifp)
+ifq_restart_task(void *p)
+{
+	struct ifqueue *ifq = p;
+	struct ifnet *ifp = ifq->ifq_if;
+
+	ifq_clr_oactive(ifq);
+	ifp->if_start(ifp);
+}
+
+void
+ifq_barrier(struct ifqueue *ifq)
 {
 	struct sleep_state sls;
-	struct ifqueue *ifq = &ifp->if_snd;
+	unsigned int notdone = 1;
+	struct task t = TASK_INITIALIZER(ifq_barrier_task, &notdone);
 
 	/* this should only be called from converted drivers */
-	KASSERT(ISSET(ifp->if_xflags, IFXF_MPSAFE));
-
-	/* drivers should only call this on the way down */
-	KASSERT(!ISSET(ifp->if_flags, IFF_RUNNING));
+	KASSERT(ISSET(ifq->ifq_if->if_xflags, IFXF_MPSAFE));
 
 	if (ifq->ifq_serializer == 0)
 		return;
 
-	if_start_mpsafe(ifp); /* spin the wheel to guarantee a wakeup */
-	do {
-		sleep_setup(&sls, &ifq->ifq_serializer, PWAIT, "ifbar");
-		sleep_finish(&sls, ifq->ifq_serializer != 0);
-	} while (ifq->ifq_serializer != 0);
+	ifq_serialize(ifq, &t);
+
+	while (notdone) {
+		sleep_setup(&sls, &notdone, PWAIT, "ifqbar");
+		sleep_finish(&sls, notdone);
+	}
+}
+
+void
+ifq_barrier_task(void *p)
+{
+	unsigned int *notdone = p;
+
+	*notdone = 0;
+	wakeup_one(notdone);
 }
 
 /*
@@ -131,8 +188,10 @@ if_start_barrier(struct ifnet *ifp)
  */
 
 void
-ifq_init(struct ifqueue *ifq)
+ifq_init(struct ifqueue *ifq, struct ifnet *ifp)
 {
+	ifq->ifq_if = ifp;
+
 	mtx_init(&ifq->ifq_mtx, IPL_NET);
 	ifq->ifq_drops = 0;
 
@@ -140,9 +199,15 @@ ifq_init(struct ifqueue *ifq)
 	ifq->ifq_ops = &priq_ops;
 	ifq->ifq_q = priq_ops.ifqop_alloc(NULL);
 
-	ifq->ifq_serializer = 0;
 	ifq->ifq_len = 0;
 
+	mtx_init(&ifq->ifq_task_mtx, IPL_NET);
+	TAILQ_INIT(&ifq->ifq_task_list);
+	ifq->ifq_serializer = 0;
+
+	task_set(&ifq->ifq_start, ifq_start_task, ifq);
+	task_set(&ifq->ifq_restart, ifq_restart_task, ifq);
+
 	if (ifq->ifq_maxlen == 0)
 		ifq_set_maxlen(ifq, IFQ_MAXLEN);
 }
diff --git a/sys/net/ifq.h b/sys/net/ifq.h
index e16dbf4f399..c23d0678522 100644
--- a/sys/net/ifq.h
+++ b/sys/net/ifq.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ifq.h,v 1.1 2015/12/08 10:06:12 dlg Exp $ */
+/*	$OpenBSD: ifq.h,v 1.2 2015/12/09 03:22:39 dlg Exp $ */
 
 /*
  * Copyright (c) 2015 David Gwynne <dlg@openbsd.org>
@@ -24,14 +24,25 @@ struct ifnet;
 struct ifq_ops;
 
 struct ifqueue {
+	struct ifnet		*ifq_if;
+
+	/* mbuf handling */
 	struct mutex		 ifq_mtx;
 	uint64_t		 ifq_drops;
 	const struct ifq_ops	*ifq_ops;
 	void			*ifq_q;
 	unsigned int		 ifq_len;
-	unsigned int		 ifq_serializer;
 	unsigned int		 ifq_oactive;
 
+	/* work serialisation */
+	struct mutex		 ifq_task_mtx;
+	struct task_list	 ifq_task_list;
+	unsigned int		 ifq_serializer;
+
+	/* work to be serialised */
+	struct task		 ifq_start;
+	struct task		 ifq_restart;
+
 	unsigned int		 ifq_maxlen;
 };
 
@@ -54,7 +65,7 @@ struct ifq_ops {
  * Interface send queues.
  */
 
-void		 ifq_init(struct ifqueue *);
+void		 ifq_init(struct ifqueue *, struct ifnet *);
 void		 ifq_attach(struct ifqueue *, const struct ifq_ops *, void *);
 void		 ifq_destroy(struct ifqueue *);
 int		 ifq_enqueue_try(struct ifqueue *, struct mbuf *);
@@ -66,6 +77,8 @@ struct mbuf	*ifq_dequeue(struct ifqueue *);
 unsigned int	 ifq_purge(struct ifqueue *);
 void		*ifq_q_enter(struct ifqueue *, const struct ifq_ops *);
 void		 ifq_q_leave(struct ifqueue *, void *);
+void		 ifq_serialize(struct ifqueue *, struct task *);
+void		 ifq_barrier(struct ifqueue *);
 
 #define	ifq_len(_ifq)			((_ifq)->ifq_len)
 #define	ifq_empty(_ifq)			(ifq_len(_ifq) == 0)
@@ -89,6 +102,18 @@ ifq_is_oactive(struct ifqueue *ifq)
 	return (ifq->ifq_oactive);
 }
 
+static inline void
+ifq_start(struct ifqueue *ifq)
+{
+	ifq_serialize(ifq, &ifq->ifq_start);
+}
+
+static inline void
+ifq_restart(struct ifqueue *ifq)
+{
+	ifq_serialize(ifq, &ifq->ifq_restart);
+}
+
 extern const struct ifq_ops * const ifq_priq_ops;
 
 #endif /* _KERNEL */