PERFORCE change 41404 for review

Sam Leffler sam at FreeBSD.org
Tue Nov 4 17:54:34 PST 2003


http://perforce.freebsd.org/chv.cgi?CH=41404

Change 41404 by sam at sam_ebb on 2003/11/04 17:53:41

	o add an additional flags parameter to netisr_register to identify
	  whether an isr thread is MPSAFE or not (those that are not have
	  Giant acquired+released for them)
	o add NET_PICKUP_GIANT and NET_DROP_GIANT definitions to acquire
	  and release Giant according to the setting of debug_mpsafenet
	o cleanup existing code to not explicitly acquire Giant when the
	  netisr code does it for us
	o move debug_mpsafenet stuff to net/netisr.c and make the sysctl
	  r/o since it's only really meaningful at boot

Affected files ...

.. //depot/projects/netperf/sys/dev/usb/usb_ethersubr.c#3 edit
.. //depot/projects/netperf/sys/kern/kern_poll.c#7 edit
.. //depot/projects/netperf/sys/net/if_ppp.c#6 edit
.. //depot/projects/netperf/sys/net/netisr.c#6 edit
.. //depot/projects/netperf/sys/net/netisr.h#2 edit
.. //depot/projects/netperf/sys/netatalk/aarp.c#5 edit
.. //depot/projects/netperf/sys/netatalk/ddp_input.c#3 edit
.. //depot/projects/netperf/sys/netatalk/ddp_usrreq.c#2 edit
.. //depot/projects/netperf/sys/netgraph/ng_base.c#3 edit
.. //depot/projects/netperf/sys/netinet/if_ether.c#16 edit
.. //depot/projects/netperf/sys/netinet/ip_divert.c#10 edit
.. //depot/projects/netperf/sys/netinet/ip_input.c#18 edit
.. //depot/projects/netperf/sys/netinet/ip_mroute.c#19 edit
.. //depot/projects/netperf/sys/netinet/ip_output.c#13 edit
.. //depot/projects/netperf/sys/netinet6/ip6_input.c#18 edit
.. //depot/projects/netperf/sys/netipx/ipx_input.c#6 edit
.. //depot/projects/netperf/sys/netnatm/natm.c#5 edit
.. //depot/projects/netperf/sys/netnatm/natm_proto.c#3 edit
.. //depot/projects/netperf/sys/sys/mutex.h#5 edit

Differences ...

==== //depot/projects/netperf/sys/dev/usb/usb_ethersubr.c#3 (text+ko) ====

@@ -117,7 +117,7 @@
 {
 	if (mtx_inited)
 		return;
-	netisr_register(NETISR_USB, (netisr_t *)usbintr, NULL);
+	netisr_register(NETISR_USB, (netisr_t *)usbintr, NULL, 0);
 	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++;

==== //depot/projects/netperf/sys/kern/kern_poll.c#7 (text+ko) ====

@@ -187,8 +187,8 @@
 init_device_poll(void)
 {
 
-	netisr_register(NETISR_POLL, (netisr_t *)netisr_poll, NULL);
-	netisr_register(NETISR_POLLMORE, (netisr_t *)netisr_pollmore, NULL);
+	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)
 

==== //depot/projects/netperf/sys/net/if_ppp.c#6 (text+ko) ====

@@ -244,7 +244,7 @@
 	case MOD_LOAD: 
 		if_clone_attach(&ppp_cloner);
 
-		netisr_register(NETISR_PPP, (netisr_t *)pppintr, NULL);
+		netisr_register(NETISR_PPP, (netisr_t *)pppintr, NULL, 0);
 		/*
 		 * XXX layering violation - if_ppp can work over any lower
 		 * level transport that cares to attach to it.

==== //depot/projects/netperf/sys/net/netisr.c#6 (text+ko) ====

@@ -53,11 +53,22 @@
 #include <net/if_var.h>
 #include <net/netisr.h>
 
+/*
+ * XXX this is a temporary measure to allow folks to
+ * XXX disable Giant locking in the network code without
+ * XXX recompiling--in case of problems.
+ */
+int	debug_mpsafenet = 1;
+TUNABLE_INT("debug.mpsafenet", &debug_mpsafenet);
+SYSCTL_INT(_debug, OID_AUTO, mpsafenet, CTLFLAG_RD, &debug_mpsafenet, 0,
+    "Enable/disable MPSAFE network support");
+
 volatile unsigned int	netisr;	/* scheduling bits for network */
 
 struct netisr {
 	netisr_t	*ni_handler;
 	struct ifqueue	*ni_queue;
+	int		ni_flags;
 } netisrs[32];
 
 static void *net_ih;
@@ -69,13 +80,16 @@
 }
 
 void
-netisr_register(int num, netisr_t *handler, struct ifqueue *inq)
+netisr_register(int num, netisr_t *handler, struct ifqueue *inq, int flags)
 {
 	
 	KASSERT(!(num < 0 || num >= (sizeof(netisrs)/sizeof(*netisrs))),
 	    ("bad isr %d", num));
 	netisrs[num].ni_handler = handler;
 	netisrs[num].ni_queue = inq;
+	if ((flags & NETISR_MPSAFE) && !debug_mpsafenet)
+		flags &= ~NETISR_MPSAFE;
+	netisrs[num].ni_flags = flags;
 }
 
 void
@@ -166,8 +180,15 @@
 		 *
 		 * Currently, we do a).  Previously, we did c).
 		 */
-		netisr_processqueue(ni);
-		ni->ni_handler(m);
+		if ((ni->ni_flags & NETISR_MPSAFE) == 0) {
+			mtx_lock(&Giant);
+			netisr_processqueue(ni);
+			ni->ni_handler(m);
+			mtx_unlock(&Giant);
+		} else {
+			netisr_processqueue(ni);
+			ni->ni_handler(m);
+		}
 	} else {
 		isrstat.isrs_deferred++;
 		if (IF_HANDOFF(ni->ni_queue, m, NULL))
@@ -224,10 +245,19 @@
 				printf("swi_net: unregistered isr %d.\n", i);
 				continue;
 			}
-			if (ni->ni_queue == NULL)
-				ni->ni_handler(NULL);
-			else
-				netisr_processqueue(ni);
+			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);
+			}
 		}
 	} while (polling);
 }

==== //depot/projects/netperf/sys/net/netisr.h#2 (text+ko) ====

@@ -91,7 +91,8 @@
   
 void	netisr_dispatch(int, struct mbuf *);
 int	netisr_queue(int, struct mbuf *);
-void	netisr_register(int, netisr_t *, struct ifqueue *);
+#define	NETISR_MPSAFE	0x0001		/* ISR does not need Giant */
+void	netisr_register(int, netisr_t *, struct ifqueue *, int);
 void	netisr_unregister(int);
 
 #endif

==== //depot/projects/netperf/sys/netatalk/aarp.c#5 (text+ko) ====

@@ -287,9 +287,7 @@
     
     switch( ntohs( ar->ar_pro )) {
     case ETHERTYPE_AT :
-	mtx_lock(&Giant);
 	at_aarpinput( ac, m );
-	mtx_unlock(&Giant);
 	return;
 
     default:
@@ -314,6 +312,8 @@
     int			op;
     u_short		net;
 
+    GIANT_REQUIRED();
+
     ea = mtod( m, struct ether_aarp *);
 
     /* Check to see if from my hardware address */

==== //depot/projects/netperf/sys/netatalk/ddp_input.c#3 (text+ko) ====

@@ -39,13 +39,12 @@
 void
 at2intr(struct mbuf *m)
 {
+	GIANT_REQUIRED();
 
 	/*
 	 * Phase 2 packet handling 
 	 */
-	mtx_lock(&Giant);
 	ddp_input(m, m->m_pkthdr.rcvif, NULL, 2);
-	mtx_unlock(&Giant);
 	return;
 }
 
@@ -68,14 +67,14 @@
 	elhp = mtod(m, struct elaphdr *);
 	m_adj(m, SZ_ELAPHDR);
 
-	mtx_lock(&Giant);
+	GIANT_REQUIRED();
+
 	if (elhp->el_type == ELAP_DDPEXTEND) {
 		ddp_input(m, m->m_pkthdr.rcvif, NULL, 1);
 	} else {
 		bcopy((caddr_t)elhp, (caddr_t)&elh, SZ_ELAPHDR);
 		ddp_input(m, m->m_pkthdr.rcvif, &elh, 1);
 	}
-	mtx_unlock(&Giant);
 	return;
 }
 

==== //depot/projects/netperf/sys/netatalk/ddp_usrreq.c#2 (text+ko) ====

@@ -553,9 +553,9 @@
 	mtx_init(&atintrq1.ifq_mtx, "at1_inq", NULL, MTX_DEF);
 	mtx_init(&atintrq2.ifq_mtx, "at2_inq", NULL, MTX_DEF);
 	mtx_init(&aarpintrq.ifq_mtx, "aarp_inq", NULL, MTX_DEF);
-	netisr_register(NETISR_ATALK1, at1intr, &atintrq1);
-	netisr_register(NETISR_ATALK2, at2intr, &atintrq2);
-	netisr_register(NETISR_AARP, aarpintr, &aarpintrq);
+	netisr_register(NETISR_ATALK1, at1intr, &atintrq1, 0);
+	netisr_register(NETISR_ATALK2, at2intr, &atintrq2, 0);
+	netisr_register(NETISR_AARP, aarpintr, &aarpintrq, 0);
 }
 
 #if 0

==== //depot/projects/netperf/sys/netgraph/ng_base.c#3 (text+ko) ====

@@ -2986,7 +2986,8 @@
 		mtx_init(&ng_idhash_mtx, "netgraph idhash mutex", NULL, 0);
 		mtx_init(&ngq_mtx, "netgraph netisr mutex", NULL, 0);
 		s = splimp();
-		netisr_register(NETISR_NETGRAPH, (netisr_t *)ngintr, NULL);
+		/* XXX could use NETISR_MPSAFE but need to verify code */
+		netisr_register(NETISR_NETGRAPH, (netisr_t *)ngintr, NULL, 0);
 		splx(s);
 		break;
 	case MOD_UNLOAD:

==== //depot/projects/netperf/sys/netinet/if_ether.c#16 (text+ko) ====

@@ -980,6 +980,6 @@
 	mtx_init(&arpintrq.ifq_mtx, "arp_inq", NULL, MTX_DEF);
 	LIST_INIT(&llinfo_arp);
 	callout_init(&arp_callout, CALLOUT_MPSAFE);
-	netisr_register(NETISR_ARP, arpintr, &arpintrq);
+	netisr_register(NETISR_ARP, arpintr, &arpintrq, NETISR_MPSAFE);
 }
 SYSINIT(arp, SI_SUB_PROTO_DOMAIN, SI_ORDER_ANY, arp_init, 0);

==== //depot/projects/netperf/sys/netinet/ip_divert.c#10 (text+ko) ====

@@ -231,7 +231,7 @@
 	 * the moment we're ignoring this. Once sockets are
 	 * locked this cruft can be removed.
 	 */
-	mtx_lock(&Giant);	/* XXX */
+	NET_PICKUP_GIANT();
 	/* Put packet on socket queue, if any */
 	sa = NULL;
 	nport = htons((u_int16_t)port);
@@ -253,7 +253,7 @@
 		INP_UNLOCK(inp);
 	}
 	INP_INFO_RUNLOCK(&divcbinfo);
-	mtx_unlock(&Giant);	/* XXX */
+	NET_DROP_GIANT();
 	if (sa == NULL) {
 		m_freem(m);
 		ipstat.ips_noproto++;

==== //depot/projects/netperf/sys/netinet/ip_input.c#18 (text+ko) ====

@@ -323,7 +323,7 @@
 #endif
 	ipintrq.ifq_maxlen = ipqmaxlen;
 	mtx_init(&ipintrq.ifq_mtx, "ip_inq", NULL, MTX_DEF);
-	netisr_register(NETISR_IP, ip_input, &ipintrq);
+	netisr_register(NETISR_IP, ip_input, &ipintrq, NETISR_MPSAFE);
 }
 
 /*
@@ -999,7 +999,7 @@
 	 * Switch out to protocol's input routine.
 	 */
 	ipstat.ips_delivered++;
-	mtx_lock(&Giant);		/* XXX */
+	NET_PICKUP_GIANT();
 	if (args.next_hop && ip->ip_p == IPPROTO_TCP) {
 		/* TCP needs IPFORWARD info if available */
 		struct m_hdr tag;
@@ -1013,7 +1013,7 @@
 			(struct mbuf *)&tag, hlen);
 	} else
 		(*inetsw[ip_protox[ip->ip_p]].pr_input)(m, hlen);
-	mtx_unlock(&Giant);		/* XXX */
+	NET_DROP_GIANT();
 	return;
 bad:
 	m_freem(m);

==== //depot/projects/netperf/sys/netinet/ip_mroute.c#19 (text+ko) ====

@@ -1289,13 +1289,13 @@
 socket_send(struct socket *s, struct mbuf *mm, struct sockaddr_in *src)
 {
     if (s) {
-	mtx_lock(&Giant);	/* XXX no socket locking */
+	NET_PICKUP_GIANT();
 	if (sbappendaddr(&s->so_rcv, (struct sockaddr *)src, mm, NULL) != 0) {
 	    sorwakeup(s);
-	    mtx_unlock(&Giant);	/* XXX */
+	    NET_DROP_GIANT();
 	    return 0;
 	}
-        mtx_unlock(&Giant);	/* XXX */
+	NET_DROP_GIANT();
     }
     m_freem(mm);
     return -1;

==== //depot/projects/netperf/sys/netinet/ip_output.c#13 (text+ko) ====

@@ -206,6 +206,8 @@
 	KASSERT(ro != NULL, ("ip_output: no route, proto %d",
 	    mtod(m, struct ip *)->ip_p));
 #endif
+	if (inp != NULL)
+		INP_LOCK_ASSERT(inp);
 
 	if (args.rule != NULL) {	/* dummynet already saw us */
 		ip = mtod(m, struct ip *);

==== //depot/projects/netperf/sys/netinet6/ip6_input.c#18 (text+ko) ====

@@ -196,7 +196,7 @@
 #endif /* PFIL_HOOKS */
 	ip6intrq.ifq_maxlen = ip6qmaxlen;
 	mtx_init(&ip6intrq.ifq_mtx, "ip6_inq", NULL, MTX_DEF);
-	netisr_register(NETISR_IPV6, ip6_input, &ip6intrq);
+	netisr_register(NETISR_IPV6, ip6_input, &ip6intrq, 0);
 	scope6_init();
 	addrsel_policy_init();
 	nd6_init();
@@ -249,9 +249,7 @@
 #endif
 	int srcrt = 0;
 
-	mtx_assert(&Giant, MA_NOTOWNED);
-	mtx_lock(&Giant);
-
+	GIANT_REQUIRED();		/* XXX for now */
 #ifdef IPSEC
 	/*
 	 * should the inner packet be considered authentic?
@@ -331,7 +329,6 @@
 		if ((m = m_pullup(m, sizeof(struct ip6_hdr))) == NULL) {
 			ip6stat.ip6s_toosmall++;
 			in6_ifstat_inc(inifp, ifs6_in_hdrerr);
-			mtx_unlock(&Giant);
 			return;
 		}
 	}
@@ -353,14 +350,10 @@
 	 *     tell ip6_forward to do the right thing.
 	 */
 	odst = ip6->ip6_dst;
-	if (pfil_run_hooks(&inet6_pfil_hook, &m, m->m_pkthdr.rcvif, PFIL_IN)) {
-		mtx_unlock(&Giant);
+	if (pfil_run_hooks(&inet6_pfil_hook, &m, m->m_pkthdr.rcvif, PFIL_IN))
 		return;
-	}
-	if (m == NULL) {		/* consumed by filter */
-		mtx_unlock(&Giant);
+	if (m == NULL)			/* consumed by filter */
 		return;
-	}
 	ip6 = mtod(m, struct ip6_hdr *);
 	srcrt = !IN6_ARE_ADDR_EQUAL(&odst, &ip6->ip6_dst);
 #endif /* PFIL_HOOKS */
@@ -378,10 +371,8 @@
 			m_freem(m);
 			m = NULL;
 		}
-		if (!m) {
-			mtx_unlock(&Giant);
+		if (!m)
 			return;
-		}
 	}
 
 	/*
@@ -641,7 +632,6 @@
 #if 0	/*touches NULL pointer*/
 			in6_ifstat_inc(m->m_pkthdr.rcvif, ifs6_in_discard);
 #endif
-			mtx_unlock(&Giant);
 			return;	/* m have already been freed */
 		}
 
@@ -665,7 +655,6 @@
 			icmp6_error(m, ICMP6_PARAM_PROB,
 				    ICMP6_PARAMPROB_HEADER,
 				    (caddr_t)&ip6->ip6_plen - (caddr_t)ip6);
-			mtx_unlock(&Giant);
 			return;
 		}
 #ifndef PULLDOWN_TEST
@@ -676,7 +665,6 @@
 			sizeof(struct ip6_hbh));
 		if (hbh == NULL) {
 			ip6stat.ip6s_tooshort++;
-			mtx_unlock(&Giant);
 			return;
 		}
 #endif
@@ -725,17 +713,14 @@
 		if (ip6_mrouter && ip6_mforward(ip6, m->m_pkthdr.rcvif, m)) {
 			ip6stat.ip6s_cantforward++;
 			m_freem(m);
-			mtx_unlock(&Giant);
 			return;
 		}
 		if (!ours) {
 			m_freem(m);
-			mtx_unlock(&Giant);
 			return;
 		}
 	} else if (!ours) {
 		ip6_forward(m, srcrt);
-		mtx_unlock(&Giant);
 		return;
 	}
 
@@ -794,11 +779,9 @@
 #endif
 		nxt = (*inet6sw[ip6_protox[nxt]].pr_input)(&m, &off, nxt);
 	}
-	mtx_unlock(&Giant);
 	return;
  bad:
 	m_freem(m);
-	mtx_unlock(&Giant);
 }
 
 /*

==== //depot/projects/netperf/sys/netipx/ipx_input.c#6 (text+ko) ====

@@ -119,7 +119,7 @@
 
 	ipxintrq.ifq_maxlen = ipxqmaxlen;
 	mtx_init(&ipxintrq.ifq_mtx, "ipx_inq", NULL, MTX_DEF);
-	netisr_register(NETISR_IPX, ipxintr, &ipxintrq);
+	netisr_register(NETISR_IPX, ipxintr, &ipxintrq, 0);
 }
 
 /*
@@ -133,7 +133,8 @@
 	struct ipx_ifaddr *ia;
 	int len;
 
-	mtx_lock(&Giant);
+	GIANT_REQUIRED();
+
 	/*
 	 * If no IPX addresses have been set yet but the interfaces
 	 * are receiving, can't do anything with incoming packets yet.
@@ -192,7 +193,6 @@
 	if (ipx->ipx_pt == IPXPROTO_NETBIOS) {
 		if (ipxnetbios) {
 			ipx_output_type20(m);
-			mtx_unlock(&Giant);
 			return;
 		} else
 			goto bad;
@@ -228,7 +228,6 @@
 			 */
 			if (ipx->ipx_tc < IPX_MAXHOPS) {
 				ipx_forward(m);
-				mtx_unlock(&Giant);
 				return;
 			}
 		}
@@ -244,7 +243,6 @@
 
 		if (ia == NULL) {
 			ipx_forward(m);
-			mtx_unlock(&Giant);
 			return;
 		}
 	}
@@ -262,19 +260,16 @@
 			switch (ipx->ipx_pt) {
 			case IPXPROTO_SPX:
 				spx_input(m, ipxp);
-				mtx_unlock(&Giant);
 				return;
 			}
 		ipx_input(m, ipxp);
 	} else
 		goto bad;
 
-	mtx_unlock(&Giant);
 	return;
 
 bad:
 	m_freem(m);
-	mtx_unlock(&Giant);
 }
 
 void

==== //depot/projects/netperf/sys/netnatm/natm.c#5 (text+ko) ====

@@ -685,7 +685,7 @@
 	struct socket *so;
 	struct natmpcb *npcb;
 
-	mtx_lock(&Giant);
+	GIANT_REQUIRED();
 
 #ifdef DIAGNOSTIC
 	M_ASSERTPKTHDR(m);
@@ -702,12 +702,12 @@
 		m_freem(m);
 		if (npcb->npcb_inq == 0)
 			FREE(npcb, M_PCB);			/* done! */
-		goto done;
+		return;
 	}
 
 	if (npcb->npcb_flags & NPCB_FREE) {
 		m_freem(m);					/* drop */
-		goto done;
+		return;
 	}
 
 #ifdef NEED_TO_RESTORE_IFP
@@ -732,8 +732,6 @@
 #endif
 		m_freem(m);
 	}
-done:
-	mtx_unlock(&Giant);
 }
 
 /* 

==== //depot/projects/netperf/sys/netnatm/natm_proto.c#3 (text+ko) ====

@@ -122,7 +122,7 @@
 	bzero(&natmintrq, sizeof(natmintrq));
 	natmintrq.ifq_maxlen = natmqmaxlen;
 	mtx_init(&natmintrq.ifq_mtx, "natm_inq", NULL, MTX_DEF);
-	netisr_register(NETISR_NATM, natmintr, &natmintrq);
+	netisr_register(NETISR_NATM, natmintr, &natmintrq, 0);
 }
 
 #if defined(__FreeBSD__)

==== //depot/projects/netperf/sys/sys/mutex.h#5 (text+ko) ====

@@ -339,6 +339,27 @@
 		WITNESS_RESTORE(&Giant.mtx_object, Giant)
 #endif
 
+/*
+ * Network MPSAFE temporary workarounds.  When debug_mpsafenet
+ * is 1 the network is assumed to operate without Giant on the
+ * input path and protocols that require Giant must collect it
+ * on entry.  When 0 Giant is grabbed in the network interface
+ * ISR's and in the netisr path and there is no need to grab
+ * the Giant lock.
+ *
+ * This mechanism is intended as temporary until everything of
+ * importance is properly locked.
+ */
+extern	int debug_mpsafenet;		/* defined in kern/subr_bus.c */
+#define	NET_PICKUP_GIANT() do {						\
+	if (debug_mpsafenet)						\
+		mtx_lock(&Giant);					\
+} while (0)
+#define	NET_DROP_GIANT() do {						\
+	if (debug_mpsafenet)						\
+		mtx_unlock(&Giant);					\
+} while (0)
+
 #define	UGAR(rval) do {							\
 	int _val = (rval);						\
 	mtx_unlock(&Giant);						\


More information about the p4-projects mailing list