On Thu, Aug 04, 2022 at 09:39:13AM +0200, Jeremie Courreges-Anglas wrote: > On Mon, Aug 01 2022, Scott Cheloha <scottchel...@gmail.com> wrote: > > On Mon, Aug 01, 2022 at 07:15:33PM +0200, Jeremie Courreges-Anglas wrote: > >> On Sun, Jul 31 2022, Scott Cheloha <scottchel...@gmail.com> wrote: > >> > Hi, > >> > > >> > I am unsure how to properly mask RISC-V timer interrupts in hardware > >> > at or above IPL_CLOCK. I think that would be cleaner than doing > >> > another timer interrupt deferral thing. > >> > > >> > But, just to get the ball rolling, here a first attempt at the timer > >> > interrupt deferral thing for riscv64. The motivation, as with every > >> > other platform, is to eventually make it unnecessary for the machine > >> > dependent code to know anything about the clock interrupt schedule. > >> > > >> > The thing I'm most unsure about is where to retrigger the timer in the > >> > PLIC code. It seems right to do it from plic_setipl() because I want > >> > to retrigger it before doing soft interrupt work, but I'm not sure. > > You're adding the timer reset to plic_setipl() but the latter is called > after softintr processing in plic_splx(). > > /* Pending software intr is handled here */ > if (ci->ci_ipending & riscv_smask[new]) > riscv_do_pending_intr(new); > > plic_setipl(new);
Yes, but plic_setipl() is also called from the softintr loop in riscv_do_pending_intr() (riscv64/intr.c) right *before* we dispatch any pending soft interrupts: 594 void 595 riscv_do_pending_intr(int pcpl) 596 { 597 struct cpu_info *ci = curcpu(); 598 u_long sie; 599 600 sie = intr_disable(); 601 602 #define DO_SOFTINT(si, ipl) \ 603 if ((ci->ci_ipending & riscv_smask[pcpl]) & \ 604 SI_TO_IRQBIT(si)) { \ 605 ci->ci_ipending &= ~SI_TO_IRQBIT(si); \ * 606 riscv_intr_func.setipl(ipl); \ 607 intr_restore(sie); \ 608 softintr_dispatch(si); \ 609 sie = intr_disable(); \ 610 } 611 612 do { 613 DO_SOFTINT(SIR_TTY, IPL_SOFTTTY); 614 DO_SOFTINT(SIR_NET, IPL_SOFTNET); 615 DO_SOFTINT(SIR_CLOCK, IPL_SOFTCLOCK); 616 DO_SOFTINT(SIR_SOFT, IPL_SOFT); 617 } while (ci->ci_ipending & riscv_smask[pcpl]); We might be fine doing it just once in plic_splx() before we do any soft interrupt stuff. That's closer to what we're doing on other platforms. I just figured it'd be safer to do it in plic_setipl() because we're already disabling interrupts there. It seems I guessed correctly because the patch didn't hang your machine. > >> > Unless I'm missing something, I don't think I need to do anything in > >> > the default interrupt handler code, i.e. riscv64_dflt_setipl(), right? > >> > >> No idea about about the items above, but... > >> > >> > I have no riscv64 machine, so this is untested. Would appreciate > >> > tests and feedback. > >> > >> There's an #include <machine/sbi.h> missing in plic.c, > > > > Whoops, corrected patch attached below. > > > >> with that added your diff builds and GENERIC.MP seems to behave > >> (currently running make -j4 build), but I don't know exactly which > >> problems I should look for. > > > > Thank you for trying it out. > > > > The patch changes how clock interrupt work is deferred on riscv64. > > > > If the code is wrong, the hardclock and statclock should eventually > > die on every CPU. The death of the hardclock in particular would > > manifest to the user as livelock. The scheduler would stop preempting > > userspace and it would be impossible to use the machine interactively. > > > > There isn't really a direct way to exercise this code change. > > > > The best we can do is make the machine busy. If the machine is busy > > we can expect more spl(9) calls and more deferred clock interrupt > > work, which leaves more opportunities for the bug to surface. > > > > So, a parallel `make build` is fine. It's our gold standard for > > making the machine really busy. > > The diff survived three make -j4 build/release in a row, the clock seems > stable. Awesome! Thank you for hammering on it. kettenis, mlarkin, drahn: Is this code fine or do you want to go about this in a different way? Index: dev/plic.c =================================================================== RCS file: /cvs/src/sys/arch/riscv64/dev/plic.c,v retrieving revision 1.10 diff -u -p -r1.10 plic.c --- dev/plic.c 6 Apr 2022 18:59:27 -0000 1.10 +++ dev/plic.c 4 Aug 2022 15:32:01 -0000 @@ -27,6 +27,7 @@ #include <machine/bus.h> #include <machine/fdt.h> #include <machine/cpu.h> +#include <machine/sbi.h> #include "riscv64/dev/riscv_cpu_intc.h" #include <dev/ofw/openfirm.h> @@ -556,6 +557,10 @@ plic_setipl(int new) /* higher values are higher priority */ plic_set_threshold(ci->ci_cpuid, new); + + /* trigger deferred timer interrupt if cpl is now low enough */ + if (ci->ci_timer_deferred && new < IPL_CLOCK) + sbi_set_timer(0); intr_restore(sie); } Index: riscv64/clock.c =================================================================== RCS file: /cvs/src/sys/arch/riscv64/riscv64/clock.c,v retrieving revision 1.3 diff -u -p -r1.3 clock.c --- riscv64/clock.c 24 Jul 2021 22:41:09 -0000 1.3 +++ riscv64/clock.c 4 Aug 2022 15:32:01 -0000 @@ -21,6 +21,7 @@ #include <sys/kernel.h> #include <sys/systm.h> #include <sys/evcount.h> +#include <sys/stdint.h> #include <sys/timetc.h> #include <machine/cpufunc.h> @@ -106,6 +107,17 @@ clock_intr(void *frame) int s; /* + * If the clock interrupt is masked, defer all clock interrupt + * work until the clock interrupt is unmasked from splx(9). + */ + if (ci->ci_cpl >= IPL_CLOCK) { + ci->ci_timer_deferred = 1; + sbi_set_timer(UINT64_MAX); + return 0; + } + ci->ci_timer_deferred = 0; + + /* * Based on the actual time delay since the last clock interrupt, * we arrange for earlier interrupt next time. */ @@ -132,30 +144,23 @@ clock_intr(void *frame) sbi_set_timer(nextevent); - if (ci->ci_cpl >= IPL_CLOCK) { - ci->ci_statspending += nstats; - } else { - nstats += ci->ci_statspending; - ci->ci_statspending = 0; - - s = splclock(); - intr_enable(); - - /* - * Do standard timer interrupt stuff. - */ - while (ci->ci_lasttb < prevtb) { - ci->ci_lasttb += tick_increment; - clock_count.ec_count++; - hardclock((struct clockframe *)frame); - } + s = splclock(); + intr_enable(); - while (nstats-- > 0) - statclock((struct clockframe *)frame); - - intr_disable(); - splx(s); + /* + * Do standard timer interrupt stuff. + */ + while (ci->ci_lasttb < prevtb) { + ci->ci_lasttb += tick_increment; + clock_count.ec_count++; + hardclock((struct clockframe *)frame); } + + while (nstats-- > 0) + statclock((struct clockframe *)frame); + + intr_disable(); + splx(s); return 0; } Index: include/cpu.h =================================================================== RCS file: /cvs/src/sys/arch/riscv64/include/cpu.h,v retrieving revision 1.12 diff -u -p -r1.12 cpu.h --- include/cpu.h 10 Jun 2022 21:34:15 -0000 1.12 +++ include/cpu.h 4 Aug 2022 15:32:01 -0000 @@ -92,7 +92,7 @@ struct cpu_info { uint64_t ci_lasttb; uint64_t ci_nexttimerevent; uint64_t ci_nextstatevent; - int ci_statspending; + volatile int ci_timer_deferred; uint32_t ci_cpl; uint32_t ci_ipending;