Rework the eventhandler locking for hopefully the last time. The scheme

used popped into my head during my morning commute a few weeks ago, but
it is also very similar (though a bit simpler) to a patch that mini@
developed a while ago.  Basically, each eventhandler list has a mutex and
a run count.  During an eventhandler invocation, the mutex is held while
we traverse the list but is dropped while we execute actual handlers.  Also,
a runcount counter is incremented at the start of an invocation and
decremented at the end of an invocation.  Adding to the list is not a big
deal since the reference of a thread currently executing the handlers
remains valid across an add operation.  Whether or not new handlers are
executed by threads currently executing the handlers for a given list is
indeterminate however.  The harder case is when a handler is removed from
the list.  If the runcount is zero, the handler is simply removed from the
list directly.  If the runcount is not zero, then another thread is
currently executing the handlers of this list, so the priority of this
handler is set to a magic value (currently -1) to mark it as dead.  Dead
handlers are not executed during an invocation.  If the runcount is zero
after it is decremented at the end of an invocation, then a new
eventhandler_prune_list() function is called to remove dead handlers from
the list.

Additional minor notes:
- All the common parts of EVENTHANDLER_INVOKE() and
  EVENTHANDLER_FAST_INVOKE() have been merged into a common
  _EVENTHANDLER_INVOKE() macro to reduce duplication and ease maintenance.
- KTR logging for eventhandlers is now available via the KTR_EVH mask.
- The global eventhander_mutex is no longer recursive.

Tested by:	scottl (SMP i386)

1 files changed, 108 insertions, 41 deletions

diff --git a/sys/kern/subr_eventhandler.c b/sys/kern/subr_eventhandler.c
index 27f21f0..a56f1e4 100644
--- a/sys/kern/subr_eventhandler.c
+++ b/sys/kern/subr_eventhandler.c
@@ -48,6 +48,8 @@ struct eventhandler_entry_generic
     void			(* func)(void);
 };
 
+static struct eventhandler_list *_eventhandler_find_list(char *name);
+
 /*
  * Initialize the eventhandler mutex and list.
  */
@@ -55,8 +57,8 @@ static void
 eventhandler_init(void *dummy __unused)
 {
     TAILQ_INIT(&eventhandler_lists);
-    mtx_init(&eventhandler_mutex, "eventhandler", NULL, MTX_DEF | MTX_RECURSE);
-    eventhandler_lists_initted = 1;
+    mtx_init(&eventhandler_mutex, "eventhandler", NULL, MTX_DEF);
+    atomic_store_rel_int(&eventhandler_lists_initted, 1);
 }
 SYSINIT(eventhandlers, SI_SUB_EVENTHANDLER, SI_ORDER_FIRST, eventhandler_init,
     NULL)
@@ -69,6 +71,7 @@ eventhandler_tag
 eventhandler_register(struct eventhandler_list *list, char *name, 
 		      void *func, void *arg, int priority)
 {
+    struct eventhandler_list		*new_list;
     struct eventhandler_entry_generic	*eg;
     struct eventhandler_entry		*ep;
     
@@ -80,51 +83,62 @@ eventhandler_register(struct eventhandler_list *list, char *name,
     /* Do we need to find/create the (slow) list? */
     if (list == NULL) {
 	/* look for a matching, existing list */
-	list = eventhandler_find_list(name);
+	list = _eventhandler_find_list(name);
 
 	/* Do we need to create the list? */
 	if (list == NULL) {
-	    if ((list = malloc(sizeof(struct eventhandler_list) + strlen(name)
-		    + 1, M_EVENTHANDLER, M_NOWAIT)) == NULL) {
-		mtx_unlock(&eventhandler_mutex);
-		return(NULL);
+	    mtx_unlock(&eventhandler_mutex);
+
+	    new_list = malloc(sizeof(struct eventhandler_list) +
+		strlen(name) + 1, M_EVENTHANDLER, M_WAITOK);
+
+	    /* If someone else created it already, then use that one. */
+	    mtx_lock(&eventhandler_mutex);
+	    list = _eventhandler_find_list(name);
+	    if (list != NULL) {
+		free(new_list, M_EVENTHANDLER);
+	    } else {
+		CTR2(KTR_EVH, "%s: creating list \"%s\"", __func__, name);
+		list = new_list;
+		list->el_flags = 0;
+		list->el_runcount = 0;
+		bzero(&list->el_lock, sizeof(list->el_lock));
+		list->el_name = (char *)list + sizeof(struct eventhandler_list);
+		strcpy(list->el_name, name);
+		TAILQ_INSERT_HEAD(&eventhandler_lists, list, el_link);
 	    }
-	    list->el_flags = 0;
-	    bzero(&list->el_lock, sizeof(list->el_lock));
-	    list->el_name = (char *)list + sizeof(struct eventhandler_list);
-	    strcpy(list->el_name, name);
-	    TAILQ_INSERT_HEAD(&eventhandler_lists, list, el_link);
 	}
     }
-    if (!(list->el_flags & EHE_INITTED)) {
+    if (!(list->el_flags & EHL_INITTED)) {
 	TAILQ_INIT(&list->el_entries);
-	sx_init(&list->el_lock, name);
-	list->el_flags = EHE_INITTED;
+	mtx_init(&list->el_lock, name, "eventhandler list", MTX_DEF);
+	atomic_store_rel_int(&list->el_flags, EHL_INITTED);
     }
     mtx_unlock(&eventhandler_mutex);
-    
+
     /* allocate an entry for this handler, populate it */
-    if ((eg = malloc(sizeof(struct eventhandler_entry_generic), 
-		     M_EVENTHANDLER, M_NOWAIT)) == NULL) {
-	return(NULL);
-    }
+    eg = malloc(sizeof(struct eventhandler_entry_generic), M_EVENTHANDLER,
+	M_WAITOK | M_ZERO);
     eg->func = func;
     eg->ee.ee_arg = arg;
     eg->ee.ee_priority = priority;
-    
+    KASSERT(priority != EHE_DEAD_PRIORITY,
+	("%s: handler for %s registered with dead priority", __func__, name));
+
     /* sort it into the list */
-    EHE_LOCK(list);
-    for (ep = TAILQ_FIRST(&list->el_entries);
-	 ep != NULL; 
-	 ep = TAILQ_NEXT(ep, ee_link)) {
-	if (eg->ee.ee_priority < ep->ee_priority) {
+    CTR4(KTR_EVH, "%s: adding item %p (function %p) to \"%s\"", __func__, eg,
+	func, name);
+    EHL_LOCK(list);
+    TAILQ_FOREACH(ep, &list->el_entries, ee_link) {
+	if (ep->ee_priority != EHE_DEAD_PRIORITY &&
+	    eg->ee.ee_priority < ep->ee_priority) {
 	    TAILQ_INSERT_BEFORE(ep, &eg->ee, ee_link);
 	    break;
 	}
     }
     if (ep == NULL)
 	TAILQ_INSERT_TAIL(&list->el_entries, &eg->ee, ee_link);
-    EHE_UNLOCK(list);
+    EHL_UNLOCK(list);
     return(&eg->ee);
 }
 
@@ -133,23 +147,59 @@ eventhandler_deregister(struct eventhandler_list *list, eventhandler_tag tag)
 {
     struct eventhandler_entry	*ep = tag;
 
-    /* XXX insert diagnostic check here? */
-    EHE_LOCK(list);
+    EHL_LOCK_ASSERT(list, MA_OWNED);
     if (ep != NULL) {
 	/* remove just this entry */
-	TAILQ_REMOVE(&list->el_entries, ep, ee_link);
-	free(ep, M_EVENTHANDLER);
+	if (list->el_runcount == 0) {
+	    CTR3(KTR_EVH, "%s: removing item %p from \"%s\"", __func__, ep,
+		list->el_name);
+	    TAILQ_REMOVE(&list->el_entries, ep, ee_link);
+	    free(ep, M_EVENTHANDLER);
+	} else {
+	    CTR3(KTR_EVH, "%s: marking item %p from \"%s\" as dead", __func__,
+		ep, list->el_name);
+	    ep->ee_priority = EHE_DEAD_PRIORITY;
+	}
     } else {
 	/* remove entire list */
-	while (!TAILQ_EMPTY(&list->el_entries)) {
-	    ep = TAILQ_FIRST(&list->el_entries);
+	if (list->el_runcount == 0) {
+	    CTR2(KTR_EVH, "%s: removing all items from \"%s\"", __func__,
+		list->el_name);
 	    TAILQ_REMOVE(&list->el_entries, ep, ee_link);
-	    free(ep, M_EVENTHANDLER);
+	    while (!TAILQ_EMPTY(&list->el_entries)) {
+		ep = TAILQ_FIRST(&list->el_entries);
+		TAILQ_REMOVE(&list->el_entries, ep, ee_link);
+		free(ep, M_EVENTHANDLER);
+	    }
+	} else {
+	    CTR2(KTR_EVH, "%s: marking all items from \"%s\" as dead",
+		__func__, list->el_name);
+	    TAILQ_FOREACH(ep, &list->el_entries, ee_link)
+		ep->ee_priority = EHE_DEAD_PRIORITY;
 	}
     }
-    EHE_UNLOCK(list);
+    EHL_UNLOCK(list);
 }
 
+/*
+ * Internal version for use when eventhandler list is already locked.
+ */
+static struct eventhandler_list *
+_eventhandler_find_list(char *name)
+{
+    struct eventhandler_list	*list;
+
+    mtx_assert(&eventhandler_mutex, MA_OWNED);
+    TAILQ_FOREACH(list, &eventhandler_lists, el_link) {
+	if (!strcmp(name, list->el_name))
+	    break;
+    }
+    return (list);
+}
+
+/*
+ * Lookup a "slow" list by name.  Returns with the list locked.
+ */
 struct eventhandler_list *
 eventhandler_find_list(char *name)
 {
@@ -160,14 +210,31 @@ eventhandler_find_list(char *name)
     
     /* scan looking for the requested list */
     mtx_lock(&eventhandler_mutex);
-    for (list = TAILQ_FIRST(&eventhandler_lists); 
-	 list != NULL; 
-	 list = TAILQ_NEXT(list, el_link)) {
-	if (!strcmp(name, list->el_name))
-	    break;
-    }
+    list = _eventhandler_find_list(name);
+    if (list != NULL)
+	EHL_LOCK(list);
     mtx_unlock(&eventhandler_mutex);
     
     return(list);
 }
 
+/*
+ * Prune "dead" entries from an eventhandler list.
+ */
+void
+eventhandler_prune_list(struct eventhandler_list *list)
+{
+    struct eventhandler_entry *ep, *en;
+
+    CTR2(KTR_EVH, "%s: pruning list \"%s\"", __func__, list->el_name);
+    EHL_LOCK_ASSERT(list, MA_OWNED);
+    ep = TAILQ_FIRST(&list->el_entries);
+    while (ep != NULL) {
+	en = TAILQ_NEXT(ep, ee_link);
+	if (ep->ee_priority == EHE_DEAD_PRIORITY) {
+	    TAILQ_REMOVE(&list->el_entries, ep, ee_link);
+	    free(ep, M_EVENTHANDLER);
+	}
+	ep = en;
+    }
+}