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);
>> > 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.
> 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);
> }
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
