On Thu, May 13, 2021 at 11:08:25AM -0500, Scott Cheloha wrote: > > On May 13, 2021, at 10:57 AM, Visa Hankala <v...@hankala.org> wrote: > > > > On Thu, May 13, 2021 at 12:04:57AM -0500, Scott Cheloha wrote: > >> The funky locking dance in softclock() and softclock_thread() is > >> needed to keep from violating the locking hierarchy. The > >> timeout_mutex is above the kernel lock, so we need to leave the > >> timeout_mutex, then drop the kernel lock, and then reenter the > >> timeout_mutex to start running TIMEOUT_MPSAFE timeouts. > > > > Are you sure that dropping the kernel lock in softclock(), in > > non-process context, is a good idea? That makes assumptions how the > > layers above work. > > I figured it was the most obvious way forward. > > When you say "layers above," do you mean softintr_dispatch()? > Or something else?
I mean softintr_dispatch() and anything else in the dispatch path. > > At the minimum, I think the soft interrupt code has to be changed so > > that it is possible to register MP-safe handlers. > > Some platforms (e.g. arm64) have softintr_establish_flags(), but this > is not universally available. > > Do I need to unlock all the softintr internals before proceeding > with this? Or do I just need to bring softrintr_establish_flags() > to every platform without it? I think softrintr_establish() could register an MP-safe handler if IPL_MPSAFE is ORed in the ipl parameter. Dropping the kernel lock has various implications. At least the code has to ensure that at most one CPU can run a given handler at a time. In addition, softintr_disestablish() should probably block until the handler has stopped (at the moment, this is not guaranteed on the platforms that have softintr_establish_flags()). Below is a hasty sketch for amd64 that I am not suggesting to commit. It is probably wrong in more ways than one. Index: arch/amd64/amd64/softintr.c =================================================================== RCS file: src/sys/arch/amd64/amd64/softintr.c,v retrieving revision 1.10 diff -u -p -r1.10 softintr.c --- arch/amd64/amd64/softintr.c 11 Sep 2020 09:27:09 -0000 1.10 +++ arch/amd64/amd64/softintr.c 13 May 2021 17:28:51 -0000 @@ -37,6 +37,7 @@ #include <sys/param.h> #include <sys/systm.h> #include <sys/malloc.h> +#include <sys/smr.h> #include <machine/intr.h> @@ -85,7 +86,6 @@ softintr_dispatch(int which) floor = ci->ci_handled_intr_level; ci->ci_handled_intr_level = ci->ci_ilevel; - KERNEL_LOCK(); for (;;) { mtx_enter(&si->softintr_lock); sih = TAILQ_FIRST(&si->softintr_q); @@ -95,14 +95,20 @@ softintr_dispatch(int which) } TAILQ_REMOVE(&si->softintr_q, sih, sih_q); sih->sih_pending = 0; - - uvmexp.softs++; - mtx_leave(&si->softintr_lock); - (*sih->sih_fn)(sih->sih_arg); + atomic_inc_int(&uvmexp.softs); + + if (sih->sih_flags & IPL_MPSAFE) { + mtx_enter(&sih->sih_mtx); + (*sih->sih_fn)(sih->sih_arg); + mtx_leave(&sih->sih_mtx); + } else { + KERNEL_LOCK(); + (*sih->sih_fn)(sih->sih_arg); + KERNEL_UNLOCK(); + } } - KERNEL_UNLOCK(); ci->ci_handled_intr_level = floor; } @@ -117,7 +123,10 @@ softintr_establish(int ipl, void (*func) { struct x86_soft_intr *si; struct x86_soft_intrhand *sih; - int which; + int flags, which; + + flags = ipl & IPL_MPSAFE; + ipl &= ~IPL_MPSAFE; switch (ipl) { case IPL_SOFTCLOCK: @@ -141,9 +150,11 @@ softintr_establish(int ipl, void (*func) sih = malloc(sizeof(*sih), M_DEVBUF, M_NOWAIT); if (__predict_true(sih != NULL)) { + mtx_init(&sih->sih_mtx, IPL_NONE); sih->sih_intrhead = si; sih->sih_fn = func; sih->sih_arg = arg; + sih->sih_flags = flags; sih->sih_pending = 0; } return (sih); @@ -167,5 +178,8 @@ softintr_disestablish(void *arg) } mtx_leave(&si->softintr_lock); + /* Wait for any running handlers to finish. */ + smr_barrier(); + free(sih, M_DEVBUF, sizeof(*sih)); } Index: arch/amd64/include/intr.h =================================================================== RCS file: src/sys/arch/amd64/include/intr.h,v retrieving revision 1.32 diff -u -p -r1.32 intr.h --- arch/amd64/include/intr.h 17 Jun 2020 06:14:52 -0000 1.32 +++ arch/amd64/include/intr.h 13 May 2021 17:28:51 -0000 @@ -232,13 +232,16 @@ extern void (*ipifunc[X86_NIPI])(struct #ifndef _LOCORE #include <sys/queue.h> +#include <sys/mutex.h> struct x86_soft_intrhand { TAILQ_ENTRY(x86_soft_intrhand) sih_q; + struct mutex sih_mtx; struct x86_soft_intr *sih_intrhead; void (*sih_fn)(void *); void *sih_arg; + int sih_flags; int sih_pending; };