Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 14 Jul 2004 00:30:40 -0400 (EDT)
From:      Robert Watson <rwatson@FreeBSD.org>
To:        current@FreeBSD.org
Subject:   Some netgraph node global locking patches
Message-ID:  <Pine.NEB.3.96L.1040714002717.83353E-100000@fledge.watson.org>

next in thread | raw e-mail | index | archive | help

I'm starting to work my way through locking for the various netgraph nodes
shipped with FreeBSD, but have a problem in that I can't easily configure
and test all of them.  As such, I've done some initial global variable
locking, and am working on tricking various FreeBSD developers into
locking down per-node/per-instance variables, etc.

The attached patch locks down global variables found in the following node
implementations:

  //depot/vendor/freebsd/src/sys/netgraph/ng_eiface.c
  //depot/vendor/freebsd/src/sys/netgraph/ng_fec.c
  //depot/vendor/freebsd/src/sys/netgraph/ng_iface.c
  //depot/vendor/freebsd/src/sys/netgraph/ng_ppp.c
  //depot/vendor/freebsd/src/sys/netgraph/ng_tty.c

If people using these netgraph modules could give the patch a spin, that
would be good.  There are a couple of comments regarding locking nits.

If you're the owner of a netgraph module and want to take a lock at
locking down the softc/per-instance variables, I'd appreciate any help I
can get, as there's quite a bit of other locking work for the stack left
in the queue.

Robert N M Watson             FreeBSD Core Team, TrustedBSD Projects
robert@fledge.watson.org      Principal Research Scientist, McAfee Research

--- //depot/vendor/freebsd/src/sys/netgraph/ng_eiface.c	2004/07/04 16:15:35
+++ //depot/user/rwatson/netperf/sys/netgraph/ng_eiface.c	2004/07/09 21:19:53
@@ -33,8 +33,10 @@
 #include <sys/systm.h>
 #include <sys/errno.h>
 #include <sys/kernel.h>
+#include <sys/lock.h>
 #include <sys/malloc.h>
 #include <sys/mbuf.h>
+#include <sys/mutex.h>
 #include <sys/errno.h>
 #include <sys/sockio.h>
 #include <sys/socket.h>
@@ -119,6 +121,8 @@
 
 #define UNITS_BITSPERWORD	(sizeof(*ng_eiface_units) * NBBY)
 
+static struct mtx	ng_eiface_mtx;
+MTX_SYSINIT(ng_eiface, &ng_eiface_mtx, "ng_eiface", MTX_DEF);
 
 /************************************************************************
 			HELPER STUFF
@@ -132,6 +136,7 @@
 {
 	int index, bit;
 
+	mtx_lock(&ng_eiface_mtx);
 	for (index = 0; index < ng_eiface_units_len
 	    && ng_eiface_units[index] == 0; index++);
 	if (index == ng_eiface_units_len) {		/* extend array */
@@ -140,8 +145,10 @@
 		newlen = (2 * ng_eiface_units_len) + 4;
 		MALLOC(newarray, int *, newlen * sizeof(*ng_eiface_units),
 		    M_NETGRAPH, M_NOWAIT);
-		if (newarray == NULL)
+		if (newarray == NULL) {
+			mtx_unlock(&ng_eiface_mtx);
 			return (ENOMEM);
+		}
 		bcopy(ng_eiface_units, newarray,
 		    ng_eiface_units_len * sizeof(*ng_eiface_units));
 		for (i = ng_eiface_units_len; i < newlen; i++)
@@ -157,6 +164,7 @@
 	ng_eiface_units[index] &= ~(1 << bit);
 	*unit = (index * UNITS_BITSPERWORD) + bit;
 	ng_units_in_use++;
+	mtx_unlock(&ng_eiface_mtx);
 	return (0);
 }
 
@@ -170,6 +178,7 @@
 
 	index = unit / UNITS_BITSPERWORD;
 	bit = unit % UNITS_BITSPERWORD;
+	mtx_lock(&ng_eiface_mtx);
 	KASSERT(index < ng_eiface_units_len,
 	    ("%s: unit=%d len=%d", __func__, unit, ng_eiface_units_len));
 	KASSERT((ng_eiface_units[index] & (1 << bit)) == 0,
@@ -187,6 +196,7 @@
 		ng_eiface_units_len = 0;
 		ng_eiface_units = NULL;
 	}
+	mtx_unlock(&ng_eiface_mtx);
 }
 
 /************************************************************************
--- //depot/vendor/freebsd/src/sys/netgraph/ng_fec.c	2004/07/04 16:15:35
+++ //depot/user/rwatson/netperf/sys/netgraph/ng_fec.c	2004/07/09 21:19:53
@@ -257,6 +257,9 @@
 
 #define UNITS_BITSPERWORD	(sizeof(*ng_fec_units) * NBBY)
 
+static struct mtx	ng_fec_mtx;
+MTX_SYSINIT(ng_fec, &ng_fec_mtx, "ng_fec", MTX_DEF);
+
 /*
  * Find the first free unit number for a new interface.
  * Increase the size of the unit bitmap as necessary.
@@ -266,6 +269,7 @@
 {
 	int index, bit;
 
+	mtx_lock(&ng_fec_mtx);
 	for (index = 0; index < ng_fec_units_len
 	    && ng_fec_units[index] == 0; index++);
 	if (index == ng_fec_units_len) {		/* extend array */
@@ -274,8 +278,10 @@
 		newlen = (2 * ng_fec_units_len) + 4;
 		MALLOC(newarray, int *, newlen * sizeof(*ng_fec_units),
 		    M_NETGRAPH, M_NOWAIT);
-		if (newarray == NULL)
+		if (newarray == NULL) {
+			mtx_unlock(&ng_fec_mtx);
 			return (ENOMEM);
+		}
 		bcopy(ng_fec_units, newarray,
 		    ng_fec_units_len * sizeof(*ng_fec_units));
 		for (i = ng_fec_units_len; i < newlen; i++)
@@ -291,6 +297,7 @@
 	ng_fec_units[index] &= ~(1 << bit);
 	*unit = (index * UNITS_BITSPERWORD) + bit;
 	ng_units_in_use++;
+	mtx_unlock(&ng_fec_mtx);
 	return (0);
 }
 
@@ -304,6 +311,7 @@
 
 	index = unit / UNITS_BITSPERWORD;
 	bit = unit % UNITS_BITSPERWORD;
+	mtx_lock(&ng_fec_mtx);
 	KASSERT(index < ng_fec_units_len,
 	    ("%s: unit=%d len=%d", __FUNCTION__, unit, ng_fec_units_len));
 	KASSERT((ng_fec_units[index] & (1 << bit)) == 0,
@@ -321,6 +329,7 @@
 		ng_fec_units_len = 0;
 		ng_fec_units = NULL;
 	}
+	mtx_unlock(&ng_fec_mtx);
 }
 
 /************************************************************************
--- //depot/vendor/freebsd/src/sys/netgraph/ng_iface.c	2004/07/04 16:15:35
+++ //depot/user/rwatson/netperf/sys/netgraph/ng_iface.c	2004/07/09 21:19:53
@@ -59,8 +59,10 @@
 #include <sys/systm.h>
 #include <sys/errno.h>
 #include <sys/kernel.h>
+#include <sys/lock.h>
 #include <sys/malloc.h>
 #include <sys/mbuf.h>
+#include <sys/mutex.h>
 #include <sys/errno.h>
 #include <sys/random.h>
 #include <sys/sockio.h>
@@ -218,6 +220,9 @@
 
 #define UNITS_BITSPERWORD	(sizeof(*ng_iface_units) * NBBY)
 
+static struct mtx	ng_iface_mtx;
+MTX_SYSINIT(ng_iface, &ng_iface_mtx, "ng_iface", MTX_DEF);
+
 /************************************************************************
 			HELPER STUFF
  ************************************************************************/
@@ -289,6 +294,7 @@
 {
 	int index, bit;
 
+	mtx_lock(&ng_iface_mtx);
 	for (index = 0; index < ng_iface_units_len
 	    && ng_iface_units[index] == 0; index++);
 	if (index == ng_iface_units_len) {		/* extend array */
@@ -297,8 +303,10 @@
 		newlen = (2 * ng_iface_units_len) + 4;
 		MALLOC(newarray, int *, newlen * sizeof(*ng_iface_units),
 		    M_NETGRAPH_IFACE, M_NOWAIT);
-		if (newarray == NULL)
+		if (newarray == NULL) {
+			mtx_unlock(&ng_iface_mtx);
 			return (ENOMEM);
+		}
 		bcopy(ng_iface_units, newarray,
 		    ng_iface_units_len * sizeof(*ng_iface_units));
 		for (i = ng_iface_units_len; i < newlen; i++)
@@ -314,6 +322,7 @@
 	ng_iface_units[index] &= ~(1 << bit);
 	*unit = (index * UNITS_BITSPERWORD) + bit;
 	ng_units_in_use++;
+	mtx_unlock(&ng_iface_mtx);
 	return (0);
 }
 
@@ -327,6 +336,7 @@
 
 	index = unit / UNITS_BITSPERWORD;
 	bit = unit % UNITS_BITSPERWORD;
+	mtx_lock(&ng_iface_mtx);
 	KASSERT(index < ng_iface_units_len,
 	    ("%s: unit=%d len=%d", __func__, unit, ng_iface_units_len));
 	KASSERT((ng_iface_units[index] & (1 << bit)) == 0,
@@ -344,6 +354,7 @@
 		ng_iface_units_len = 0;
 		ng_iface_units = NULL;
 	}
+	mtx_unlock(&ng_iface_mtx);
 }
 
 /************************************************************************
--- //depot/vendor/freebsd/src/sys/netgraph/ng_ppp.c	2004/06/26 22:25:30
+++ //depot/user/rwatson/netperf/sys/netgraph/ng_ppp.c	2004/06/27 16:41:59
@@ -362,6 +362,13 @@
 
 static int *compareLatencies;			/* hack for ng_ppp_intcmp() */
 
+/*
+ * XXXRW: An ugly synchronization hack to protect an ugly hack.
+ */
+static struct mtx	ng_ppp_latencies_mtx;
+MTX_SYSINIT(ng_ppp_latencies, &ng_ppp_latencies_mtx, "ng_ppp_latencies",
+    MTX_DEF);
+
 /* Address and control field header */
 static const u_char ng_ppp_acf[2] = { 0xff, 0x03 };
 
@@ -1764,10 +1771,12 @@
 	}
 
 	/* Sort active links by latency */
+	mtx_lock(&ng_ppp_latencies_mtx);
 	compareLatencies = latency;
 	qsort(sortByLatency,
 	    priv->numActiveLinks, sizeof(*sortByLatency), ng_ppp_intcmp);
 	compareLatencies = NULL;
+	mtx_unlock(&ng_ppp_latencies_mtx);
 
 	/* Find the interval we need (add links in sortByLatency[] order) */
 	for (numFragments = 1;
@@ -1858,6 +1867,8 @@
 	const int index1 = *((const int *) v1);
 	const int index2 = *((const int *) v2);
 
+	mtx_assert(&ng_ppp_latencies_mtx, MA_OWNED);
+
 	return compareLatencies[index1] - compareLatencies[index2];
 }
 
--- //depot/vendor/freebsd/src/sys/netgraph/ng_pppoe.c	2004/06/26 22:25:30
+++ //depot/user/rwatson/netperf/sys/netgraph/ng_pppoe.c	2004/06/27 16:41:59
@@ -238,6 +238,10 @@
 };
 typedef struct PPPOE *priv_p;
 
+/*
+ * XXXRW: Leave this unsynchronized, since only a single field is modified,
+ * and it's done so infrequently.  Likewise, pppoe_mode.
+ */
 struct ether_header eh_prototype =
 	{{0xff,0xff,0xff,0xff,0xff,0xff},
 	 {0x00,0x00,0x00,0x00,0x00,0x00},
--- //depot/vendor/freebsd/src/sys/netgraph/ng_tty.c	2004/06/26 08:45:27
+++ //depot/user/rwatson/netperf/sys/netgraph/ng_tty.c	2004/06/27 03:09:51
@@ -166,10 +166,18 @@
 };
 NETGRAPH_INIT(tty, &typestruct);
 
+/*
+ * XXXRW: ngt_unit is protected by ng_tty_mtx.  ngt_ldisc is constant once
+ * ng_tty is initialized.  ngt_nodeop_ok is untouched, and might want to be a
+ * sleep lock in the future?
+ */
 static int ngt_unit;
 static int ngt_nodeop_ok;	/* OK to create/remove node */
 static int ngt_ldisc;
 
+static struct mtx	ng_tty_mtx;
+MTX_SYSINIT(ng_tty, &ng_tty_mtx, "ng_tty", MTX_DEF);
+
 /******************************************************************
 		    LINE DISCIPLINE METHODS
 ******************************************************************/
@@ -214,7 +222,9 @@
 		FREE(sc, M_NETGRAPH);
 		goto done;
 	}
+	mtx_lock(&ng_tty_mtx);
 	snprintf(name, sizeof(name), "%s%d", typestruct.name, ngt_unit++);
+	mtx_unlock(&ng_tty_mtx);
 
 	/* Assign node its name */
 	if ((error = ng_name_node(sc->node, name))) {



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.NEB.3.96L.1040714002717.83353E-100000>