Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 23 Jul 2018 12:51:23 +0000 (UTC)
From:      Andriy Gapon <avg@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r336635 - in head/sys: kern sys
Message-ID:  <201807231251.w6NCpNYK070272@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: avg
Date: Mon Jul 23 12:51:23 2018
New Revision: 336635
URL: https://svnweb.freebsd.org/changeset/base/336635

Log:
  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

Modified:
  head/sys/kern/kern_intr.c
  head/sys/sys/interrupt.h

Modified: head/sys/kern/kern_intr.c
==============================================================================
--- head/sys/kern/kern_intr.c	Mon Jul 23 11:21:43 2018	(r336634)
+++ head/sys/kern/kern_intr.c	Mon Jul 23 12:51:23 2018	(r336635)
@@ -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 *sou
 	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 ch
     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 ch
 
 	/* 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 ch
 	}
 
 	/* 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, voi
 
 	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 trapfr
 #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 trapfr
 	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 trapfr
 		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)

Modified: head/sys/sys/interrupt.h
==============================================================================
--- head/sys/sys/interrupt.h	Mon Jul 23 11:21:43 2018	(r336634)
+++ head/sys/sys/interrupt.h	Mon Jul 23 12:51:23 2018	(r336635)
@@ -33,6 +33,7 @@
 
 #include <sys/_lock.h>
 #include <sys/_mutex.h>
+#include <sys/ck.h>
 
 struct intr_event;
 struct intr_thread;
@@ -52,7 +53,7 @@ struct intr_handler {
 	char		 ih_name[MAXCOMLEN + 1]; /* Name of handler. */
 	struct intr_event *ih_event;	/* Event we are connected to. */
 	int		 ih_need;	/* Needs service. */
-	TAILQ_ENTRY(intr_handler) ih_next; /* Next handler for this event. */
+	CK_SLIST_ENTRY(intr_handler) ih_next; /* Next handler for this event. */
 	u_char		 ih_pri;	/* Priority of this handler. */
 	struct intr_thread *ih_thread;	/* Ithread for filtered handler. */
 };
@@ -105,7 +106,7 @@ struct intr_handler {
  */
 struct intr_event {
 	TAILQ_ENTRY(intr_event) ie_list;
-	TAILQ_HEAD(, intr_handler) ie_handlers; /* Interrupt handlers. */
+	CK_SLIST_HEAD(, intr_handler) ie_handlers; /* Interrupt handlers. */
 	char		ie_name[MAXCOMLEN + 1]; /* Individual event name. */
 	char		ie_fullname[MAXCOMLEN + 1];
 	struct mtx	ie_lock;
@@ -174,7 +175,6 @@ int	intr_event_create(struct intr_event **event, void 
 int	intr_event_describe_handler(struct intr_event *ie, void *cookie,
 	    const char *descr);
 int	intr_event_destroy(struct intr_event *ie);
-void	intr_event_execute_handlers(struct proc *p, struct intr_event *ie);
 int	intr_event_handle(struct intr_event *ie, struct trapframe *frame);
 int	intr_event_remove_handler(void *cookie);
 int	intr_getaffinity(int irq, int mode, void *mask);



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201807231251.w6NCpNYK070272>