From: Tim Leslie <tleslie@protonmail.com>
Subject: Re: Shift several proc.h fields out of SCHED_LOCK
To: Miod Vallat <miod@online.fr>
Cc: "tech@openbsd.org" <tech@openbsd.org>
Date: Sat, 17 Jan 2026 11:21:05 +0000


> > [...] as byte stores are atomic by nature.
> 
> 
> They aren't necessarily. Some platforms (e.g. alpha) are not able to
> perform smaller-than-32-bit load and stores atomically.

Updated to be a small, mechanical diff for just p_estcpu update atomic and to use atomic reads in all in-kernel callers. READ_ONCE/WRITE_ONCE are used for cross-platform safety.

—
Tim Leslie

diff --git a/sys/kern/kern_resource.c b/sys/kern/kern_resource.c
--- a/sys/kern/kern_resource.c
+++ b/sys/kern/kern_resource.c
@@ -213,7 +213,7 @@ donice(struct proc *curp, struct process *chgpr, int n)
 	mtx_enter(&chgpr->ps_mtx);
 	SCHED_LOCK();
 	TAILQ_FOREACH(p, &chgpr->ps_threads, p_thr_link) {
-		setpriority(p, p->p_estcpu, n);
+		setpriority(p, READ_ONCE(p->p_estcpu), n);
 	}
 	SCHED_UNLOCK();
 	mtx_leave(&chgpr->ps_mtx);
diff --git a/sys/kern/sched_bsd.c b/sys/kern/sched_bsd.c
--- a/sys/kern/sched_bsd.c
+++ b/sys/kern/sched_bsd.c
@@ -271,7 +271,7 @@ schedcpu(void *unused)
 #endif
 		p->p_pctcpu = pctcpu;
 		p->p_cpticks2 = cpt;
-		newcpu = (u_int) decay_cpu(loadfac, p->p_estcpu);
+		newcpu = (u_int) decay_cpu(loadfac, READ_ONCE(p->p_estcpu));
 		setpriority(p, newcpu, p->p_p->ps_nice);

 		if (p->p_stat == SRUN &&
@@ -493,7 +493,7 @@ setrunnable(struct proc *p)
 	if (p->p_slptime > 1) {
 		uint32_t newcpu;

-		newcpu = decay_aftersleep(p->p_estcpu, p->p_slptime);
+		newcpu = decay_aftersleep(READ_ONCE(p->p_estcpu), p->p_slptime);
 		setpriority(p, newcpu, pr->ps_nice);
 	}
 	p->p_slptime = 0;
@@ -510,7 +510,7 @@ setpriority(struct proc *p, uint32_t newcpu, uint8_t nice)
 	newprio = min((PUSER + newcpu + NICE_WEIGHT * (nice - NZERO)), MAXPRI);

 	SCHED_ASSERT_LOCKED();
-	p->p_estcpu = newcpu;
+	WRITE_ONCE(p->p_estcpu, newcpu);
 	p->p_usrpri = newprio;
 }

@@ -539,7 +539,7 @@ schedclock(struct proc *p)
 		return;

 	SCHED_LOCK();
-	newcpu = ESTCPULIM(p->p_estcpu + 1);
+	newcpu = ESTCPULIM(READ_ONCE(p->p_estcpu) + 1);
 	setpriority(p, newcpu, p->p_p->ps_nice);
 	SCHED_UNLOCK();
 }
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -357,6 +357,7 @@ struct p_inentry {
  *	l	read only reference, see lim_read_enter()
  *	o	owned (modified only) by this thread
  *	m	this proc's' `p->p_p->ps_mtx'
+ *	a	modified atomically
  */
 struct proc {
 	TAILQ_ENTRY(proc) p_runq;	/* [S] current run/sleep queue */
@@ -414,7 +415,7 @@ struct proc {
 	char	p_name[_MAXCOMLEN];	/* thread name, incl NUL */
 	u_char	p_slppri;		/* [S] Sleeping priority */
 	u_char	p_usrpri;	/* [S] Priority based on p_estcpu & ps_nice */
-	u_int	p_estcpu;		/* [S] Time averaged val of p_cpticks */
+	u_int	p_estcpu;		/* [a] Time averaged val of p_cpticks */
 	int	p_pledge_syscall;	/* Cache of current syscall */
 	uint64_t p_pledge;		/* [o] copy of p_p->ps_pledge */

diff --git a/sys/sys/sched.h b/sys/sys/sched.h
--- a/sys/sys/sched.h
+++ b/sys/sys/sched.h
@@ -194,7 +194,8 @@ void remrunqueue(struct proc *);

 /* Chargeback parents for the sins of their children.  */
 #define scheduler_wait_hook(parent, child) do {				\
-	(parent)->p_estcpu = ESTCPULIM((parent)->p_estcpu + (child)->p_estcpu);\
+	(parent)->p_estcpu = ESTCPULIM(READ_ONCE((parent)->p_estcpu) + \
+	   READ_ONCE((child)->p_estcpu));\
 } while (0)

 /* Allow other processes to progress */
diff --git a/sys/sys/sysctl.h b/sys/sys/sysctl.h
--- a/sys/sys/sysctl.h
+++ b/sys/sys/sysctl.h
@@ -625,7 +625,7 @@ do {									\
 									\
 	(kp)->p_jobc = (pg)->pg_jobc;					\
 									\
-	(kp)->p_estcpu = (p)->p_estcpu;					\
+	(kp)->p_estcpu = READ_ONCE((p)->p_estcpu);					\
 	if (isthread) {							\
 		(kp)->p_tid = (p)->p_tid + THREAD_PID_OFFSET;		\
 		strlcpy((kp)->p_name, (p)->p_name, sizeof((kp)->p_name)); \