Somewhat re-factor the read/write locking mechanism associated with the packet

filtering mechanisms to use the new rwlock(9) locking API:

- Drop the variables stored in the phil_head structure which were specific to
  conditions and the home rolled read/write locking mechanism.
- Drop some includes which were used for condition variables
- Drop the inline functions, and convert them to macros. Also, move these
  macros into pfil.h
- Move pfil list locking macros intp phil.h as well
- Rename ph_busy_count to ph_nhooks. This variable will represent the number
  of IN/OUT hooks registered with the pfil head structure
- Define PFIL_HOOKED macro which evaluates to true if there are any
  hooks to be ran by pfil_run_hooks
- In the IP/IP6 stacks, change the ph_busy_count comparison to use the new
  PFIL_HOOKED macro.
- Drop optimization in pfil_run_hooks which checks to see if there are any
  hooks to be ran, and returns if not. This check is already performed by the
  IP stacks when they call:

        if (!PFIL_HOOKED(ph))
                goto skip_hooks;

- Drop in assertion which makes sure that the number of hooks never drops
  below 0 for good measure. This in theory should never happen, and if it
  does than there are problems somewhere
- Drop special logic around PFIL_WAITOK because rw_wlock(9) does not sleep
- Drop variables which support home rolled read/write locking mechanism from
  the IPFW firewall chain structure.
- Swap out the read/write firewall chain lock internal to use the rwlock(9)
  API instead of our home rolled version
- Convert the inlined functions to macros

Reviewed by:	mlaier, andre, glebius
Thanks to:	jhb for the new locking API

9 files changed, 50 insertions, 163 deletions

diff --git a/sys/net/pfil.c b/sys/net/pfil.c
index 35ac338..78b87f3 100644
--- a/sys/net/pfil.c
+++ b/sys/net/pfil.c
@@ -32,7 +32,9 @@
 #include <sys/param.h>
 #include <sys/kernel.h>
 #include <sys/errno.h>
+#include <sys/lock.h>
 #include <sys/malloc.h>
+#include <sys/rwlock.h>
 #include <sys/socket.h>
 #include <sys/socketvar.h>
 #include <sys/systm.h>
@@ -57,57 +59,6 @@ static int pfil_list_remove(pfil_list_t *,
 LIST_HEAD(, pfil_head) pfil_head_list =
     LIST_HEAD_INITIALIZER(&pfil_head_list);
 
-static __inline void
-PFIL_RLOCK(struct pfil_head *ph)
-{
-	mtx_lock(&ph->ph_mtx);
-	ph->ph_busy_count++;
-	mtx_unlock(&ph->ph_mtx);
-}
-
-static __inline void
-PFIL_RUNLOCK(struct pfil_head *ph)
-{
-	mtx_lock(&ph->ph_mtx);
-	ph->ph_busy_count--;
-	if (ph->ph_busy_count == 0 && ph->ph_want_write)
-		cv_signal(&ph->ph_cv);
-	mtx_unlock(&ph->ph_mtx);
-}
-
-static __inline void
-PFIL_WLOCK(struct pfil_head *ph)
-{
-	mtx_lock(&ph->ph_mtx);
-	ph->ph_want_write = 1;
-	while (ph->ph_busy_count > 0)
-		cv_wait(&ph->ph_cv, &ph->ph_mtx);
-}
-
-static __inline int
-PFIL_TRY_WLOCK(struct pfil_head *ph)
-{
-	mtx_lock(&ph->ph_mtx);
-	ph->ph_want_write = 1;
-	if (ph->ph_busy_count > 0) {
-		ph->ph_want_write = 0;
-		mtx_unlock(&ph->ph_mtx);
-		return EBUSY;
-	}
-	return 0;
-}
-
-static __inline void
-PFIL_WUNLOCK(struct pfil_head *ph)
-{
-	ph->ph_want_write = 0;
-	cv_signal(&ph->ph_cv);
-	mtx_unlock(&ph->ph_mtx);
-}
-
-#define PFIL_LIST_LOCK() mtx_lock(&pfil_global_lock)
-#define PFIL_LIST_UNLOCK() mtx_unlock(&pfil_global_lock)
-
 /*
  * pfil_run_hooks() runs the specified packet filter hooks.
  */
@@ -119,20 +70,8 @@ pfil_run_hooks(struct pfil_head *ph, struct mbuf **mp, struct ifnet *ifp,
 	struct mbuf *m = *mp;
 	int rv = 0;
 
-	if (ph->ph_busy_count == -1)
-		return (0);
-	/*
-	 * Prevent packet filtering from starving the modification of
-	 * the packet filters. We would prefer a reader/writer locking
-	 * mechanism with guaranteed ordering, though.
-	 */
-	if (ph->ph_want_write) {
-		m_freem(*mp);
-		*mp = NULL;
-		return (ENOBUFS);
-	}
-
 	PFIL_RLOCK(ph);
+	KASSERT(ph->ph_nhooks >= 0, ("Pfil hook count dropped < 0"));
 	for (pfh = pfil_hook_get(dir, ph); pfh != NULL;
 	     pfh = TAILQ_NEXT(pfh, pfil_link)) {
 		if (pfh->pfil_func != NULL) {
@@ -165,16 +104,9 @@ pfil_head_register(struct pfil_head *ph)
 		}
 	PFIL_LIST_UNLOCK();
 
-	if (mtx_initialized(&ph->ph_mtx)) {	/* should not happen */
-		KASSERT((0), ("%s: allready initialized!", __func__));
-		return EBUSY;
-	} else {
-		ph->ph_busy_count = -1;
-		ph->ph_want_write = 1;
-		mtx_init(&ph->ph_mtx, "pfil_head_mtx", NULL, MTX_DEF);
-		cv_init(&ph->ph_cv, "pfil_head_cv");
-		mtx_lock(&ph->ph_mtx);			/* XXX: race? */
-	}
+	rw_init(&ph->ph_mtx, "PFil hook read/write mutex");
+	PFIL_WLOCK(ph);
+	ph->ph_nhooks = 0;
 
 	TAILQ_INIT(&ph->ph_in);
 	TAILQ_INIT(&ph->ph_out);
@@ -182,9 +114,9 @@ pfil_head_register(struct pfil_head *ph)
 	PFIL_LIST_LOCK();
 	LIST_INSERT_HEAD(&pfil_head_list, ph, ph_list);
 	PFIL_LIST_UNLOCK();
-	
+
 	PFIL_WUNLOCK(ph);
-	
+
 	return (0);
 }
 
@@ -205,14 +137,13 @@ pfil_head_unregister(struct pfil_head *ph)
 	LIST_REMOVE(ph, ph_list);
 	PFIL_LIST_UNLOCK();
 
-	PFIL_WLOCK(ph);			/* XXX: may sleep (cv_wait)! */
+	PFIL_WLOCK(ph);
 	
 	TAILQ_FOREACH_SAFE(pfh, &ph->ph_in, pfil_link, pfnext)
 		free(pfh, M_IFADDR);
 	TAILQ_FOREACH_SAFE(pfh, &ph->ph_out, pfil_link, pfnext)
 		free(pfh, M_IFADDR);
-	cv_destroy(&ph->ph_cv);
-	mtx_destroy(&ph->ph_mtx);
+	rw_destroy(&ph->ph_mtx);
 	
 	return (0);
 }
@@ -269,13 +200,7 @@ pfil_add_hook(int (*func)(void *, struct mbuf **, struct ifnet *, int, struct in
 	}
 
 	/* Lock */
-	if (flags & PFIL_WAITOK)
-		PFIL_WLOCK(ph);
-	else {
-		err = PFIL_TRY_WLOCK(ph);
-		if (err)
-			goto error;
-	}
+	PFIL_WLOCK(ph);
 
 	/* Add */
 	if (flags & PFIL_IN) {
@@ -284,6 +209,7 @@ pfil_add_hook(int (*func)(void *, struct mbuf **, struct ifnet *, int, struct in
 		err = pfil_list_add(&ph->ph_in, pfh1, flags & ~PFIL_OUT);
 		if (err)
 			goto done;
+		ph->ph_nhooks++;
 	}
 	if (flags & PFIL_OUT) {
 		pfh2->pfil_func = func;
@@ -294,9 +220,9 @@ pfil_add_hook(int (*func)(void *, struct mbuf **, struct ifnet *, int, struct in
 				pfil_list_remove(&ph->ph_in, func, arg);
 			goto done;
 		}
+		ph->ph_nhooks++;
 	}
 
-	ph->ph_busy_count = 0;
 	PFIL_WUNLOCK(ph);
 
 	return 0;
@@ -320,22 +246,18 @@ pfil_remove_hook(int (*func)(void *, struct mbuf **, struct ifnet *, int, struct
 {
 	int err = 0;
 
-	if (flags & PFIL_WAITOK)
-		PFIL_WLOCK(ph);
-	else {
-		err = PFIL_TRY_WLOCK(ph);
-		if (err)
-			return err;
-	}
+	PFIL_WLOCK(ph);
 
-	if (flags & PFIL_IN)
+	if (flags & PFIL_IN) {
 		err = pfil_list_remove(&ph->ph_in, func, arg);
-	if ((err == 0) && (flags & PFIL_OUT))
+		if (err == 0)
+			ph->ph_nhooks--;
+	}
+	if ((err == 0) && (flags & PFIL_OUT)) {
 		err = pfil_list_remove(&ph->ph_out, func, arg);
-
-	if (TAILQ_EMPTY(&ph->ph_in) && TAILQ_EMPTY(&ph->ph_out))
-		ph->ph_busy_count = -1;
-
+		if (err == 0)
+			ph->ph_nhooks--;
+	}
 	PFIL_WUNLOCK(ph);
 	
 	return err;
diff --git a/sys/net/pfil.h b/sys/net/pfil.h
index da14f5b..ccf7a65 100644
--- a/sys/net/pfil.h
+++ b/sys/net/pfil.h
@@ -36,7 +36,7 @@
 #include <sys/queue.h>
 #include <sys/_lock.h>
 #include <sys/_mutex.h>
-#include <sys/condvar.h>	/* XXX */
+#include <sys/rwlock.h>
 
 struct mbuf;
 struct ifnet;
@@ -67,14 +67,8 @@ struct pfil_head {
 	pfil_list_t	ph_in;
 	pfil_list_t	ph_out;
 	int		ph_type;
-	/*
-	 * Locking: use a busycounter per pfil_head.
-	 * Use ph_busy_count = -1 to indicate pfil_head is empty.
-	 */
-	int		ph_busy_count;	/* count of threads with read lock */
-	int		ph_want_write;	/* want write lock flag */
-	struct cv	ph_cv;		/* for waking up writers */
-	struct mtx	ph_mtx;		/* mutex on locking state */
+	int		ph_nhooks;
+	struct rwlock	ph_mtx;
 	union {
 		u_long		phu_val;
 		void		*phu_ptr;
@@ -97,11 +91,17 @@ int	pfil_head_unregister(struct pfil_head *);
 
 struct pfil_head *pfil_head_get(int, u_long);
 
+#define	PFIL_HOOKED(p) (&(p)->ph_nhooks > 0)
+#define PFIL_RLOCK(p) rw_rlock(&(p)->ph_mtx)
+#define PFIL_WLOCK(p) rw_wlock(&(p)->ph_mtx)
+#define PFIL_RUNLOCK(p) rw_runlock(&(p)->ph_mtx)
+#define PFIL_WUNLOCK(p) rw_wunlock(&(p)->ph_mtx)
+#define PFIL_LIST_LOCK() mtx_lock(&pfil_global_lock)
+#define PFIL_LIST_UNLOCK() mtx_unlock(&pfil_global_lock)
+
 static __inline struct packet_filter_hook *
 pfil_hook_get(int dir, struct pfil_head *ph)
 {
-	KASSERT(ph->ph_busy_count > 0, 
-	    ("pfil_hook_get: called on unbusy pfil_head"));
 	if (dir == PFIL_IN)
 		return (TAILQ_FIRST(&ph->ph_in));
 	else if (dir == PFIL_OUT)
diff --git a/sys/netinet/ip_fastfwd.c b/sys/netinet/ip_fastfwd.c
index 6ad8e9d..94da84b 100644
--- a/sys/netinet/ip_fastfwd.c
+++ b/sys/netinet/ip_fastfwd.c
@@ -345,7 +345,7 @@ ip_fastforward(struct mbuf *m)
 	/*
 	 * Run through list of ipfilter hooks for input packets
 	 */
-	if (inet_pfil_hook.ph_busy_count == -1)
+	if (!PFIL_HOOKED(&inet_pfil_hook))
 		goto passin;
 
 	if (pfil_run_hooks(&inet_pfil_hook, &m, m->m_pkthdr.rcvif, PFIL_IN, NULL) ||
@@ -430,7 +430,7 @@ passin:
 	/*
 	 * Run through list of hooks for output packets.
 	 */
-	if (inet_pfil_hook.ph_busy_count == -1)
+	if (!PFIL_HOOKED(&inet_pfil_hook))
 		goto passout;
 
 	if (pfil_run_hooks(&inet_pfil_hook, &m, ifp, PFIL_OUT, NULL) || m == NULL) {
diff --git a/sys/netinet/ip_fw2.c b/sys/netinet/ip_fw2.c
index e286951..ea43ece 100644
--- a/sys/netinet/ip_fw2.c
+++ b/sys/netinet/ip_fw2.c
@@ -50,9 +50,11 @@
 #include <sys/malloc.h>
 #include <sys/mbuf.h>
 #include <sys/kernel.h>
+#include <sys/lock.h>
 #include <sys/jail.h>
 #include <sys/module.h>
 #include <sys/proc.h>
+#include <sys/rwlock.h>
 #include <sys/socket.h>
 #include <sys/socketvar.h>
 #include <sys/sysctl.h>
@@ -131,54 +133,20 @@ struct ip_fw_chain {
 	struct ip_fw	*rules;		/* list of rules */
 	struct ip_fw	*reap;		/* list of rules to reap */
 	struct radix_node_head *tables[IPFW_TABLES_MAX];
-	struct mtx	mtx;		/* lock guarding rule list */
-	int		busy_count;	/* busy count for rw locks */
-	int		want_write;
-	struct cv	cv;
+	struct rwlock	rwmtx;
 };
 #define	IPFW_LOCK_INIT(_chain) \
-	mtx_init(&(_chain)->mtx, "IPFW static rules", NULL, \
-		MTX_DEF | MTX_RECURSE)
-#define	IPFW_LOCK_DESTROY(_chain)	mtx_destroy(&(_chain)->mtx)
+	rw_init(&(_chain)->rwmtx, "IPFW static rules")
+#define	IPFW_LOCK_DESTROY(_chain)	rw_destroy(&(_chain)->rwmtx)
 #define	IPFW_WLOCK_ASSERT(_chain)	do {				\
-	mtx_assert(&(_chain)->mtx, MA_OWNED);				\
+	rw_assert(rw, RA_WLOCKED);					\
 	NET_ASSERT_GIANT();						\
 } while (0)
 
-static __inline void
-IPFW_RLOCK(struct ip_fw_chain *chain)
-{
-	mtx_lock(&chain->mtx);
-	chain->busy_count++;
-	mtx_unlock(&chain->mtx);
-}
-
-static __inline void
-IPFW_RUNLOCK(struct ip_fw_chain *chain)
-{
-	mtx_lock(&chain->mtx);
-	chain->busy_count--;
-	if (chain->busy_count == 0 && chain->want_write)
-		cv_signal(&chain->cv);
-	mtx_unlock(&chain->mtx);
-}
-
-static __inline void
-IPFW_WLOCK(struct ip_fw_chain *chain)
-{
-	mtx_lock(&chain->mtx);
-	chain->want_write++;
-	while (chain->busy_count > 0)
-		cv_wait(&chain->cv, &chain->mtx);
-}
-
-static __inline void
-IPFW_WUNLOCK(struct ip_fw_chain *chain)
-{
-	chain->want_write--;
-	cv_signal(&chain->cv);
-	mtx_unlock(&chain->mtx);
-}
+#define IPFW_RLOCK(p) rw_rlock(&(p)->rwmtx)
+#define IPFW_RUNLOCK(p) rw_runlock(&(p)->rwmtx)
+#define IPFW_WLOCK(p) rw_wlock(&(p)->rwmtx)
+#define IPFW_WUNLOCK(p) rw_wunlock(&(p)->rwmtx)
 
 /*
  * list of rules for layer 3
@@ -4155,9 +4123,6 @@ ipfw_init(void)
 #endif
 
 	layer3_chain.rules = NULL;
-	layer3_chain.want_write = 0;
-	layer3_chain.busy_count = 0;
-	cv_init(&layer3_chain.cv, "Condition variable for IPFW rw locks");
 	IPFW_LOCK_INIT(&layer3_chain);
 	ipfw_dyn_rule_zone = uma_zcreate("IPFW dynamic rule zone",
 	    sizeof(ipfw_dyn_rule), NULL, NULL, NULL, NULL,
diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c
index 6f1130c..2250116 100644
--- a/sys/netinet/ip_input.c
+++ b/sys/netinet/ip_input.c
@@ -403,7 +403,7 @@ tooshort:
 	 */
 
 	/* Jump over all PFIL processing if hooks are not active. */
-	if (inet_pfil_hook.ph_busy_count == -1)
+	if (!PFIL_HOOKED(&inet_pfil_hook))
 		goto passin;
 
 	odst = ip->ip_dst;
diff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
index a3fa63b..915512e 100644
--- a/sys/netinet/ip_output.c
+++ b/sys/netinet/ip_output.c
@@ -405,7 +405,7 @@ sendit:
 #endif /* IPSEC */
 
 	/* Jump over all PFIL processing if hooks are not active. */
-	if (inet_pfil_hook.ph_busy_count == -1)
+	if (!PFIL_HOOKED(&inet_pfil_hook))
 		goto passout;
 
 	/* Run through list of hooks for output packets. */
diff --git a/sys/netinet6/ip6_forward.c b/sys/netinet6/ip6_forward.c
index 5041fe8..3b542cf 100644
--- a/sys/netinet6/ip6_forward.c
+++ b/sys/netinet6/ip6_forward.c
@@ -611,7 +611,7 @@ ip6_forward(m, srcrt)
 	in6_clearscope(&ip6->ip6_dst);
 
 	/* Jump over all PFIL processing if hooks are not active. */
-	if (inet6_pfil_hook.ph_busy_count == -1)
+	if (!PFIL_HOOKED(&inet6_pfil_hook))
 		goto pass;
 
 	/* Run through list of hooks for output packets. */
diff --git a/sys/netinet6/ip6_input.c b/sys/netinet6/ip6_input.c
index 3b34557..a7f5a7c 100644
--- a/sys/netinet6/ip6_input.c
+++ b/sys/netinet6/ip6_input.c
@@ -414,7 +414,7 @@ ip6_input(m)
 	odst = ip6->ip6_dst;
 
 	/* Jump over all PFIL processing if hooks are not active. */
-	if (inet6_pfil_hook.ph_busy_count == -1)
+	if (!PFIL_HOOKED(&inet6_pfil_hook))
 		goto passin;
 
 	if (pfil_run_hooks(&inet6_pfil_hook, &m, m->m_pkthdr.rcvif, PFIL_IN, NULL))
diff --git a/sys/netinet6/ip6_output.c b/sys/netinet6/ip6_output.c
index 4eb7a6e..57999f8 100644
--- a/sys/netinet6/ip6_output.c
+++ b/sys/netinet6/ip6_output.c
@@ -889,7 +889,7 @@ again:
 	}
 
 	/* Jump over all PFIL processing if hooks are not active. */
-	if (inet6_pfil_hook.ph_busy_count == -1)
+	if (!PFIL_HOOKED(&inet6_pfil_hook))
 		goto passout;
 
 	odst = ip6->ip6_dst;