On Thu, Aug 04, 2022 at 09:39:13AM +0200, Jeremie Courreges-Anglas wrote:
> On Mon, Aug 01 2022, Scott Cheloha <[email protected]> wrote:
> > On Mon, Aug 01, 2022 at 07:15:33PM +0200, Jeremie Courreges-Anglas wrote:
> >> On Sun, Jul 31 2022, Scott Cheloha <[email protected]> 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;
