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
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
