From: Mateusz Guzik <mjguzik@gmail.com>
Subject: [PATCH] return from mi_switch() without the scheduler lock
To: tech@openbsd.org
Date: Tue, 26 Nov 2024 09:26:23 +0100

The scheduler lock gets taken at the end only to be immediately released
by most callers.

The expectations of the lock not being held is documented with
SCHED_ASSERT_UNLOCKED().

====

The change is not even compile-tested, it came up while I was poking
around the rwlock diff (to which I'll respond some time later).

I did patch up everything which grepped for mi_switch, but I reserve the
right to have missed something.

Alternatively one could add a variant which explicitly does not return
with the lock and explicitly patch up consumers to use it.

diff --git a/sys/kern/kern_sched.c b/sys/kern/kern_sched.c
index b84e4b0d08d..b9b52851ba1 100644
--- a/sys/kern/kern_sched.c
+++ b/sys/kern/kern_sched.c
@@ -152,6 +152,7 @@ sched_idle(void *v)
 	p->p_cpu = ci;
 	atomic_setbits_int(&p->p_flag, P_CPUPEG);
 	mi_switch();
+	SCHED_LOCK();
 	cpuset_del(&sched_idle_cpus, ci);
 	SCHED_UNLOCK();
 
@@ -165,7 +166,7 @@ sched_idle(void *v)
 			SCHED_LOCK();
 			p->p_stat = SSLEEP;
 			mi_switch();
-			SCHED_UNLOCK();
+			SCHED_ASSERT_UNLOCKED();
 
 			while ((dead = LIST_FIRST(&spc->spc_deadproc))) {
 				LIST_REMOVE(dead, p_hash);
@@ -634,7 +635,7 @@ sched_peg_curproc(struct cpu_info *ci)
 	setrunqueue(ci, p, p->p_usrpri);
 	p->p_ru.ru_nvcsw++;
 	mi_switch();
-	SCHED_UNLOCK();
+	SCHED_ASSERT_UNLOCKED();
 }
 
 void
diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 53e5d68f4df..828e5b2ae61 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -1538,8 +1538,10 @@ proc_stop(struct proc *p, int sw)
 	 * Extra calls to softclock don't hurt.
 	 */
 	softintr_schedule(proc_stop_si);
-	if (sw)
+	if (sw) {
 		mi_switch();
+		SCHED_LOCK();
+	}
 }
 
 /*
@@ -2102,7 +2104,7 @@ single_thread_check_locked(struct proc *p, int deep)
 		SCHED_LOCK();
 		p->p_stat = SSTOP;
 		mi_switch();
-		SCHED_UNLOCK();
+		SCHED_ASSERT_UNLOCKED();
 		mtx_enter(&pr->ps_mtx);
 	} while (pr->ps_single != NULL);
 
diff --git a/sys/kern/kern_synch.c b/sys/kern/kern_synch.c
index 8fa6e299b5b..5b75a795788 100644
--- a/sys/kern/kern_synch.c
+++ b/sys/kern/kern_synch.c
@@ -416,6 +416,7 @@ sleep_finish(int timo, int do_sleep)
 		KASSERT(p->p_stat == SSLEEP || p->p_stat == SSTOP);
 		p->p_ru.ru_nvcsw++;
 		mi_switch();
+		SCHED_LOCK();
 	} else {
 		KASSERT(p->p_stat == SONPROC || p->p_stat == SSLEEP);
 		p->p_stat = SONPROC;
@@ -615,7 +616,7 @@ sys_sched_yield(struct proc *p, void *v, register_t *retval)
 	setrunqueue(p->p_cpu, p, newprio);
 	p->p_ru.ru_nvcsw++;
 	mi_switch();
-	SCHED_UNLOCK();
+	SCHED_ASSERT_UNLOCKED();
 
 	return (0);
 }
diff --git a/sys/kern/sched_bsd.c b/sys/kern/sched_bsd.c
index 31bce5d87f7..3b3dccaae75 100644
--- a/sys/kern/sched_bsd.c
+++ b/sys/kern/sched_bsd.c
@@ -317,7 +317,7 @@ yield(void)
 	setrunqueue(p->p_cpu, p, p->p_usrpri);
 	p->p_ru.ru_nvcsw++;
 	mi_switch();
-	SCHED_UNLOCK();
+	SCHED_ASSERT_UNLOCKED();
 }
 
 /*
@@ -335,7 +335,7 @@ preempt(void)
 	setrunqueue(p->p_cpu, p, p->p_usrpri);
 	p->p_ru.ru_nivcsw++;
 	mi_switch();
-	SCHED_UNLOCK();
+	SCHED_ASSERT_UNLOCKED();
 }
 
 void
@@ -440,7 +440,7 @@ mi_switch(void)
 	if (hold_count)
 		__mp_acquire_count(&kernel_lock, hold_count);
 #endif
-	SCHED_LOCK();
+	SCHED_ASSERT_UNLOCKED();
 }
 
 /*