Date: Thu, 29 Dec 2011 15:27:26 -0500 From: John Baldwin <jhb@freebsd.org> To: freebsd-net@freebsd.org, Robert Watson <rwatson@freebsd.org> Subject: Re: Transitioning if_addr_lock to an rwlock Message-ID: <201112291527.26763.jhb@freebsd.org> In-Reply-To: <201112221130.01823.jhb@freebsd.org> References: <201112221130.01823.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201112291527.26763.jhb>