From 2f667a82acff749f81828ac6041ff92602c69e51 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 28 Feb 2024 03:55:49 +0000 Subject: [PATCH] heartbeat(9): No kpreempt_disable/enable in heartbeat_suspend/resume. This causes a leak of l_nopreempt in xc_thread when a CPU is offlined and onlined again, because the offlining heartbeat_suspend and the onlining heartbeat_resume happen in separate xcalls. No change to callers because they are already bound to the CPU: 1. cnpollc does kpreempt_disable/enable itself around the calls to heartbeat_suspend/resume anyway 2. cpu_xc_offline/online run in the xcall thread, which is always bound to the CPU that is being offlined or onlined --- sys/kern/kern_heartbeat.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/sys/kern/kern_heartbeat.c b/sys/kern/kern_heartbeat.c index 8d4ea01880d1..e6b5b57c1b4f 100644 --- a/sys/kern/kern_heartbeat.c +++ b/sys/kern/kern_heartbeat.c @@ -132,24 +132,20 @@ void *heartbeat_sih __read_mostly; * * Called after the current CPU has been marked offline but before * it has stopped running, or after IPL has been raised for - * polling-mode console input. Binds to the current CPU as a side - * effect. Nestable (but only up to 2^32 times, so don't do this - * in a loop). Reversed by heartbeat_resume. + * polling-mode console input. Nestable. Reversed by + * heartbeat_resume. + * + * Caller must be bound to the CPU, i.e., curcpu_stable() must be + * true. This function does not assert curcpu_stable() since it + * is used in the ddb entry path, where any assertions risk + * infinite regress into undebuggable chaos, so callers must be + * careful. */ void heartbeat_suspend(void) { unsigned *p; - /* - * We could use curlwp_bind, but we'd have to record whether we - * were already bound or not to pass to curlwp_bindx in - * heartbeat_resume. Using kpreempt_disable is simpler and - * unlikely to have any adverse consequences, since this only - * happens when we're about to go into a tight polling loop at - * raised IPL anyway. - */ - kpreempt_disable(); p = &curcpu()->ci_heartbeat_suspend; atomic_store_relaxed(p, *p + 1); } @@ -186,6 +182,9 @@ heartbeat_resume_cpu(struct cpu_info *ci) * Called after the current CPU has started running but before it * has been marked online, or when ending polling-mode input * before IPL is restored. Reverses heartbeat_suspend. + * + * Caller must be bound to the CPU, i.e., curcpu_stable() must be + * true. */ void heartbeat_resume(void) @@ -194,6 +193,8 @@ heartbeat_resume(void) unsigned *p; int s; + KASSERT(curcpu_stable()); + /* * Reset the state so nobody spuriously thinks we had a heart * attack as soon as the heartbeat checks resume. @@ -204,7 +205,6 @@ heartbeat_resume(void) p = &ci->ci_heartbeat_suspend; atomic_store_relaxed(p, *p - 1); - kpreempt_enable(); } /*