Remove NETISR_MPSAFE, which allows specific netisr handlers to be directly

dispatched without Giant, and add NETISR_FORCEQUEUE, which allows specific
netisr handlers to always be dispatched via a queue (deferred).  Mark the
usb and if_ppp netisr handlers as NETISR_FORCEQUEUE, and explicitly
acquire Giant in those handlers.

Previously, any netisr handler not marked NETISR_MPSAFE would necessarily
run deferred and with Giant acquired.  This change removes Giant
scaffolding from the netisr infrastructure, but NETISR_FORCEQUEUE allows
non-MPSAFE handlers to continue to force deferred dispatch so as to avoid
lock order reversals between their acqusition of Giant and any calling
context.

It is likely we will be able to remove NETISR_FORCEQUEUE once
IFF_NEEDSGIANT is removed, as non-MPSAFE usb and if_ppp drivers will no
longer be supported.

Reviewed by:	bz
MFC after:	1 month
X-MFC note:	We can't remove NETISR_MPSAFE from stable/7 for KPI reasons,
		but the rest can go back.

13 files changed, 38 insertions, 51 deletions

diff --git a/sys/dev/usb/usb_ethersubr.c b/sys/dev/usb/usb_ethersubr.c
index 2cb5fa6..d0a3b83 100644
--- a/sys/dev/usb/usb_ethersubr.c
+++ b/sys/dev/usb/usb_ethersubr.c
@@ -82,6 +82,8 @@ usbintr(void)
 	struct usb_qdat		*q;
 	struct ifnet		*ifp;
 
+	mtx_lock(&Giant);
+
 	/* Check the RX queue */
 	while(1) {
 		IF_DEQUEUE(&usbq_rx, m);
@@ -109,6 +111,8 @@ usbintr(void)
 			(*ifp->if_start)(ifp);
 	}
 
+	mtx_unlock(&Giant);
+
 	return;
 }
 
@@ -117,7 +121,8 @@ usb_register_netisr(void)
 {
 	if (mtx_inited)
 		return;
-	netisr_register(NETISR_USB, (netisr_t *)usbintr, NULL, 0);
+	netisr_register(NETISR_USB, (netisr_t *)usbintr, NULL,
+	    NETISR_FORCEQUEUE);
 	mtx_init(&usbq_tx.ifq_mtx, "usbq_tx_mtx", NULL, MTX_DEF);
 	mtx_init(&usbq_rx.ifq_mtx, "usbq_rx_mtx", NULL, MTX_DEF);
 	mtx_inited++;
diff --git a/sys/kern/kern_poll.c b/sys/kern/kern_poll.c
index 95d6e41..64fa7af 100644
--- a/sys/kern/kern_poll.c
+++ b/sys/kern/kern_poll.c
@@ -262,10 +262,8 @@ init_device_poll(void)
 {
 
 	mtx_init(&poll_mtx, "polling", NULL, MTX_DEF);
-	netisr_register(NETISR_POLL, (netisr_t *)netisr_poll, NULL,
-	    NETISR_MPSAFE);
-	netisr_register(NETISR_POLLMORE, (netisr_t *)netisr_pollmore, NULL,
-	    NETISR_MPSAFE);
+	netisr_register(NETISR_POLL, (netisr_t *)netisr_poll, NULL, 0);
+	netisr_register(NETISR_POLLMORE, (netisr_t *)netisr_pollmore, NULL, 0);
 }
 SYSINIT(device_poll, SI_SUB_CLOCKS, SI_ORDER_MIDDLE, init_device_poll, NULL);
 
diff --git a/sys/net/if_ppp.c b/sys/net/if_ppp.c
index 5030ac58..a24cd81 100644
--- a/sys/net/if_ppp.c
+++ b/sys/net/if_ppp.c
@@ -274,7 +274,8 @@ ppp_modevent(module_t mod, int type, void *data)
 		LIST_INIT(&ppp_softc_list);
 		if_clone_attach(&ppp_cloner);
 
-		netisr_register(NETISR_PPP, (netisr_t *)pppintr, NULL, 0);
+		netisr_register(NETISR_PPP, (netisr_t *)pppintr, NULL,
+		    NETISR_FORCEQUEUE);
 		/*
 		 * XXX layering violation - if_ppp can work over any lower
 		 * level transport that cares to attach to it.
@@ -1212,8 +1213,7 @@ pppintr()
     int s;
     struct mbuf *m;
 
-    GIANT_REQUIRED;
-
+    mtx_lock(&Giant);
     PPP_LIST_LOCK();
     LIST_FOREACH(sc, &ppp_softc_list, sc_list) {
 	s = splimp();
@@ -1237,6 +1237,7 @@ pppintr()
 	}
     }
     PPP_LIST_UNLOCK();
+    mtx_unlock(&Giant);
 }
 
 #ifdef PPP_COMPRESS
diff --git a/sys/net/netisr.c b/sys/net/netisr.c
index 9307b53..ed5466c 100644
--- a/sys/net/netisr.c
+++ b/sys/net/netisr.c
@@ -77,6 +77,8 @@ netisr_register(int num, netisr_t *handler, struct ifqueue *inq, int flags)
 	
 	KASSERT(!(num < 0 || num >= (sizeof(netisrs)/sizeof(*netisrs))),
 	    ("bad isr %d", num));
+	KASSERT(flags == 0 || flags == NETISR_FORCEQUEUE,
+	    ("netisr_register: bad flags 0x%x\n", flags));
 	netisrs[num].ni_handler = handler;
 	netisrs[num].ni_queue = inq;
 	netisrs[num].ni_flags = flags;
@@ -161,27 +163,18 @@ netisr_dispatch(int num, struct mbuf *m)
 		m_freem(m);
 		return;
 	}
+
 	/*
-	 * Do direct dispatch only for MPSAFE netisrs (and
-	 * only when enabled).  Note that when a netisr is
-	 * marked MPSAFE we permit multiple concurrent instances
-	 * to run.  We guarantee only the order in which
-	 * packets are processed for each "dispatch point" in
-	 * the system (i.e. call to netisr_dispatch or
-	 * netisr_queue).  This insures ordering of packets
-	 * from an interface but does not guarantee ordering
-	 * between multiple places in the system (e.g. IP
-	 * dispatched from interfaces vs. IP queued from IPSec).
+	 * Unless NETISR_FORCEQUEUE is set on the netisr (generally
+	 * indicating that the handler still requires Giant, which cannot be
+	 * acquired in arbitrary order with respect to a caller), directly
+	 * dispatch handling of this packet.  Source ordering is maintained
+	 * by virtue of callers consistently calling one of queued or direct
+	 * dispatch, and the forcequeue flag being immutable after
+	 * registration.
 	 */
-	if (netisr_direct && (ni->ni_flags & NETISR_MPSAFE)) {
+	if (netisr_direct && !(ni->ni_flags & NETISR_FORCEQUEUE)) {
 		isrstat.isrs_directed++;
-		/*
-		 * NB: We used to drain the queue before handling
-		 * the packet but now do not.  Doing so here will
-		 * not preserve ordering so instead we fallback to
-		 * guaranteeing order only from dispatch points
-		 * in the system (see above).
-		 */
 		ni->ni_handler(m);
 	} else {
 		isrstat.isrs_deferred++;
@@ -242,19 +235,10 @@ swi_net(void *dummy)
 				printf("swi_net: unregistered isr %d.\n", i);
 				continue;
 			}
-			if ((ni->ni_flags & NETISR_MPSAFE) == 0) {
-				mtx_lock(&Giant);
-				if (ni->ni_queue == NULL)
-					ni->ni_handler(NULL);
-				else
-					netisr_processqueue(ni);
-				mtx_unlock(&Giant);
-			} else {
-				if (ni->ni_queue == NULL)
-					ni->ni_handler(NULL);
-				else
-					netisr_processqueue(ni);
-			}
+			if (ni->ni_queue == NULL)
+				ni->ni_handler(NULL);
+			else
+				netisr_processqueue(ni);
 		}
 	} while (polling);
 }
diff --git a/sys/net/netisr.h b/sys/net/netisr.h
index 1fb411c..d5bd8e7 100644
--- a/sys/net/netisr.h
+++ b/sys/net/netisr.h
@@ -83,7 +83,7 @@ typedef void netisr_t (struct mbuf *);
   
 void	netisr_dispatch(int, struct mbuf *);
 int	netisr_queue(int, struct mbuf *);
-#define	NETISR_MPSAFE	0x0001		/* ISR does not need Giant */
+#define	NETISR_FORCEQUEUE	0x0002		/* Force queued dispatch. */
 void	netisr_register(int, netisr_t *, struct ifqueue *, int);
 void	netisr_unregister(int);
 
diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c
index 9511035..99e0257 100644
--- a/sys/net/rtsock.c
+++ b/sys/net/rtsock.c
@@ -117,7 +117,7 @@ rts_init(void)
 	if (TUNABLE_INT_FETCH("net.route.netisr_maxqlen", &tmp))
 		rtsintrq.ifq_maxlen = tmp;
 	mtx_init(&rtsintrq.ifq_mtx, "rts_inq", NULL, MTX_DEF);
-	netisr_register(NETISR_ROUTE, rts_input, &rtsintrq, NETISR_MPSAFE);
+	netisr_register(NETISR_ROUTE, rts_input, &rtsintrq, 0);
 }
 SYSINIT(rtsock, SI_SUB_PROTO_DOMAIN, SI_ORDER_THIRD, rts_init, 0);
 
diff --git a/sys/netatalk/ddp_usrreq.c b/sys/netatalk/ddp_usrreq.c
index df38e2a..3db88cf 100644
--- a/sys/netatalk/ddp_usrreq.c
+++ b/sys/netatalk/ddp_usrreq.c
@@ -267,9 +267,9 @@ ddp_init(void)
 	mtx_init(&atintrq2.ifq_mtx, "at2_inq", NULL, MTX_DEF);
 	mtx_init(&aarpintrq.ifq_mtx, "aarp_inq", NULL, MTX_DEF);
 	DDP_LIST_LOCK_INIT();
-	netisr_register(NETISR_ATALK1, at1intr, &atintrq1, NETISR_MPSAFE);
-	netisr_register(NETISR_ATALK2, at2intr, &atintrq2, NETISR_MPSAFE);
-	netisr_register(NETISR_AARP, aarpintr, &aarpintrq, NETISR_MPSAFE);
+	netisr_register(NETISR_ATALK1, at1intr, &atintrq1, 0);
+	netisr_register(NETISR_ATALK2, at2intr, &atintrq2, 0);
+	netisr_register(NETISR_AARP, aarpintr, &aarpintrq, 0);
 }
 
 #if 0
diff --git a/sys/netgraph/ng_base.c b/sys/netgraph/ng_base.c
index 12256b0..1213273 100644
--- a/sys/netgraph/ng_base.c
+++ b/sys/netgraph/ng_base.c
@@ -3067,8 +3067,7 @@ ngb_mod_event(module_t mod, int event, void *data)
 		ng_qdzone = uma_zcreate("NetGraph data items", sizeof(struct ng_item),
 		    NULL, NULL, NULL, NULL, UMA_ALIGN_CACHE, 0);
 		uma_zone_set_max(ng_qdzone, maxdata);
-		netisr_register(NETISR_NETGRAPH, (netisr_t *)ngintr, NULL,
-		    NETISR_MPSAFE);
+		netisr_register(NETISR_NETGRAPH, (netisr_t *)ngintr, NULL, 0);
 		break;
 	case MOD_UNLOAD:
 		/* You can't unload it because an interface may be using it. */
diff --git a/sys/netinet/if_ether.c b/sys/netinet/if_ether.c
index 6939dbb..206cf37 100644
--- a/sys/netinet/if_ether.c
+++ b/sys/netinet/if_ether.c
@@ -1040,6 +1040,6 @@ arp_init(void)
 
 	arpintrq.ifq_maxlen = 50;
 	mtx_init(&arpintrq.ifq_mtx, "arp_inq", NULL, MTX_DEF);
-	netisr_register(NETISR_ARP, arpintr, &arpintrq, NETISR_MPSAFE);
+	netisr_register(NETISR_ARP, arpintr, &arpintrq, 0);
 }
 SYSINIT(arp, SI_SUB_PROTO_DOMAIN, SI_ORDER_ANY, arp_init, 0);
diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c
index 34aa982..7abb4e4 100644
--- a/sys/netinet/ip_input.c
+++ b/sys/netinet/ip_input.c
@@ -267,7 +267,7 @@ ip_init(void)
 	ip_id = time_second & 0xffff;
 	ipintrq.ifq_maxlen = ipqmaxlen;
 	mtx_init(&ipintrq.ifq_mtx, "ip_inq", NULL, MTX_DEF);
-	netisr_register(NETISR_IP, ip_input, &ipintrq, NETISR_MPSAFE);
+	netisr_register(NETISR_IP, ip_input, &ipintrq, 0);
 }
 
 void
diff --git a/sys/netinet6/ip6_input.c b/sys/netinet6/ip6_input.c
index 50fca58..7b4e4dc 100644
--- a/sys/netinet6/ip6_input.c
+++ b/sys/netinet6/ip6_input.c
@@ -180,7 +180,7 @@ ip6_init(void)
 
 	ip6intrq.ifq_maxlen = ip6qmaxlen;
 	mtx_init(&ip6intrq.ifq_mtx, "ip6_inq", NULL, MTX_DEF);
-	netisr_register(NETISR_IPV6, ip6_input, &ip6intrq, NETISR_MPSAFE);
+	netisr_register(NETISR_IPV6, ip6_input, &ip6intrq, 0);
 	scope6_init();
 	addrsel_policy_init();
 	nd6_init();
diff --git a/sys/netipx/ipx_input.c b/sys/netipx/ipx_input.c
index e57586c..4d29c60 100644
--- a/sys/netipx/ipx_input.c
+++ b/sys/netipx/ipx_input.c
@@ -153,7 +153,7 @@ ipx_init(void)
 
 	ipxintrq.ifq_maxlen = ipxqmaxlen;
 	mtx_init(&ipxintrq.ifq_mtx, "ipx_inq", NULL, MTX_DEF);
-	netisr_register(NETISR_IPX, ipxintr, &ipxintrq, NETISR_MPSAFE);
+	netisr_register(NETISR_IPX, ipxintr, &ipxintrq, 0);
 }
 
 /*
diff --git a/sys/netnatm/natm_proto.c b/sys/netnatm/natm_proto.c
index 976f4b4..145fb58 100644
--- a/sys/netnatm/natm_proto.c
+++ b/sys/netnatm/natm_proto.c
@@ -105,7 +105,7 @@ natm_init(void)
 	natmintrq.ifq_maxlen = natmqmaxlen;
 	NATM_LOCK_INIT();
 	mtx_init(&natmintrq.ifq_mtx, "natm_inq", NULL, MTX_DEF);
-	netisr_register(NETISR_NATM, natmintr, &natmintrq, NETISR_MPSAFE);
+	netisr_register(NETISR_NATM, natmintr, &natmintrq, 0);
 }
 
 DOMAIN_SET(natm);