From owner-freebsd-current@FreeBSD.ORG Wed Jul 14 04:30:57 2004 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 1814516A4CE for ; Wed, 14 Jul 2004 04:30:57 +0000 (GMT) Received: from fledge.watson.org (fledge.watson.org [204.156.12.50]) by mx1.FreeBSD.org (Postfix) with ESMTP id 8D31443D1F for ; Wed, 14 Jul 2004 04:30:56 +0000 (GMT) (envelope-from robert@fledge.watson.org) Received: from fledge.watson.org (localhost [127.0.0.1]) by fledge.watson.org (8.12.11/8.12.11) with ESMTP id i6E4UesO017867 for ; Wed, 14 Jul 2004 00:30:40 -0400 (EDT) (envelope-from robert@fledge.watson.org) Received: from localhost (robert@localhost)i6E4UejP017864 for ; Wed, 14 Jul 2004 00:30:40 -0400 (EDT) (envelope-from robert@fledge.watson.org) Date: Wed, 14 Jul 2004 00:30:40 -0400 (EDT) From: Robert Watson X-Sender: robert@fledge.watson.org To: current@FreeBSD.org Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Subject: Some netgraph node global locking patches X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Jul 2004 04:30:57 -0000 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 #include #include +#include #include #include +#include #include #include #include @@ -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 #include #include +#include #include #include +#include #include #include #include @@ -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))) {