From 42385d6c6d9a8e7473f35e0ffa3bb6f8ac3ab53a Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 27 Aug 2023 15:30:51 +0000 Subject: [PATCH 1/6] sys/dev/cons.c: Sort includes. No functional change intended. --- sys/dev/cons.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/sys/dev/cons.c b/sys/dev/cons.c index fd2a5b507503..12525d2f75e2 100644 --- a/sys/dev/cons.c +++ b/sys/dev/cons.c @@ -42,20 +42,21 @@ __KERNEL_RCSID(0, "$NetBSD: cons.c,v 1.92 2022/10/25 23:21:33 riastradh Exp $"); #include -#include -#include + +#include #include -#include -#include -#include -#include #include -#include +#include +#include #include -#include #include -#include +#include +#include +#include #include +#include +#include +#include #include From 64cb8dd1c83d5724c2b96a608d99d094f5d89a89 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 27 Aug 2023 15:34:02 +0000 Subject: [PATCH 2/6] ukbd(4): Fix ordering in ukbd_cnpollc exit. This is probably an MP-safety issue waiting to happen, but let's at least make the wind and unwind sequences mirror images. --- sys/dev/usb/ukbd.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sys/dev/usb/ukbd.c b/sys/dev/usb/ukbd.c index 0f1b73eda04d..0dd593f758f5 100644 --- a/sys/dev/usb/ukbd.c +++ b/sys/dev/usb/ukbd.c @@ -1055,11 +1055,12 @@ ukbd_cnpollc(void *v, int on) if (on) { sc->sc_spl = splusb(); pollenter++; - } else { - splx(sc->sc_spl); - pollenter--; } usbd_set_polling(dev, on); + if (!on) { + pollenter--; + splx(sc->sc_spl); + } } int From 21c9fcb6662d674a8ee4ac58ae05ab60391d54e1 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 27 Aug 2023 15:47:14 +0000 Subject: [PATCH 3/6] cpu_setstate: Fix call to heartbeat_suspend. Do this on successful offlining, not on failed offlining. No functional change right now because heartbeat_suspend is implemented as a noop -- heartbeat(9) will just check the SPCF_OFFLINE flag. But if we change it to not be a noop, well, then we need to call it in the right place. --- sys/kern/kern_cpu.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sys/kern/kern_cpu.c b/sys/kern/kern_cpu.c index d082994d2661..88cb8f505617 100644 --- a/sys/kern/kern_cpu.c +++ b/sys/kern/kern_cpu.c @@ -370,6 +370,10 @@ cpu_xc_offline(struct cpu_info *ci, void *unused) pcu_save_all_on_cpu(); #endif +#ifdef HEARTBEAT + heartbeat_suspend(); +#endif + #ifdef __HAVE_MD_CPU_OFFLINE cpu_offline_md(); #endif @@ -379,10 +383,6 @@ fail: s = splsched(); spc->spc_flags &= ~SPCF_OFFLINE; splx(s); - -#ifdef HEARTBEAT - heartbeat_suspend(); -#endif } static void From 5729f35fd623e4d1c54b613e83308181087b997b Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 27 Aug 2023 15:49:41 +0000 Subject: [PATCH 4/6] ukbd(4): Sort includes. No functional change intended. --- sys/dev/usb/ukbd.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/sys/dev/usb/ukbd.c b/sys/dev/usb/ukbd.c index 0dd593f758f5..3207c0af2069 100644 --- a/sys/dev/usb/ukbd.c +++ b/sys/dev/usb/ukbd.c @@ -47,28 +47,29 @@ __KERNEL_RCSID(0, "$NetBSD: ukbd.c,v 1.162 2023/01/10 18:20:10 mrg Exp $"); #endif /* _KERNEL_OPT */ #include -#include + #include -#include #include -#include #include -#include +#include +#include +#include #include +#include +#include #include -#include -#include -#include +#include +#include +#include +#include +#include +#include #include #include #include -#include -#include -#include -#include -#include +#include #include #include From fba36aa3e2a8b6aee980ed9e08c9586d041ea746 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 27 Aug 2023 15:52:49 +0000 Subject: [PATCH 5/6] heartbeat(9): New flag SPCF_HEARTBEATSUSPENDED. This way we can suspend heartbeats on a single CPU while the console is in polling mode, not just when the CPU is offlined. This should be rare, so it's not _convenient_, but it should enable us to fix polling-mode USB keyboard input when the hardclock timer is still running. --- sys/kern/kern_heartbeat.c | 40 ++++++++++++++++++++++++++------------- sys/sys/sched.h | 1 + 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/sys/kern/kern_heartbeat.c b/sys/kern/kern_heartbeat.c index e293eb4ad3e2..32d2b44d33e0 100644 --- a/sys/kern/kern_heartbeat.c +++ b/sys/kern/kern_heartbeat.c @@ -127,17 +127,22 @@ void *heartbeat_sih __read_mostly; * Suspend heartbeat monitoring of the current CPU. * * Called after the current CPU has been marked offline but before - * it has stopped running. Caller must have preemption disabled. + * it has stopped running, or after IPL has been raised for + * polling-mode console input. Caller must have preemption + * disabled. Non-nestable. Reversed by heartbeat_resume. */ void heartbeat_suspend(void) { + struct cpu_info *ci = curcpu(); + int s; KASSERT(curcpu_stable()); + KASSERT((ci->ci_schedstate.spc_flags & SPCF_HEARTBEATSUSPENDED) == 0); - /* - * Nothing to do -- we just check the SPCF_OFFLINE flag. - */ + s = splsched(); + ci->ci_schedstate.spc_flags |= SPCF_HEARTBEATSUSPENDED; + splx(s); } /* @@ -148,6 +153,8 @@ heartbeat_suspend(void) * Called at startup while cold, and whenever heartbeat monitoring * is re-enabled after being disabled or the period is changed. * When not cold, ci must be the current CPU. + * + * Must be run at splsched. */ static void heartbeat_resume_cpu(struct cpu_info *ci) @@ -155,6 +162,7 @@ heartbeat_resume_cpu(struct cpu_info *ci) KASSERT(__predict_false(cold) || curcpu_stable()); KASSERT(__predict_false(cold) || ci == curcpu()); + /* XXX KASSERT IPL_SCHED */ ci->ci_heartbeat_count = 0; ci->ci_heartbeat_uptime_cache = time_uptime; @@ -167,9 +175,8 @@ heartbeat_resume_cpu(struct cpu_info *ci) * Resume heartbeat monitoring of the current CPU. * * Called after the current CPU has started running but before it - * has been marked online. Also used internally when starting up - * heartbeat monitoring at boot or when the maximum period is set - * from zero to nonzero. Caller must have preemption disabled. + * has been marked online, or when ending polling-mode input + * before IPL is restored. Caller must have preemption disabled. */ void heartbeat_resume(void) @@ -178,6 +185,7 @@ heartbeat_resume(void) int s; KASSERT(curcpu_stable()); + KASSERT(ci->ci_schedstate.spc_flags & SPCF_HEARTBEATSUSPENDED); /* * Block heartbeats while we reset the state so we don't @@ -185,6 +193,7 @@ heartbeat_resume(void) * resetting the count and the uptime stamp. */ s = splsched(); + ci->ci_schedstate.spc_flags &= ~SPCF_HEARTBEATSUSPENDED; heartbeat_resume_cpu(ci); splx(s); } @@ -198,8 +207,11 @@ heartbeat_resume(void) static void heartbeat_reset_xc(void *a, void *b) { + int s; - heartbeat_resume(); + s = splsched(); + heartbeat_resume_cpu(curcpu()); + splx(s); } /* @@ -488,7 +500,7 @@ select_patient(void) * in the iteration order. */ for (CPU_INFO_FOREACH(cii, ci)) { - if (ci->ci_schedstate.spc_flags & SPCF_OFFLINE) + if (ci->ci_schedstate.spc_flags & SPCF_HEARTBEATSUSPENDED) continue; if (passedcur) { /* @@ -565,7 +577,8 @@ heartbeat(void) period_secs = atomic_load_relaxed(&heartbeat_max_period_secs); if (__predict_false(period_ticks == 0) || __predict_false(period_secs == 0) || - __predict_false(curcpu()->ci_schedstate.spc_flags & SPCF_OFFLINE)) + __predict_false(curcpu()->ci_schedstate.spc_flags & + SPCF_HEARTBEATSUSPENDED)) return; /* @@ -637,8 +650,8 @@ heartbeat(void) * Verify that time is advancing on the patient CPU. If the * delta exceeds UINT_MAX/2, that means it is already ahead by * a little on the other CPU, and the subtraction went - * negative, which is OK. If the CPU has been - * offlined since we selected it, no worries. + * negative, which is OK. If the CPU's heartbeats have been + * suspended since we selected it, no worries. * * This uses the current CPU to ensure the other CPU has made * progress, even if the other CPU's hard timer interrupt @@ -650,7 +663,8 @@ heartbeat(void) d = uptime - atomic_load_relaxed(&patient->ci_heartbeat_uptime_cache); if (__predict_false(d > period_secs) && __predict_false(d < UINT_MAX/2) && - ((patient->ci_schedstate.spc_flags & SPCF_OFFLINE) == 0)) + ((patient->ci_schedstate.spc_flags & SPCF_HEARTBEATSUSPENDED) + == 0)) defibrillate(patient, d); } diff --git a/sys/sys/sched.h b/sys/sys/sched.h index 119aa1c768d1..2e67b59b4c01 100644 --- a/sys/sys/sched.h +++ b/sys/sys/sched.h @@ -188,6 +188,7 @@ struct schedstate_percpu { #define SPCF_1STCLASS 0x0040 /* first class scheduling entity */ #define SPCF_CORE1ST 0x0100 /* first CPU in core */ #define SPCF_PACKAGE1ST 0x0200 /* first CPU in package */ +#define SPCF_HEARTBEATSUSPENDED 0x0400 /* heartbeat (temporarily) suspended */ #define SPCF_SWITCHCLEAR (SPCF_SEENRR|SPCF_SHOULDYIELD) From d36fc9104faeb40b35217c96a34b5adf454ef7df Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 27 Aug 2023 15:54:03 +0000 Subject: [PATCH 6/6] ukbd(4): Suspend heartbeats while polling for console input. --- sys/dev/usb/ukbd.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sys/dev/usb/ukbd.c b/sys/dev/usb/ukbd.c index 3207c0af2069..ae58cd5c5500 100644 --- a/sys/dev/usb/ukbd.c +++ b/sys/dev/usb/ukbd.c @@ -51,6 +51,7 @@ __KERNEL_RCSID(0, "$NetBSD: ukbd.c,v 1.162 2023/01/10 18:20:10 mrg Exp $"); #include #include #include +#include #include #include #include @@ -1056,9 +1057,15 @@ ukbd_cnpollc(void *v, int on) if (on) { sc->sc_spl = splusb(); pollenter++; +#ifdef HEARTBEAT + heartbeat_suspend(); +#endif } usbd_set_polling(dev, on); if (!on) { +#ifdef HEARTBEAT + heartbeat_resume(); +#endif pollenter--; splx(sc->sc_spl); }