Rework interaction between sleep API and exit1() and start unlocking ps_threads

This diff adjusts how single_thread_set() accounts the threads by using
ps_threadcnt as initial value and counting all threads out that are already
parked. In single_thread_check call exit1() before decreasing ps_singlecount
this is now done in exit1().

exit1() and thread_fork() ensure that ps_threadcnt is updated with the
pr->ps_mtx held and in exit1() also account for exiting threads since
exit1() can sleep.

OK mpi@

7 files changed, 94 insertions, 72 deletions

diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c
index f881ff1c53f..029b5930915 100644
--- a/sys/kern/kern_exit.c
+++ b/sys/kern/kern_exit.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_exit.c,v 1.220 2024/01/19 01:43:26 bluhm Exp $	*/
+/*	$OpenBSD: kern_exit.c,v 1.221 2024/05/20 10:32:20 claudio Exp $	*/
 /*	$NetBSD: kern_exit.c,v 1.39 1996/04/22 01:38:25 christos Exp $	*/
 
 /*
@@ -132,8 +132,6 @@ exit1(struct proc *p, int xexit, int xsig, int flags)
 		/* nope, multi-threaded */
 		if (flags == EXIT_NORMAL)
 			single_thread_set(p, SINGLE_EXIT);
-		else if (flags == EXIT_THREAD)
-			single_thread_check(p, 0);
 	}
 
 	if (flags == EXIT_NORMAL && !(pr->ps_flags & PS_EXITING)) {
@@ -157,15 +155,27 @@ exit1(struct proc *p, int xexit, int xsig, int flags)
 	}
 
 	/* unlink ourselves from the active threads */
-	SCHED_LOCK(s);
+	mtx_enter(&pr->ps_mtx);
 	TAILQ_REMOVE(&pr->ps_threads, p, p_thr_link);
-	SCHED_UNLOCK(s);
+	pr->ps_threadcnt--;
+	pr->ps_exitcnt++;
+
+	/*
+	 * if somebody else wants to take us to single threaded mode,
+	 * count ourselves out.
+	 */
+	if (pr->ps_single) {
+		if (--pr->ps_singlecnt == 0)
+			wakeup(&pr->ps_singlecnt);
+	}
 
 	if ((p->p_flag & P_THREAD) == 0) {
 		/* main thread gotta wait because it has the pid, et al */
-		while (pr->ps_threadcnt > 1)
-			tsleep_nsec(&pr->ps_threads, PWAIT, "thrdeath", INFSLP);
+		while (pr->ps_threadcnt + pr->ps_exitcnt > 1)
+			msleep_nsec(&pr->ps_threads, &pr->ps_mtx, PWAIT,
+			    "thrdeath", INFSLP);
 	}
+	mtx_leave(&pr->ps_mtx);
 
 	rup = pr->ps_ru;
 	if (rup == NULL) {
@@ -352,9 +362,11 @@ exit1(struct proc *p, int xexit, int xsig, int flags)
 	/* just a thread? detach it from its process */
 	if (p->p_flag & P_THREAD) {
 		/* scheduler_wait_hook(pr->ps_mainproc, p); XXX */
-		if (--pr->ps_threadcnt == 1)
+		mtx_enter(&pr->ps_mtx);
+		pr->ps_exitcnt--;
+		if (pr->ps_threadcnt + pr->ps_exitcnt == 1)
 			wakeup(&pr->ps_threads);
-		KASSERT(pr->ps_threadcnt > 0);
+		mtx_leave(&pr->ps_mtx);
 	}
 
 	/*
@@ -829,7 +841,8 @@ process_zap(struct process *pr)
 	if (otvp)
 		vrele(otvp);
 
-	KASSERT(pr->ps_threadcnt == 1);
+	KASSERT(pr->ps_threadcnt == 0);
+	KASSERT(pr->ps_exitcnt == 1);
 	if (pr->ps_ptstat != NULL)
 		free(pr->ps_ptstat, M_SUBPROC, sizeof(*pr->ps_ptstat));
 	pool_put(&rusage_pool, pr->ps_ru);
diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index 90acfdc09b2..1cfb84d092b 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_fork.c,v 1.257 2024/01/24 19:23:38 cheloha Exp $	*/
+/*	$OpenBSD: kern_fork.c,v 1.258 2024/05/20 10:32:20 claudio Exp $	*/
 /*	$NetBSD: kern_fork.c,v 1.29 1996/02/09 18:59:34 christos Exp $	*/
 
 /*
@@ -535,7 +535,7 @@ thread_fork(struct proc *curp, void *stack, void *tcb, pid_t *tidptr,
 	struct proc *p;
 	pid_t tid;
 	vaddr_t uaddr;
-	int s, error;
+	int error;
 
 	if (stack == NULL)
 		return EINVAL;
@@ -559,7 +559,6 @@ thread_fork(struct proc *curp, void *stack, void *tcb, pid_t *tidptr,
 
 	/* other links */
 	p->p_p = pr;
-	pr->ps_threadcnt++;
 
 	/* local copies */
 	p->p_fd		= pr->ps_fd;
@@ -578,18 +577,19 @@ thread_fork(struct proc *curp, void *stack, void *tcb, pid_t *tidptr,
 	LIST_INSERT_HEAD(&allproc, p, p_list);
 	LIST_INSERT_HEAD(TIDHASH(p->p_tid), p, p_hash);
 
-	SCHED_LOCK(s);
+	mtx_enter(&pr->ps_mtx);
 	TAILQ_INSERT_TAIL(&pr->ps_threads, p, p_thr_link);
+	pr->ps_threadcnt++;
 
 	/*
 	 * if somebody else wants to take us to single threaded mode,
 	 * count ourselves in.
 	 */
 	if (pr->ps_single) {
-		atomic_inc_int(&pr->ps_singlecount);
+		pr->ps_singlecnt++;
 		atomic_setbits_int(&p->p_flag, P_SUSPSINGLE);
 	}
-	SCHED_UNLOCK(s);
+	mtx_leave(&pr->ps_mtx);
 
 	/*
 	 * Return tid to parent thread and copy it out to userspace
diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c
index 165d10c6817..ee05f6a6d0c 100644
--- a/sys/kern/kern_proc.c
+++ b/sys/kern/kern_proc.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_proc.c,v 1.97 2024/01/19 01:43:27 bluhm Exp $	*/
+/*	$OpenBSD: kern_proc.c,v 1.98 2024/05/20 10:32:20 claudio Exp $	*/
 /*	$NetBSD: kern_proc.c,v 1.14 1996/02/09 18:59:41 christos Exp $	*/
 
 /*
@@ -495,9 +495,9 @@ proc_printit(struct proc *p, const char *modif,
 	    p->p_p->ps_flags, PS_BITS, p->p_flag, P_BITS);
 	(*pr)("    runpri=%u, usrpri=%u, slppri=%u, nice=%d\n",
 	    p->p_runpri, p->p_usrpri, p->p_slppri, p->p_p->ps_nice);
-	(*pr)("    wchan=%p, wmesg=%s, ps_single=%p\n",
+	(*pr)("    wchan=%p, wmesg=%s, ps_single=%p scnt=%d ecnt=%d\n",
 	    p->p_wchan, (p->p_wchan && p->p_wmesg) ?  p->p_wmesg : "",
-	    p->p_p->ps_single);
+	    p->p_p->ps_single, p->p_p->ps_singlecnt, p->p_p->ps_exitcnt);
 	(*pr)("    forw=%p, list=%p,%p\n",
 	    TAILQ_NEXT(p, p_runq), p->p_list.le_next, p->p_list.le_prev);
 	(*pr)("    process=%p user=%p, vmspace=%p\n",
diff --git a/sys/kern/kern_resource.c b/sys/kern/kern_resource.c
index 789c980625d..04247e33094 100644
--- a/sys/kern/kern_resource.c
+++ b/sys/kern/kern_resource.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_resource.c,v 1.81 2024/04/17 09:41:44 claudio Exp $	*/
+/*	$OpenBSD: kern_resource.c,v 1.82 2024/05/20 10:32:20 claudio Exp $	*/
 /*	$NetBSD: kern_resource.c,v 1.38 1996/10/23 07:19:38 matthias Exp $	*/
 
 /*-
@@ -212,11 +212,13 @@ donice(struct proc *curp, struct process *chgpr, int n)
 	if (n < chgpr->ps_nice && suser(curp))
 		return (EACCES);
 	chgpr->ps_nice = n;
-	SCHED_LOCK(s);
+	mtx_enter(&chgpr->ps_mtx);
 	TAILQ_FOREACH(p, &chgpr->ps_threads, p_thr_link) {
+		SCHED_LOCK(s);
 		setpriority(p, p->p_estcpu, n);
+		SCHED_UNLOCK(s);
 	}
-	SCHED_UNLOCK(s);
+	mtx_leave(&chgpr->ps_mtx);
 	return (0);
 }
 
diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index d5b896cbfed..ae6a2bea196 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_sig.c,v 1.327 2024/05/08 13:05:33 claudio Exp $	*/
+/*	$OpenBSD: kern_sig.c,v 1.328 2024/05/20 10:32:20 claudio Exp $	*/
 /*	$NetBSD: kern_sig.c,v 1.54 1996/04/22 01:38:32 christos Exp $	*/
 
 /*
@@ -2060,11 +2060,12 @@ userret(struct proc *p)
 }
 
 int
-single_thread_check_locked(struct proc *p, int deep, int s)
+single_thread_check_locked(struct proc *p, int deep)
 {
 	struct process *pr = p->p_p;
+	int s;
 
-	SCHED_ASSERT_LOCKED();
+	MUTEX_ASSERT_LOCKED(&pr->ps_mtx);
 
 	if (pr->ps_single == NULL || pr->ps_single == p)
 		return (0);
@@ -2078,19 +2079,24 @@ single_thread_check_locked(struct proc *p, int deep, int s)
 				return (EINTR);
 		}
 
-		if (atomic_dec_int_nv(&pr->ps_singlecount) == 0)
-			wakeup(&pr->ps_singlecount);
-
 		if (pr->ps_flags & PS_SINGLEEXIT) {
-			SCHED_UNLOCK(s);
+			mtx_leave(&pr->ps_mtx);
 			KERNEL_LOCK();
 			exit1(p, 0, 0, EXIT_THREAD_NOCHECK);
 			/* NOTREACHED */
 		}
 
+		if (--pr->ps_singlecnt == 0)
+			wakeup(&pr->ps_singlecnt);
+
 		/* not exiting and don't need to unwind, so suspend */
+		mtx_leave(&pr->ps_mtx);
+
+		SCHED_LOCK(s);
 		p->p_stat = SSTOP;
 		mi_switch();
+		SCHED_UNLOCK(s);
+		mtx_enter(&pr->ps_mtx);
 	} while (pr->ps_single != NULL);
 
 	return (0);
@@ -2099,11 +2105,11 @@ single_thread_check_locked(struct proc *p, int deep, int s)
 int
 single_thread_check(struct proc *p, int deep)
 {
-	int s, error;
+	int error;
 
-	SCHED_LOCK(s);
-	error = single_thread_check_locked(p, deep, s);
-	SCHED_UNLOCK(s);
+	mtx_enter(&p->p_p->ps_mtx);
+	error = single_thread_check_locked(p, deep);
+	mtx_leave(&p->p_p->ps_mtx);
 
 	return error;
 }
@@ -2126,10 +2132,10 @@ single_thread_set(struct proc *p, int flags)
 
 	KASSERT(curproc == p);
 
-	SCHED_LOCK(s);
-	error = single_thread_check_locked(p, flags & SINGLE_DEEP, s);
+	mtx_enter(&pr->ps_mtx);
+	error = single_thread_check_locked(p, flags & SINGLE_DEEP);
 	if (error) {
-		SCHED_UNLOCK(s);
+		mtx_leave(&pr->ps_mtx);
 		return error;
 	}
 
@@ -2148,27 +2154,17 @@ single_thread_set(struct proc *p, int flags)
 		panic("single_thread_mode = %d", mode);
 #endif
 	}
-	pr->ps_singlecount = 0;
-	membar_producer();
 	pr->ps_single = p;
+	pr->ps_singlecnt = pr->ps_threadcnt;
+
 	TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
 		if (q == p)
 			continue;
-		if (q->p_flag & P_WEXIT) {
-			if (mode == SINGLE_EXIT) {
-				if (q->p_stat == SSTOP) {
-					unsleep(q);
-					setrunnable(q);
-					atomic_inc_int(&pr->ps_singlecount);
-				}
-			}
-			continue;
-		}
+		SCHED_LOCK(s);
 		atomic_setbits_int(&q->p_flag, P_SUSPSINGLE);
 		switch (q->p_stat) {
 		case SIDL:
-		case SRUN:
-			atomic_inc_int(&pr->ps_singlecount);
+		case SDEAD:
 			break;
 		case SSLEEP:
 			/* if it's not interruptible, then just have to wait */
@@ -2176,30 +2172,33 @@ single_thread_set(struct proc *p, int flags)
 				/* merely need to suspend?  just stop it */
 				if (mode == SINGLE_SUSPEND) {
 					q->p_stat = SSTOP;
+					--pr->ps_singlecnt;
 					break;
 				}
 				/* need to unwind or exit, so wake it */
 				unsleep(q);
 				setrunnable(q);
 			}
-			atomic_inc_int(&pr->ps_singlecount);
 			break;
 		case SSTOP:
 			if (mode == SINGLE_EXIT) {
 				unsleep(q);
 				setrunnable(q);
-				atomic_inc_int(&pr->ps_singlecount);
-			}
-			break;
-		case SDEAD:
+			} else
+				--pr->ps_singlecnt;
 			break;
 		case SONPROC:
-			atomic_inc_int(&pr->ps_singlecount);
 			signotify(q);
+			/* FALLTHROUGH */
+		case SRUN:
 			break;
 		}
+		SCHED_UNLOCK(s);
 	}
-	SCHED_UNLOCK(s);
+
+	/* count ourselfs out */
+	--pr->ps_singlecnt;
+	mtx_leave(&pr->ps_mtx);
 
 	if ((flags & SINGLE_NOWAIT) == 0)
 		single_thread_wait(pr, 1);
@@ -2218,14 +2217,14 @@ single_thread_wait(struct process *pr, int recheck)
 	int wait;
 
 	/* wait until they're all suspended */
-	wait = pr->ps_singlecount > 0;
-	while (wait) {
-		sleep_setup(&pr->ps_singlecount, PWAIT, "suspend");
-		wait = pr->ps_singlecount > 0;
-		sleep_finish(0, wait);
+	mtx_enter(&pr->ps_mtx);
+	while ((wait = pr->ps_singlecnt > 0)) {
+		msleep_nsec(&pr->ps_singlecnt, &pr->ps_mtx, PWAIT, "suspend",
+		    INFSLP);
 		if (!recheck)
 			break;
 	}
+	mtx_leave(&pr->ps_mtx);
 
 	return wait;
 }
@@ -2240,9 +2239,10 @@ single_thread_clear(struct proc *p, int flag)
 	KASSERT(pr->ps_single == p);
 	KASSERT(curproc == p);
 
-	SCHED_LOCK(s);
+	mtx_enter(&pr->ps_mtx);
 	pr->ps_single = NULL;
 	atomic_clearbits_int(&pr->ps_flags, PS_SINGLEUNWIND | PS_SINGLEEXIT);
+
 	TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
 		if (q == p || (q->p_flag & P_SUSPSINGLE) == 0)
 			continue;
@@ -2253,6 +2253,7 @@ single_thread_clear(struct proc *p, int flag)
 		 * then clearing that either makes it runnable or puts
 		 * it back into some sleep queue
 		 */
+		SCHED_LOCK(s);
 		if (q->p_stat == SSTOP && (q->p_flag & flag) == 0) {
 			if (q->p_wchan == NULL)
 				setrunnable(q);
@@ -2261,8 +2262,9 @@ single_thread_clear(struct proc *p, int flag)
 				q->p_stat = SSLEEP;
 			}
 		}
+		SCHED_UNLOCK(s);
 	}
-	SCHED_UNLOCK(s);
+	mtx_leave(&pr->ps_mtx);
 }
 
 void
diff --git a/sys/kern/kern_synch.c b/sys/kern/kern_synch.c
index e613b67742e..44fe8974534 100644
--- a/sys/kern/kern_synch.c
+++ b/sys/kern/kern_synch.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_synch.c,v 1.202 2024/04/18 08:59:38 claudio Exp $	*/
+/*	$OpenBSD: kern_synch.c,v 1.203 2024/05/20 10:32:20 claudio Exp $	*/
 /*	$NetBSD: kern_synch.c,v 1.37 1996/04/22 01:38:37 christos Exp $	*/
 
 /*
@@ -581,15 +581,18 @@ sys_sched_yield(struct proc *p, void *v, register_t *retval)
 	uint8_t newprio;
 	int s;
 
-	SCHED_LOCK(s);
 	/*
 	 * If one of the threads of a multi-threaded process called
 	 * sched_yield(2), drop its priority to ensure its siblings
 	 * can make some progress.
 	 */
+	mtx_enter(&p->p_p->ps_mtx);
 	newprio = p->p_usrpri;
 	TAILQ_FOREACH(q, &p->p_p->ps_threads, p_thr_link)
 		newprio = max(newprio, q->p_runpri);
+	mtx_leave(&p->p_p->ps_mtx);
+
+	SCHED_LOCK(s);
 	setrunqueue(p->p_cpu, p, newprio);
 	p->p_ru.ru_nvcsw++;
 	mi_switch();
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 41df671728c..53f5bf9b61c 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: proc.h,v 1.360 2024/04/18 10:29:39 claudio Exp $	*/
+/*	$OpenBSD: proc.h,v 1.361 2024/05/20 10:32:20 claudio Exp $	*/
 /*	$NetBSD: proc.h,v 1.44 1996/04/22 01:23:21 christos Exp $	*/
 
 /*-
@@ -145,7 +145,7 @@ struct process {
 	struct	ucred *ps_ucred;	/* Process owner's identity. */
 
 	LIST_ENTRY(process) ps_list;	/* List of all processes. */
-	TAILQ_HEAD(,proc) ps_threads;	/* [K|S] Threads in this process. */
+	TAILQ_HEAD(,proc) ps_threads;	/* [K|m] Threads in this process. */
 
 	LIST_ENTRY(process) ps_pglist;	/* List of processes in pgrp. */
 	struct	process *ps_pptr; 	/* Pointer to parent process. */
@@ -180,8 +180,9 @@ struct process {
 	u_int	ps_flags;		/* [a] PS_* flags. */
 	int	ps_siglist;		/* Signals pending for the process. */
 
-	struct	proc *ps_single;	/* [S] Thread for single-threading. */
-	u_int	ps_singlecount;		/* [a] Not yet suspended threads. */
+	struct	proc *ps_single;	/* [m] Thread for single-threading. */
+	u_int	ps_singlecnt;		/* [m] Number of threads to suspend. */
+	u_int	ps_exitcnt;		/* [m] Number of threads in exit1. */
 
 	int	ps_traceflag;		/* Kernel trace points. */
 	struct	vnode *ps_tracevp;	/* Trace to vnode. */
@@ -252,7 +253,7 @@ struct process {
 
 /* End area that is copied on creation. */
 #define ps_endcopy	ps_threadcnt
-	u_int	ps_threadcnt;		/* Number of threads. */
+	u_int	ps_threadcnt;		/* [m] Number of threads. */
 
 	struct	timespec ps_start;	/* starting uptime. */
 	struct	timeout ps_realit_to;	/* [m] ITIMER_REAL timeout */
@@ -322,13 +323,14 @@ struct p_inentry {
  *	U	uidinfolk
  *	l	read only reference, see lim_read_enter()
  *	o	owned (modified only) by this thread
+ *	m	this proc's' `p->p_p->ps_mtx'
  */
 struct proc {
 	TAILQ_ENTRY(proc) p_runq;	/* [S] current run/sleep queue */
 	LIST_ENTRY(proc) p_list;	/* List of all threads. */
 
 	struct	process *p_p;		/* [I] The process of this thread. */
-	TAILQ_ENTRY(proc) p_thr_link;	/* Threads in a process linkage. */
+	TAILQ_ENTRY(proc) p_thr_link;	/* [K|m] Threads in a process linkage. */
 
 	TAILQ_ENTRY(proc) p_fut_link;	/* Threads in a futex linkage. */
 	struct	futex	*p_futex;	/* Current sleeping futex. */