From owner-freebsd-current@FreeBSD.ORG Wed Feb 15 21:15:39 2006 Return-Path: X-Original-To: current@freebsd.org 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 651BE16A420 for ; Wed, 15 Feb 2006 21:15:39 +0000 (GMT) (envelope-from thompsa@freebsd.org) Received: from dbmail-mx1.orcon.net.nz (loadbalancer1.orcon.net.nz [219.88.242.3]) by mx1.FreeBSD.org (Postfix) with ESMTP id 8ABC143D45 for ; Wed, 15 Feb 2006 21:15:38 +0000 (GMT) (envelope-from thompsa@freebsd.org) Received-SPF: none Received: from heff.fud.org.nz (60-234-149-201.bitstream.orcon.net.nz [60.234.149.201]) by dbmail-mx1.orcon.net.nz (8.13.2/8.13.2/Debian-1) with ESMTP id k1FLFs3l013599 for ; Thu, 16 Feb 2006 10:15:54 +1300 Received: by heff.fud.org.nz (Postfix, from userid 1001) id E946E1CC38; Thu, 16 Feb 2006 10:15:34 +1300 (NZDT) Date: Thu, 16 Feb 2006 10:15:34 +1300 From: Andrew Thompson To: FreeBSD Current Message-ID: <20060215211534.GA78376@heff.fud.org.nz> Mail-Followup-To: Andrew Thompson , FreeBSD Current Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="W/nzBZO5zC0uMSeA" Content-Disposition: inline User-Agent: Mutt/1.5.11 X-Virus-Scanned: ClamAV version 0.88, clamav-milter version 0.87 on dbmail-mx1.orcon.net.nz X-Virus-Status: Clean Cc: Subject: rwlock patch for bridge X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 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, 15 Feb 2006 21:15:39 -0000 --W/nzBZO5zC0uMSeA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi, Here is a patch that changes if_bridge to use rwlock(9) rather than the handrolled ref counting. Can I please get it reviewed to ensure I have the changes correct. I pondered if the order of unlocking the softc mutex and grabbing the rlock mattered but decided it didn't. It has passed a runtime test. cheers, Andrew --W/nzBZO5zC0uMSeA Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="bridge_rwlock.diff" Index: if_bridge.c =================================================================== RCS file: /home/ncvs/src/sys/net/if_bridge.c,v retrieving revision 1.54 diff -u -p -r1.54 if_bridge.c --- if_bridge.c 3 Feb 2006 23:03:07 -0000 1.54 +++ if_bridge.c 15 Feb 2006 01:35:52 -0000 @@ -786,9 +786,9 @@ bridge_delete_member(struct bridge_softc } ifs->if_bridge = NULL; - BRIDGE_XLOCK(sc); + BRIDGE_WLOCK(sc); LIST_REMOVE(bif, bif_next); - BRIDGE_XDROP(sc); + BRIDGE_WUNLOCK(sc); bridge_rtdelete(sc, ifs, IFBF_FLUSHALL); @@ -1595,13 +1595,10 @@ bridge_output(struct ifnet *ifp, struct if (dst_if == NULL) { struct bridge_iflist *bif; struct mbuf *mc; - int error = 0, used = 0; + int used = 0; - BRIDGE_LOCK2REF(sc, error); - if (error) { - m_freem(m); - return (0); - } + BRIDGE_UNLOCK(sc); + BRIDGE_RLOCK(sc); bridge_span(sc, m); @@ -1644,7 +1641,7 @@ bridge_output(struct ifnet *ifp, struct } if (used == 0) m_freem(m); - BRIDGE_UNREF(sc); + BRIDGE_RUNLOCK(sc); return (0); } @@ -2045,14 +2042,10 @@ bridge_broadcast(struct bridge_softc *sc struct bridge_iflist *bif; struct mbuf *mc; struct ifnet *dst_if; - int error = 0, used = 0; + int used = 0; - BRIDGE_LOCK_ASSERT(sc); - BRIDGE_LOCK2REF(sc, error); - if (error) { - m_freem(m); - return; - } + BRIDGE_UNLOCK(sc); + BRIDGE_RLOCK(sc); /* Filter on the bridge interface before broadcasting */ if (runfilt && (PFIL_HOOKED(&inet_pfil_hook) @@ -2119,7 +2112,7 @@ bridge_broadcast(struct bridge_softc *sc m_freem(m); out: - BRIDGE_UNREF(sc); + BRIDGE_RUNLOCK(sc); } /* Index: if_bridgevar.h =================================================================== RCS file: /home/ncvs/src/sys/net/if_bridgevar.h,v retrieving revision 1.10 diff -u -p -r1.10 if_bridgevar.h --- if_bridgevar.h 14 Jan 2006 03:51:30 -0000 1.10 +++ if_bridgevar.h 15 Feb 2006 20:38:33 -0000 @@ -75,8 +75,7 @@ */ #include -#include -#include +#include /* * Commands used in the SIOCSDRVSPEC ioctl. Note the lookup of the @@ -270,7 +269,7 @@ struct bridge_softc { struct ifnet *sc_ifp; /* make this an interface */ LIST_ENTRY(bridge_softc) sc_list; struct mtx sc_mtx; - struct cv sc_cv; + struct rwlock sc_rwlock; uint64_t sc_designated_root; uint64_t sc_bridge_id; struct bridge_iflist *sc_root_port; @@ -294,8 +293,6 @@ struct bridge_softc { uint32_t sc_brttimeout; /* rt timeout in seconds */ struct callout sc_brcallout; /* bridge callout */ struct callout sc_bstpcallout; /* STP callout */ - uint32_t sc_iflist_ref; /* refcount for sc_iflist */ - uint32_t sc_iflist_xcnt; /* refcount for sc_iflist */ LIST_HEAD(, bridge_iflist) sc_iflist; /* member interface list */ LIST_HEAD(, bridge_rtnode) *sc_rthash; /* our forwarding table */ LIST_HEAD(, bridge_rtnode) sc_rtlist; /* list version of above */ @@ -305,41 +302,20 @@ struct bridge_softc { #define BRIDGE_LOCK_INIT(_sc) do { \ mtx_init(&(_sc)->sc_mtx, "if_bridge", NULL, MTX_DEF); \ - cv_init(&(_sc)->sc_cv, "if_bridge_cv"); \ + rw_init(&(_sc)->sc_rwlock, "if_bridge_rw"); \ } while (0) #define BRIDGE_LOCK_DESTROY(_sc) do { \ mtx_destroy(&(_sc)->sc_mtx); \ - cv_destroy(&(_sc)->sc_cv); \ + rw_destroy(&(_sc)->sc_rwlock); \ } while (0) #define BRIDGE_LOCK(_sc) mtx_lock(&(_sc)->sc_mtx) #define BRIDGE_UNLOCK(_sc) mtx_unlock(&(_sc)->sc_mtx) #define BRIDGE_LOCKED(_sc) mtx_owned(&(_sc)->sc_mtx) #define BRIDGE_LOCK_ASSERT(_sc) mtx_assert(&(_sc)->sc_mtx, MA_OWNED) -#define BRIDGE_LOCK2REF(_sc, _err) do { \ - mtx_assert(&(_sc)->sc_mtx, MA_OWNED); \ - if ((_sc)->sc_iflist_xcnt > 0) \ - (_err) = EBUSY; \ - else \ - (_sc)->sc_iflist_ref++; \ - mtx_unlock(&(_sc)->sc_mtx); \ -} while (0) -#define BRIDGE_UNREF(_sc) do { \ - mtx_lock(&(_sc)->sc_mtx); \ - (_sc)->sc_iflist_ref--; \ - if (((_sc)->sc_iflist_xcnt > 0) && ((_sc)->sc_iflist_ref == 0)) \ - cv_broadcast(&(_sc)->sc_cv); \ - mtx_unlock(&(_sc)->sc_mtx); \ -} while (0) -#define BRIDGE_XLOCK(_sc) do { \ - mtx_assert(&(_sc)->sc_mtx, MA_OWNED); \ - (_sc)->sc_iflist_xcnt++; \ - while ((_sc)->sc_iflist_ref > 0) \ - cv_wait(&(_sc)->sc_cv, &(_sc)->sc_mtx); \ -} while (0) -#define BRIDGE_XDROP(_sc) do { \ - mtx_assert(&(_sc)->sc_mtx, MA_OWNED); \ - (_sc)->sc_iflist_xcnt--; \ -} while (0) +#define BRIDGE_RLOCK(_sc) rw_rlock(&(_sc)->sc_rwlock) +#define BRIDGE_WLOCK(_sc) rw_wlock(&(_sc)->sc_rwlock) +#define BRIDGE_RUNLOCK(_sc) rw_runlock(&(_sc)->sc_rwlock) +#define BRIDGE_WUNLOCK(_sc) rw_wunlock(&(_sc)->sc_rwlock) #define BRIDGE_INPUT(_ifp, _m) do { \ KASSERT(bridge_input_p != NULL, \ --W/nzBZO5zC0uMSeA--