change interrupt event's list of handlers from TAILQ to CK_SLIST

The primary reason for this commit is to separate mechanical and nearly
mechanical code changes from an upcoming fix for unsafe teardown of
shared interrupt handlers that have only filters (see D15905).

The technical rationale is that SLIST is sufficient.  The only operation
that gets worse performance -- O(n) instead of O(1) is a removal of a
handler,  but it is not a critical operation and the list is expected to
be rather short.

Additionally, it is easier to reason about SLIST when considering the
concurrent lock-free access to the list from the interrupt context and
the interrupt thread.

CK_SLIST is used because the upcoming change depends on the memory order
provided by CK_SLIST insert and the fact that CL_SLIST remove does not
trash the linkage in a removed element.

While here, I also fixed a couple of whitespace issues, made code under
ifdef notyet compilable, added a lock assertion to ithread_update() and
made intr_event_execute_handlers() static as it had no external callers.

Reviewed by:	cem (earlier version)
MFC after:	4 weeks
Differential Revision: https://reviews.freebsd.org/D16016

1 files changed, 51 insertions, 45 deletions

diff --git a/sys/kern/kern_intr.c b/sys/kern/kern_intr.c
index 5c638fb8569e..541e33906d6a 100644
--- a/sys/kern/kern_intr.c
+++ b/sys/kern/kern_intr.c
@@ -160,12 +160,13 @@ ithread_update(struct intr_thread *ithd)
 
 	ie = ithd->it_event;
 	td = ithd->it_thread;
+	mtx_assert(&ie->ie_lock, MA_OWNED);
 
 	/* Determine the overall priority of this event. */
-	if (TAILQ_EMPTY(&ie->ie_handlers))
+	if (CK_SLIST_EMPTY(&ie->ie_handlers))
 		pri = PRI_MAX_ITHD;
 	else
-		pri = TAILQ_FIRST(&ie->ie_handlers)->ih_pri;
+		pri = CK_SLIST_FIRST(&ie->ie_handlers)->ih_pri;
 
 	/* Update name and priority. */
 	strlcpy(td->td_name, ie->ie_fullname, sizeof(td->td_name));
@@ -195,7 +196,7 @@ intr_event_update(struct intr_event *ie)
 	space = 1;
 
 	/* Run through all the handlers updating values. */
-	TAILQ_FOREACH(ih, &ie->ie_handlers, ih_next) {
+	CK_SLIST_FOREACH(ih, &ie->ie_handlers, ih_next) {
 		if (strlen(ie->ie_fullname) + strlen(ih->ih_name) + 1 <
 		    sizeof(ie->ie_fullname)) {
 			strcat(ie->ie_fullname, " ");
@@ -257,7 +258,7 @@ intr_event_create(struct intr_event **event, void *source, int flags, int irq,
 	ie->ie_flags = flags;
 	ie->ie_irq = irq;
 	ie->ie_cpu = NOCPU;
-	TAILQ_INIT(&ie->ie_handlers);
+	CK_SLIST_INIT(&ie->ie_handlers);
 	mtx_init(&ie->ie_lock, "intr event", NULL, MTX_DEF);
 
 	va_start(ap, fmt);
@@ -378,7 +379,7 @@ intr_lookup(int irq)
 	TAILQ_FOREACH(ie, &event_list, ie_list)
 		if (ie->ie_irq == irq &&
 		    (ie->ie_flags & IE_SOFT) == 0 &&
-		    TAILQ_FIRST(&ie->ie_handlers) != NULL)
+		    CK_SLIST_FIRST(&ie->ie_handlers) != NULL)
 			break;
 	mtx_unlock(&event_lock);
 	return (ie);
@@ -474,7 +475,7 @@ intr_event_destroy(struct intr_event *ie)
 
 	mtx_lock(&event_lock);
 	mtx_lock(&ie->ie_lock);
-	if (!TAILQ_EMPTY(&ie->ie_handlers)) {
+	if (!CK_SLIST_EMPTY(&ie->ie_handlers)) {
 		mtx_unlock(&ie->ie_lock);
 		mtx_unlock(&event_lock);
 		return (EBUSY);
@@ -504,7 +505,7 @@ ithread_create(const char *name)
 
 	error = kproc_kthread_add(ithread_loop, ithd, &intrproc,
 		    &td, RFSTOPPED | RFHIGHPID,
-	    	    0, "intr", "%s", name);
+		    0, "intr", "%s", name);
 	if (error)
 		panic("kproc_create() failed with %d", error);
 	thread_lock(td);
@@ -539,6 +540,7 @@ intr_event_add_handler(struct intr_event *ie, const char *name,
     enum intr_type flags, void **cookiep)
 {
 	struct intr_handler *ih, *temp_ih;
+	struct intr_handler **prevptr;
 	struct intr_thread *it;
 
 	if (ie == NULL || name == NULL || (handler == NULL && filter == NULL))
@@ -561,9 +563,9 @@ intr_event_add_handler(struct intr_event *ie, const char *name,
 
 	/* We can only have one exclusive handler in a event. */
 	mtx_lock(&ie->ie_lock);
-	if (!TAILQ_EMPTY(&ie->ie_handlers)) {
+	if (!CK_SLIST_EMPTY(&ie->ie_handlers)) {
 		if ((flags & INTR_EXCL) ||
-		    (TAILQ_FIRST(&ie->ie_handlers)->ih_flags & IH_EXCLUSIVE)) {
+		    (CK_SLIST_FIRST(&ie->ie_handlers)->ih_flags & IH_EXCLUSIVE)) {
 			mtx_unlock(&ie->ie_lock);
 			free(ih, M_ITHREAD);
 			return (EINVAL);
@@ -588,14 +590,12 @@ intr_event_add_handler(struct intr_event *ie, const char *name,
 	}
 
 	/* Add the new handler to the event in priority order. */
-	TAILQ_FOREACH(temp_ih, &ie->ie_handlers, ih_next) {
+	CK_SLIST_FOREACH_PREVPTR(temp_ih, prevptr, &ie->ie_handlers, ih_next) {
 		if (temp_ih->ih_pri > ih->ih_pri)
 			break;
 	}
-	if (temp_ih == NULL)
-		TAILQ_INSERT_TAIL(&ie->ie_handlers, ih, ih_next);
-	else
-		TAILQ_INSERT_BEFORE(temp_ih, ih, ih_next);
+	CK_SLIST_INSERT_PREVPTR(prevptr, temp_ih, ih, ih_next);
+
 	intr_event_update(ie);
 
 	CTR3(KTR_INTR, "%s: added %s to %s", __func__, ih->ih_name,
@@ -621,7 +621,7 @@ intr_event_describe_handler(struct intr_event *ie, void *cookie,
 
 	mtx_lock(&ie->ie_lock);
 #ifdef INVARIANTS
-	TAILQ_FOREACH(ih, &ie->ie_handlers, ih_next) {
+	CK_SLIST_FOREACH(ih, &ie->ie_handlers, ih_next) {
 		if (ih == cookie)
 			break;
 	}
@@ -721,15 +721,13 @@ _intr_drain(int irq)
 	return;
 }
 
-
 int
 intr_event_remove_handler(void *cookie)
 {
 	struct intr_handler *handler = (struct intr_handler *)cookie;
 	struct intr_event *ie;
-#ifdef INVARIANTS
 	struct intr_handler *ih;
-#endif
+	struct intr_handler **prevptr;
 #ifdef notyet
 	int dead;
 #endif
@@ -740,25 +738,26 @@ intr_event_remove_handler(void *cookie)
 	KASSERT(ie != NULL,
 	    ("interrupt handler \"%s\" has a NULL interrupt event",
 	    handler->ih_name));
+
 	mtx_lock(&ie->ie_lock);
 	CTR3(KTR_INTR, "%s: removing %s from %s", __func__, handler->ih_name,
 	    ie->ie_name);
-#ifdef INVARIANTS
-	TAILQ_FOREACH(ih, &ie->ie_handlers, ih_next)
+	CK_SLIST_FOREACH_PREVPTR(ih, prevptr, &ie->ie_handlers, ih_next) {
 		if (ih == handler)
-			goto ok;
-	mtx_unlock(&ie->ie_lock);
-	panic("interrupt handler \"%s\" not found in interrupt event \"%s\"",
-	    ih->ih_name, ie->ie_name);
-ok:
-#endif
+			break;
+	}
+	if (ih == NULL) {
+		panic("interrupt handler \"%s\" not found in "
+		    "interrupt event \"%s\"", handler->ih_name, ie->ie_name);
+	}
+
 	/*
 	 * If there is no ithread, then just remove the handler and return.
 	 * XXX: Note that an INTR_FAST handler might be running on another
 	 * CPU!
 	 */
 	if (ie->ie_thread == NULL) {
-		TAILQ_REMOVE(&ie->ie_handlers, handler, ih_next);
+		CK_SLIST_REMOVE_PREVPTR(prevptr, ih, ih_next);
 		mtx_unlock(&ie->ie_lock);
 		free(handler, M_ITHREAD);
 		return (0);
@@ -789,11 +788,12 @@ ok:
 		 */
 		atomic_store_rel_int(&ie->ie_thread->it_need, 1);
 	} else
-		TAILQ_REMOVE(&ie->ie_handlers, handler, ih_next);
+		CK_SLIST_REMOVE_PREVPTR(prevptr, ih, ih_next);
 	thread_unlock(ie->ie_thread->it_thread);
 	while (handler->ih_flags & IH_DEAD)
 		msleep(handler, &ie->ie_lock, 0, "iev_rmh", 0);
 	intr_event_update(ie);
+
 #ifdef notyet
 	/*
 	 * XXX: This could be bad in the case of ppbus(8).  Also, I think
@@ -801,8 +801,8 @@ ok:
 	 * interrupt.
 	 */
 	dead = 1;
-	TAILQ_FOREACH(ih, &ie->ie_handlers, ih_next) {
-		if (!(ih->ih_flags & IH_FAST)) {
+	CK_SLIST_FOREACH(ih, &ie->ie_handlers, ih_next) {
+		if (ih->ih_handler != NULL) {
 			dead = 0;
 			break;
 		}
@@ -828,7 +828,7 @@ intr_event_schedule_thread(struct intr_event *ie)
 	/*
 	 * If no ithread or no handlers, then we have a stray interrupt.
 	 */
-	if (ie == NULL || TAILQ_EMPTY(&ie->ie_handlers) ||
+	if (ie == NULL || CK_SLIST_EMPTY(&ie->ie_handlers) ||
 	    ie->ie_thread == NULL)
 		return (EINVAL);
 
@@ -962,30 +962,35 @@ swi_remove(void *cookie)
 	return (intr_event_remove_handler(cookie));
 }
 
-
-/*
- * This is a public function for use by drivers that mux interrupt
- * handlers for child devices from their interrupt handler.
- */
-void
+static void
 intr_event_execute_handlers(struct proc *p, struct intr_event *ie)
 {
-	struct intr_handler *ih, *ihn;
+	struct intr_handler *ih, *ihn, *ihp;
 
-	TAILQ_FOREACH_SAFE(ih, &ie->ie_handlers, ih_next, ihn) {
+	ihp = NULL;
+	CK_SLIST_FOREACH_SAFE(ih, &ie->ie_handlers, ih_next, ihn) {
 		/*
 		 * If this handler is marked for death, remove it from
 		 * the list of handlers and wake up the sleeper.
 		 */
 		if (ih->ih_flags & IH_DEAD) {
 			mtx_lock(&ie->ie_lock);
-			TAILQ_REMOVE(&ie->ie_handlers, ih, ih_next);
+			if (ihp == NULL)
+				CK_SLIST_REMOVE_HEAD(&ie->ie_handlers, ih_next);
+			else
+				CK_SLIST_REMOVE_AFTER(ihp, ih_next);
 			ih->ih_flags &= ~IH_DEAD;
 			wakeup(ih);
 			mtx_unlock(&ie->ie_lock);
 			continue;
 		}
 
+		/*
+		 * Now that we know that the current element won't be removed
+		 * update the previous element.
+		 */
+		ihp = ih;
+
 		/* Skip filter only handlers */
 		if (ih->ih_handler == NULL)
 			continue;
@@ -1157,7 +1162,7 @@ intr_event_handle(struct intr_event *ie, struct trapframe *frame)
 #endif
 
 	/* An interrupt with no event or handlers is a stray interrupt. */
-	if (ie == NULL || TAILQ_EMPTY(&ie->ie_handlers))
+	if (ie == NULL || CK_SLIST_EMPTY(&ie->ie_handlers))
 		return (EINVAL);
 
 	/*
@@ -1172,7 +1177,8 @@ intr_event_handle(struct intr_event *ie, struct trapframe *frame)
 	critical_enter();
 	oldframe = td->td_intr_frame;
 	td->td_intr_frame = frame;
-	TAILQ_FOREACH(ih, &ie->ie_handlers, ih_next) {
+
+	CK_SLIST_FOREACH(ih, &ie->ie_handlers, ih_next) {
 		if (ih->ih_filter == NULL) {
 			thread = 1;
 			continue;
@@ -1218,7 +1224,7 @@ intr_event_handle(struct intr_event *ie, struct trapframe *frame)
 		if (ie->ie_post_filter != NULL)
 			ie->ie_post_filter(ie->ie_source);
 	}
-	
+
 	/* Schedule the ithread if needed. */
 	if (thread) {
 		int error __unused;
@@ -1364,7 +1370,7 @@ db_dump_intr_event(struct intr_event *ie, int handlers)
 	db_printf("\n");
 
 	if (handlers)
-		TAILQ_FOREACH(ih, &ie->ie_handlers, ih_next)
+		CK_SLIST_FOREACH(ih, &ie->ie_handlers, ih_next)
 		    db_dump_intrhand(ih);
 }
 
@@ -1379,7 +1385,7 @@ DB_SHOW_COMMAND(intr, db_show_intr)
 	verbose = strchr(modif, 'v') != NULL;
 	all = strchr(modif, 'a') != NULL;
 	TAILQ_FOREACH(ie, &event_list, ie_list) {
-		if (!all && TAILQ_EMPTY(&ie->ie_handlers))
+		if (!all && CK_SLIST_EMPTY(&ie->ie_handlers))
 			continue;
 		db_dump_intr_event(ie, verbose);
 		if (db_pager_quit)