Removing NET_NEEDS_GIANT: first patch
Robert Watson
rwatson at FreeBSD.org
Tue Jul 24 10:17:46 UTC 2007
Dear all,
Per many discussions over the past few years, we've finally reached the end of
the road on non-MPSAFE network protocol components. Recently, Bjoern, George,
and I resolved, in various ways, the last few remaining protocol-layer
dependencies (IPSEC, i4b, IPX over IP, netatm), and there are now no
Giant-dependent protocol components left. This is the result of a lot of
hardware work by a great many FreeBSD developers over the past six years, and
I really appreciate everyone's hard work to make this possible!
Attached is the first of a series of patches to start removing the
NET_NEEDS_GIANT and debug.mpsafenet scaffolding. This source code declaration
was used by optionally compiled components to declare a strict requirement for
Giant, and forced Giant over the entire network stack. debug.mpsafenet could
also be set by users in loader.conf in order to similar force Giant over the
network stack, and existed for two reasons: to allow Giant to be put back over
the network stack for debugging purposes, and to support these recently
removed or fixed unsafe components. As such, this patch removes the
following:
- NET_NEEDS_GIANT() macro
- debug.mpsafenet tunable/sysctl and associated debug_mpsafenet variable, as
well as functions supporting these.
- Use of this variable to control acqusition of Giant in network-related
interrupt handlers and various other paths.
If also converts the following to no-ops, allowing them to be gradually
removed in a series of follow-up patches to minimize risk and disruption:
- NET_LOCK_GIANT(), NET_UNLOCK_GIANT()
- NET_ASSERT_GIANT()
Finally, the following conversion takes place:
- NET_CALLOUT_MPSAFE is now an alias for CALLOUT_MPSAFE
The patch is attached, and I would appreciate feedback on the patch and any
testing. Assuming no serious negative feedback in the next couple of days,
I'll request permission to commit from re at . The patches that follow will:
- Replace all remaining NET_CALLOUT_MPSAFE instances with CALLOUT_MPSAFE,
removing NET_CALLOUT_MPSAFE.
- Removing all references to NET_{LOCK,UNLOCK,ASSERT}_GIANT.
- Clean up some other weird Giant-related compatibility in the Cronyx drivers.
Things this patch doesn't do:
- Address the WITNESS lock order warnings generated when credential rules are
used with ipfw/pf. These are believed to be annoying but non-harmful, as
deadlocks are no longer reported. This view may be revised if evidence to
the contrary is presented.
- Remove IFF_NEEDSGIANT, a similar set of Giant-acquiring shims used to
protect non-MPSAFE device drivers. I had hoped to remove this in 7.0, but I
think this will happen instead for 8.0 due to remaining non-MPSAFEty in a
few key network device drivers.
Patch attached, and also available at this URL:
http://www.watson.org/~robert/freebsd/netperf/20070724-no_net_needs_giant_1.diff
Robert N M Watson
Computer Laboratory
University of Cambridge
-------------- next part --------------
Index: dev/ath/ath_rate/amrr/amrr.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/dev/ath/ath_rate/amrr/amrr.c,v
retrieving revision 1.13
diff -u -r1.13 amrr.c
--- dev/ath/ath_rate/amrr/amrr.c 11 Jun 2007 03:36:50 -0000 1.13
+++ dev/ath/ath_rate/amrr/amrr.c 16 Jul 2007 07:46:43 -0000
@@ -504,7 +504,7 @@
if (asc == NULL)
return NULL;
asc->arc.arc_space = sizeof(struct amrr_node);
- callout_init(&asc->timer, debug_mpsafenet ? CALLOUT_MPSAFE : 0);
+ callout_init(&asc->timer, CALLOUT_MPSAFE);
ath_rate_sysctlattach(sc);
return &asc->arc;
Index: dev/ath/ath_rate/onoe/onoe.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/dev/ath/ath_rate/onoe/onoe.c,v
retrieving revision 1.14
diff -u -r1.14 onoe.c
--- dev/ath/ath_rate/onoe/onoe.c 11 Jun 2007 03:36:50 -0000 1.14
+++ dev/ath/ath_rate/onoe/onoe.c 16 Jul 2007 07:46:57 -0000
@@ -478,7 +478,7 @@
if (osc == NULL)
return NULL;
osc->arc.arc_space = sizeof(struct onoe_node);
- callout_init(&osc->timer, debug_mpsafenet ? CALLOUT_MPSAFE : 0);
+ callout_init(&osc->timer, CALLOUT_MPSAFE);
ath_rate_sysctlattach(sc);
return &osc->arc;
Index: dev/ce/if_ce.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/dev/ce/if_ce.c,v
retrieving revision 1.8
diff -u -r1.8 if_ce.c
--- dev/ce/if_ce.c 25 Mar 2007 20:21:31 -0000 1.8
+++ dev/ce/if_ce.c 16 Jul 2007 07:47:45 -0000
@@ -2604,13 +2604,6 @@
#if __FreeBSD_version < 500000
dev = makedev (CDEV_MAJOR, 0);
#endif
-#if __FreeBSD_version >= 501114
- if (!debug_mpsafenet && ce_mpsafenet) {
- printf ("WORNING! Network stack is not MPSAFE. "
- "Turning off debug.ce.mpsafenet.\n");
- ce_mpsafenet = 0;
- }
-#endif
#if __FreeBSD_version >= 502103
if (ce_mpsafenet)
ce_cdevsw.d_flags &= ~D_NEEDGIANT;
Index: dev/cp/if_cp.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/dev/cp/if_cp.c,v
retrieving revision 1.33
diff -u -r1.33 if_cp.c
--- dev/cp/if_cp.c 21 Mar 2007 03:38:35 -0000 1.33
+++ dev/cp/if_cp.c 16 Jul 2007 07:48:07 -0000
@@ -2265,11 +2265,6 @@
{
static int load_count = 0;
- if (!debug_mpsafenet && cp_mpsafenet) {
- printf ("WORNING! Network stack is not MPSAFE. "
- "Turning off debug.cp.mpsafenet.\n");
- cp_mpsafenet = 0;
- }
if (cp_mpsafenet)
cp_cdevsw.d_flags &= ~D_NEEDGIANT;
Index: dev/ctau/if_ct.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/dev/ctau/if_ct.c,v
retrieving revision 1.33
diff -u -r1.33 if_ct.c
--- dev/ctau/if_ct.c 21 Mar 2007 03:38:35 -0000 1.33
+++ dev/ctau/if_ct.c 16 Jul 2007 07:48:58 -0000
@@ -2206,11 +2206,6 @@
{
static int load_count = 0;
- if (!debug_mpsafenet && ct_mpsafenet) {
- printf ("WORNING! Network stack is not MPSAFE. "
- "Turning off debug.ct.mpsafenet.\n");
- ct_mpsafenet = 0;
- }
if (ct_mpsafenet)
ct_cdevsw.d_flags &= ~D_NEEDGIANT;
Index: dev/cx/if_cx.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/dev/cx/if_cx.c,v
retrieving revision 1.56
diff -u -r1.56 if_cx.c
--- dev/cx/if_cx.c 21 Mar 2007 03:38:35 -0000 1.56
+++ dev/cx/if_cx.c 16 Jul 2007 07:49:08 -0000
@@ -2523,11 +2523,6 @@
{
static int load_count = 0;
- if (!debug_mpsafenet && cx_mpsafenet) {
- printf ("WORNING! Network stack is not MPSAFE. "
- "Turning off debug.cx.mpsafenet.\n");
- cx_mpsafenet = 0;
- }
if (cx_mpsafenet)
cx_cdevsw.d_flags &= ~D_NEEDGIANT;
Index: kern/subr_bus.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/kern/subr_bus.c,v
retrieving revision 1.200
diff -u -r1.200 subr_bus.c
--- kern/subr_bus.c 23 May 2007 17:28:21 -0000 1.200
+++ kern/subr_bus.c 16 Jul 2007 07:46:25 -0000
@@ -3464,9 +3464,6 @@
int error;
if (dev->parent != NULL) {
- if ((flags &~ INTR_ENTROPY) == (INTR_TYPE_NET | INTR_MPSAFE) &&
- !debug_mpsafenet)
- flags &= ~INTR_MPSAFE;
error = BUS_SETUP_INTR(dev->parent, dev, r, flags,
filter, handler, arg, cookiep);
if (error == 0) {
Index: kern/uipc_domain.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/kern/uipc_domain.c,v
retrieving revision 1.49
diff -u -r1.49 uipc_domain.c
--- kern/uipc_domain.c 16 May 2007 20:41:07 -0000 1.49
+++ kern/uipc_domain.c 16 Jul 2007 07:45:56 -0000
@@ -211,13 +211,8 @@
if (max_linkhdr < 16) /* XXX */
max_linkhdr = 16;
- if (debug_mpsafenet) {
- callout_init(&pffast_callout, CALLOUT_MPSAFE);
- callout_init(&pfslow_callout, CALLOUT_MPSAFE);
- } else {
- callout_init(&pffast_callout, 0);
- callout_init(&pfslow_callout, 0);
- }
+ callout_init(&pffast_callout, CALLOUT_MPSAFE);
+ callout_init(&pfslow_callout, CALLOUT_MPSAFE);
mtx_lock(&dom_mtx);
KASSERT(domain_init_status == 0, ("domaininit called too late!"));
Index: net/if.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/net/if.c,v
retrieving revision 1.272
diff -u -r1.272 if.c
--- net/if.c 16 May 2007 19:59:01 -0000 1.272
+++ net/if.c 16 Jul 2007 07:43:32 -0000
@@ -2694,9 +2694,7 @@
if_start(struct ifnet *ifp)
{
- NET_ASSERT_GIANT();
-
- if ((ifp->if_flags & IFF_NEEDSGIANT) != 0 && debug_mpsafenet != 0) {
+ if (ifp->if_flags & IFF_NEEDSGIANT) {
if (mtx_owned(&Giant))
(*(ifp)->if_start)(ifp);
else
@@ -2711,11 +2709,6 @@
{
struct ifnet *ifp;
- /*
- * This code must be entered with Giant, and should never run if
- * we're not running with debug.mpsafenet.
- */
- KASSERT(debug_mpsafenet != 0, ("if_start_deferred: debug.mpsafenet"));
GIANT_REQUIRED;
ifp = context;
Index: net/if_ethersubr.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/net/if_ethersubr.c,v
retrieving revision 1.234
diff -u -r1.234 if_ethersubr.c
--- net/if_ethersubr.c 3 Jul 2007 12:46:06 -0000 1.234
+++ net/if_ethersubr.c 16 Jul 2007 07:45:43 -0000
@@ -922,7 +922,7 @@
break;
if (i != ifp->if_addrlen)
if_printf(ifp, "Ethernet address: %6D\n", lla, ":");
- if (debug_mpsafenet && (ifp->if_flags & IFF_NEEDSGIANT) != 0)
+ if (ifp->if_flags & IFF_NEEDSGIANT)
if_printf(ifp, "if_start running deferred for Giant\n");
}
Index: net/netisr.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/net/netisr.c,v
retrieving revision 1.18
diff -u -r1.18 netisr.c
--- net/netisr.c 28 Nov 2006 11:19:36 -0000 1.18
+++ net/netisr.c 16 Jul 2007 07:40:57 -0000
@@ -56,24 +56,6 @@
#include <net/if_var.h>
#include <net/netisr.h>
-/*
- * debug_mpsafenet controls network subsystem-wide use of the Giant lock,
- * from system calls down to interrupt handlers. It can be changed only via
- * a tunable at boot, not at run-time, due to the complexity of unwinding.
- * The compiled default is set via a kernel option; right now, the default
- * unless otherwise specified is to run the network stack without Giant.
- */
-#ifdef NET_WITH_GIANT
-int debug_mpsafenet = 0;
-#else
-int debug_mpsafenet = 1;
-#endif
-int debug_mpsafenet_toolatetotwiddle = 0;
-
-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 {
@@ -84,78 +66,6 @@
static void *net_ih;
-/*
- * Not all network code is currently capable of running MPSAFE; however,
- * most of it is. Since those sections that are not are generally optional
- * components not shipped with default kernels, we provide a basic way to
- * determine whether MPSAFE operation is permitted: based on a default of
- * yes, we permit non-MPSAFE components to use a registration call to
- * identify that they require Giant. If the system is early in the boot
- * process still, then we change the debug_mpsafenet setting to choose a
- * non-MPSAFE execution mode (degraded). If it's too late for that (since
- * the setting cannot be changed at run time), we generate a console warning
- * that the configuration may be unsafe.
- */
-static int mpsafe_warn_count;
-
-/*
- * Function call implementing registration of a non-MPSAFE network component.
- */
-void
-net_warn_not_mpsafe(const char *component)
-{
-
- /*
- * If we're running with Giant over the network stack, there is no
- * problem.
- */
- if (!debug_mpsafenet)
- return;
-
- /*
- * If it's not too late to change the MPSAFE setting for the network
- * stack, do so now. This effectively suppresses warnings by
- * components registering later.
- */
- if (!debug_mpsafenet_toolatetotwiddle) {
- debug_mpsafenet = 0;
- printf("WARNING: debug.mpsafenet forced to 0 as %s requires "
- "Giant\n", component);
- return;
- }
-
- /*
- * We must run without Giant, so generate a console warning with some
- * information with what to do about it. The system may be operating
- * unsafely, however.
- */
- printf("WARNING: Network stack Giant-free, but %s requires Giant.\n",
- component);
- if (mpsafe_warn_count == 0)
- printf(" Consider adding 'options NET_WITH_GIANT' or "
- "setting debug.mpsafenet=0\n");
- mpsafe_warn_count++;
-}
-
-/*
- * This sysinit is run after any pre-loaded or compiled-in components have
- * announced that they require Giant, but before any modules loaded at
- * run-time.
- */
-static void
-net_mpsafe_toolate(void *arg)
-{
-
- debug_mpsafenet_toolatetotwiddle = 1;
-
- if (!debug_mpsafenet)
- printf("WARNING: MPSAFE network stack disabled, expect "
- "reduced performance.\n");
-}
-
-SYSINIT(net_mpsafe_toolate, SI_SUB_SETTINGS, SI_ORDER_ANY, net_mpsafe_toolate,
- NULL);
-
void
legacy_setsoftnet(void)
{
@@ -170,8 +80,6 @@
("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;
}
Index: nfsserver/nfs_srvsubs.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/nfsserver/nfs_srvsubs.c,v
retrieving revision 1.147
diff -u -r1.147 nfs_srvsubs.c
--- nfsserver/nfs_srvsubs.c 2 Apr 2007 13:53:26 -0000 1.147
+++ nfsserver/nfs_srvsubs.c 16 Jul 2007 07:45:20 -0000
@@ -549,10 +549,7 @@
nfsrv_initcache(); /* Init the server request cache */
NFSD_LOCK();
nfsrv_init(0); /* Init server data structures */
- if (debug_mpsafenet)
- callout_init(&nfsrv_callout, CALLOUT_MPSAFE);
- else
- callout_init(&nfsrv_callout, 0);
+ callout_init(&nfsrv_callout, CALLOUT_MPSAFE);
NFSD_UNLOCK();
nfsrv_timer(0);
Index: nfsserver/nfs_syscalls.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/nfsserver/nfs_syscalls.c,v
retrieving revision 1.114
diff -u -r1.114 nfs_syscalls.c
--- nfsserver/nfs_syscalls.c 22 Apr 2007 15:31:21 -0000 1.114
+++ nfsserver/nfs_syscalls.c 16 Jul 2007 07:45:03 -0000
@@ -561,16 +561,11 @@
nfsd->nfsd_slp = NULL;
nfsrv_slpderef(slp);
}
- KASSERT(!(debug_mpsafenet == 0 && !mtx_owned(&Giant)),
- ("nfssvc_nfsd(): debug.mpsafenet=0 && !Giant"));
- KASSERT(!(debug_mpsafenet == 1 && mtx_owned(&Giant)),
- ("nfssvc_nfsd(): debug.mpsafenet=1 && Giant"));
+ KASSERT(!mtx_owned(&Giant),
+ ("nfssvc_nfsd: Giant when loop ends"));
}
done:
- KASSERT(!(debug_mpsafenet == 0 && !mtx_owned(&Giant)),
- ("nfssvc_nfsd(): debug.mpsafenet=0 && !Giant"));
- KASSERT(!(debug_mpsafenet == 1 && mtx_owned(&Giant)),
- ("nfssvc_nfsd(): debug.mpsafenet=1 && Giant"));
+ KASSERT(!mtx_owned(&Giant), ("nfssvc_nfsd: Giant when done"));
TAILQ_REMOVE(&nfsd_head, nfsd, nfsd_chain);
splx(s);
free((caddr_t)nfsd, M_NFSD);
Index: sys/kernel.h
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/sys/kernel.h,v
retrieving revision 1.135
diff -u -r1.135 kernel.h
--- sys/kernel.h 9 Apr 2007 22:29:13 -0000 1.135
+++ sys/kernel.h 22 Jul 2007 17:35:20 -0000
@@ -343,11 +343,6 @@
#define TUNABLE_STR_FETCH(path, var, size) \
getenv_string((path), (var), (size))
-void net_warn_not_mpsafe(const char *component);
-#define NET_NEEDS_GIANT(component) \
- SYSINIT(__CONCAT(__net_warn_not_mpsafe_, __LINE__), \
- SI_SUB_SETTINGS, SI_ORDER_SECOND, net_warn_not_mpsafe, component);
-
struct intr_config_hook {
TAILQ_ENTRY(intr_config_hook) ich_links;
void (*ich_func)(void *arg);
Index: sys/mutex.h
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/sys/mutex.h,v
retrieving revision 1.98
diff -u -r1.98 mutex.h
--- sys/mutex.h 18 Jul 2007 20:46:06 -0000 1.98
+++ sys/mutex.h 20 Jul 2007 11:11:04 -0000
@@ -397,35 +397,18 @@
} while (0)
/*
- * 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. Note that, unlike PICKUP_GIANT() and
- * DROP_GIANT(), these macros directly wrap mutex operations
- * without special recursion handling.
- *
- * This mechanism is intended as temporary until everything of
- * importance is properly locked. Note: the semantics for
- * NET_{LOCK,UNLOCK}_GIANT() are not the same as DROP_GIANT()
- * and PICKUP_GIANT(), as they are plain mutex operations
- * without a recursion counter.
+ * With the advent of fine-grained locking, the Giant lock is no longer
+ * required around the network stack. These macros exist for historical
+ * reasons, allowing conditional acquisition of Giant based on a debugging
+ * setting, and will be removed.
*/
-extern int debug_mpsafenet; /* defined in net/netisr.c */
#define NET_LOCK_GIANT() do { \
- if (!debug_mpsafenet) \
- mtx_lock(&Giant); \
} while (0)
#define NET_UNLOCK_GIANT() do { \
- if (!debug_mpsafenet) \
- mtx_unlock(&Giant); \
} while (0)
#define NET_ASSERT_GIANT() do { \
- if (!debug_mpsafenet) \
- mtx_assert(&Giant, MA_OWNED); \
} while (0)
-#define NET_CALLOUT_MPSAFE (debug_mpsafenet ? CALLOUT_MPSAFE : 0)
+#define NET_CALLOUT_MPSAFE CALLOUT_MPSAFE
struct mtx_args {
struct mtx *ma_mtx;
More information about the freebsd-arch
mailing list