d/freebsd-dev
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
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
This commit is contained in:
Andriy Gapon 2018-07-23 12:51:23 +00:00
parent 725de58112
commit 111b043cdf
Notes: svn2git 2020-12-20 02:59:44 +00:00 
svn path=/head/; revision=336635
2 changed files with 54 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

96
sys/kern/kern_intr.c
View File

@ -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 @@ intr_event_remove_handler(void *cookie)
*/
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 @@ intr_event_remove_handler(void *cookie)
* 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)

6
sys/sys/interrupt.h
View File

@ -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 *source,
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);