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.
> >
> > 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.
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 1 Aug 2022 17:36:41 -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;
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 1 Aug 2022 17:36:41 -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: 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 1 Aug 2022 17:36:42 -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);
}
