Date: Fri, 6 Jan 2012 15:39:55 -0500 From: John Baldwin <jhb@freebsd.org> To: freebsd-net@freebsd.org Cc: "Bjoern A. Zeeb" <bz@freebsd.org>, Robert Watson <rwatson@freebsd.org> Subject: Re: Transitioning if_addr_lock to an rwlock Message-ID: <201201061539.55984.jhb@freebsd.org> In-Reply-To: <201112291527.26763.jhb@freebsd.org> References: <201112221130.01823.jhb@freebsd.org> <201112291527.26763.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, December 29, 2011 3:27:26 pm John Baldwin wrote: > On Thursday, December 22, 2011 11:30:01 am John Baldwin wrote: > > Another bit of lock contention I ran into between a device driver doing slow > > MAC filter updates and the receive path is IF_ADDR_LOCK(). NIC device drivers > > typically hold this lock while iterating the list of multicast addresses to > > program their MAC filter. OTOH, ip_input() uses this lock to check to see if > > an incoming packet is a broadcast packet or not. So even with the pcbinfo > > contention from my previous patch addressed, I still ran into a problem with > > IF_ADDR_LOCK(). We already have some partial support for making this lock be > > an rwlock (the APIs that drivers now use implies an rlock), so I finished the > > transition and checked various non-driver users to see which ones could use a > > read lock (most uses can). The current patch I have for this is on 8, but if > > folks think this is a good idea I can work on porting it to HEAD. For HEAD > > the strategy I would use would be to split this up into 2 phases: > > > > 1) Add IF_ADDR locking macros to differentiate read locks vs write locks along > > with appropriate assertion macros. Update current users of the locking > > macros to use either read or write locks explicitly. To preserve KPI, > > the existing LOCK/UNLOCK macros would map to write lock operations. In > > the initial patch, the locking would still be implemented via a mtx. > > > > 2) Replace the mutex with an rwlock and update the locking macros as > > appropriate. > > > > Phase 1 should definitely be MFC'able. Phase 2 may or may not be. Robert had > > the foresight to change drivers to use explicit function wrappers around > > IF_ADDR_LOCK, and sizeof(struct mtx) == sizeof(struct rwlock), so if we > > changed the lock type the KBI for existing device drivers would all be fine. > > Most of the remaining uses of the locking macros are in parts of the kernel > > that aren't loadable (such as inet and inet6). We can look at the places that > > to do change and if any of them are in kld's then it would be up to re@ to > > decide if 2) was actually safe to merge. However, even if Phase 2 cannot be > > MFC'd, having phase 1 makes it easier for downstream consumers to apply Phase > > 2 locally if they need it. > > I've gone ahead with this approach. I have three separate patches that should > implement Phase 1. All of them can be found at > http://www.FreeBSD.org/~jhb/patches/ > > - if_addr_dev.patch This fixes a few new device drivers that were using > the locking macros directly rather than the wrapper > functions Robert added. I've already sent this > directly to the relevant driver maintainers for their > review. > - if_addr_macros.patch This adds new locking macros to support read locks vs > write locks. However, they all still map to mutex > operations. > - if_addr_uses.patch This changes callers of the existing macros to use > either read or write locks. This is the patch that > could use the most review. Now that all of this is in the tree, here is the small patch to cut the locks over to rwlocks rather than mutexes: Index: kern/subr_witness.c =================================================================== --- kern/subr_witness.c (revision 229726) +++ kern/subr_witness.c (working copy) @@ -520,7 +520,7 @@ { "udpinp", &lock_class_rw }, { "in_multi_mtx", &lock_class_mtx_sleep }, { "igmp_mtx", &lock_class_mtx_sleep }, - { "if_addr_mtx", &lock_class_mtx_sleep }, + { "if_addr_lock", &lock_class_rw }, { NULL, NULL }, /* * IPv6 multicast: @@ -529,7 +529,7 @@ { "udpinp", &lock_class_rw }, { "in6_multi_mtx", &lock_class_mtx_sleep }, { "mld_mtx", &lock_class_mtx_sleep }, - { "if_addr_mtx", &lock_class_mtx_sleep }, + { "if_addr_lock", &lock_class_rw }, { NULL, NULL }, /* * UNIX Domain Sockets Index: net/if_var.h =================================================================== --- net/if_var.h (revision 229726) +++ net/if_var.h (working copy) @@ -189,7 +189,7 @@ int if_afdata_initialized; struct rwlock if_afdata_lock; struct task if_linktask; /* task for link change events */ - struct mtx if_addr_mtx; /* mutex to protect address lists */ + struct rwlock if_addr_lock; /* lock to protect address lists */ LIST_ENTRY(ifnet) if_clones; /* interfaces of a cloner */ TAILQ_HEAD(, ifg_list) if_groups; /* linked list of groups per if */ @@ -246,15 +246,14 @@ /* * Locks for address lists on the network interface. */ -#define IF_ADDR_LOCK_INIT(if) mtx_init(&(if)->if_addr_mtx, \ - "if_addr_mtx", NULL, MTX_DEF) -#define IF_ADDR_LOCK_DESTROY(if) mtx_destroy(&(if)->if_addr_mtx) -#define IF_ADDR_WLOCK(if) mtx_lock(&(if)->if_addr_mtx) -#define IF_ADDR_WUNLOCK(if) mtx_unlock(&(if)->if_addr_mtx) -#define IF_ADDR_RLOCK(if) mtx_lock(&(if)->if_addr_mtx) -#define IF_ADDR_RUNLOCK(if) mtx_unlock(&(if)->if_addr_mtx) -#define IF_ADDR_LOCK_ASSERT(if) mtx_assert(&(if)->if_addr_mtx, MA_OWNED) -#define IF_ADDR_WLOCK_ASSERT(if) mtx_assert(&(if)->if_addr_mtx, MA_OWNED) +#define IF_ADDR_LOCK_INIT(if) rw_init(&(if)->if_addr_lock, "if_addr_lock") +#define IF_ADDR_LOCK_DESTROY(if) rw_destroy(&(if)->if_addr_lock) +#define IF_ADDR_WLOCK(if) rw_wlock(&(if)->if_addr_lock) +#define IF_ADDR_WUNLOCK(if) rw_wunlock(&(if)->if_addr_lock) +#define IF_ADDR_RLOCK(if) rw_rlock(&(if)->if_addr_lock) +#define IF_ADDR_RUNLOCK(if) rw_runlock(&(if)->if_addr_lock) +#define IF_ADDR_LOCK_ASSERT(if) rw_assert(&(if)->if_addr_lock, RA_LOCKED) +#define IF_ADDR_WLOCK_ASSERT(if) rw_assert(&(if)->if_addr_lock, RA_WLOCKED) /* XXX: Compat. */ #define IF_ADDR_LOCK(if) IF_ADDR_WLOCK(if) #define IF_ADDR_UNLOCK(if) IF_ADDR_WUNLOCK(if) -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201201061539.55984.jhb>