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>