From aa563902a9fb835e591cb776b8d405d10f098e06 Mon Sep 17 00:00:00 2001 From: claudio Date: Tue, 11 Jul 2023 07:02:43 +0000 Subject: [PATCH] Rework sleep_setup()/sleep_finish() to no longer hold the scheduler lock between calls. Instead of forcing an atomic operation across multiple calls use a three step transaction. 1. setup sleep state by calling sleep_setup() 2. recheck sleep condition to ensure that the event did not fire before sleep_setup() registered the proc onto the sleep queue 3. call sleep_finish() to either sleep or keep on running based on the step 2 outcome and any possible signal delivery To make this work wakeup from signals, single thread api and wakeup(9) need to be aware if a process is between step 1 and step 3 so that the process is not enqueued back onto the runqueue while going to sleep. Introduce the p_flag P_WSLEEP to detect this situation. On top of this remove the spl dance in msleep() which is no longer required. It is ok to process interrupts between step 1 and 3. OK mpi@ cheloha@ --- sys/kern/kern_sched.c | 3 ++- sys/kern/kern_sig.c | 6 ++--- sys/kern/kern_synch.c | 59 ++++++++++++++++++++----------------------- sys/kern/sched_bsd.c | 6 ++++- sys/sys/proc.h | 5 ++-- 5 files changed, 41 insertions(+), 38 deletions(-) diff --git a/sys/kern/kern_sched.c b/sys/kern/kern_sched.c index fe179e4ef64..7fae2f8df33 100644 --- a/sys/kern/kern_sched.c +++ b/sys/kern/kern_sched.c @@ -1,4 +1,4 @@ -/* $OpenBSD: kern_sched.c,v 1.77 2023/06/28 08:23:25 claudio Exp $ */ +/* $OpenBSD: kern_sched.c,v 1.78 2023/07/11 07:02:43 claudio Exp $ */ /* * Copyright (c) 2007, 2008 Artur Grabowski * @@ -248,6 +248,7 @@ setrunqueue(struct cpu_info *ci, struct proc *p, uint8_t prio) KASSERT(ci != NULL); SCHED_ASSERT_LOCKED(); + KASSERT(!ISSET(p->p_flag, P_WSLEEP) || p->p_stat == SSTOP); p->p_cpu = ci; p->p_stat = SRUN; diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index 98683fe16ff..d46b229acc9 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -1,4 +1,4 @@ -/* $OpenBSD: kern_sig.c,v 1.308 2023/07/10 22:54:40 deraadt Exp $ */ +/* $OpenBSD: kern_sig.c,v 1.309 2023/07/11 07:02:43 claudio Exp $ */ /* $NetBSD: kern_sig.c,v 1.54 1996/04/22 01:38:32 christos Exp $ */ /* @@ -1151,7 +1151,7 @@ ptsignal(struct proc *p, int signum, enum signal_type type) atomic_clearbits_int(siglist, mask); if (action == SIG_CATCH) goto runfast; - if (p->p_wchan == NULL) + if (p->p_wchan == NULL || p->p_flag & P_WSLEEP) goto run; p->p_stat = SSLEEP; goto out; @@ -2204,7 +2204,7 @@ single_thread_clear(struct proc *p, int flag) * it back into some sleep queue */ if (q->p_stat == SSTOP && (q->p_flag & flag) == 0) { - if (q->p_wchan == NULL) + if (p->p_wchan == NULL || p->p_flag & P_WSLEEP) setrunnable(q); else q->p_stat = SSLEEP; diff --git a/sys/kern/kern_synch.c b/sys/kern/kern_synch.c index e6e1a472059..c856ab7903d 100644 --- a/sys/kern/kern_synch.c +++ b/sys/kern/kern_synch.c @@ -1,4 +1,4 @@ -/* $OpenBSD: kern_synch.c,v 1.193 2023/06/28 08:23:25 claudio Exp $ */ +/* $OpenBSD: kern_synch.c,v 1.194 2023/07/11 07:02:43 claudio Exp $ */ /* $NetBSD: kern_synch.c,v 1.37 1996/04/22 01:38:37 christos Exp $ */ /* @@ -246,21 +246,12 @@ msleep(const volatile void *ident, struct mutex *mtx, int priority, sleep_setup(&sls, ident, priority, wmesg); - /* XXX - We need to make sure that the mutex doesn't - * unblock splsched. This can be made a bit more - * correct when the sched_lock is a mutex. - */ - spl = MUTEX_OLDIPL(mtx); - MUTEX_OLDIPL(mtx) = splsched(); mtx_leave(mtx); /* signal may stop the process, release mutex before that */ error = sleep_finish(&sls, priority, timo, 1); - if ((priority & PNORELOCK) == 0) { + if ((priority & PNORELOCK) == 0) mtx_enter(mtx); - MUTEX_OLDIPL(mtx) = spl; /* put the ipl back */ - } else - splx(spl); return error; } @@ -301,6 +292,7 @@ rwsleep(const volatile void *ident, struct rwlock *rwl, int priority, KASSERT((priority & ~(PRIMASK | PCATCH | PNORELOCK)) == 0); KASSERT(ident != &nowake || ISSET(priority, PCATCH) || timo != 0); + KASSERT(ident != rwl); rw_assert_anylock(rwl); status = rw_status(rwl); @@ -344,6 +336,7 @@ sleep_setup(struct sleep_state *sls, const volatile void *ident, int prio, const char *wmesg) { struct proc *p = curproc; + int s; #ifdef DIAGNOSTIC if (p->p_flag & P_CANTSLEEP) @@ -354,7 +347,7 @@ sleep_setup(struct sleep_state *sls, const volatile void *ident, int prio, panic("tsleep: not SONPROC"); #endif - SCHED_LOCK(sls->sls_s); + SCHED_LOCK(s); TRACEPOINT(sched, sleep, NULL); @@ -362,15 +355,20 @@ sleep_setup(struct sleep_state *sls, const volatile void *ident, int prio, p->p_wmesg = wmesg; p->p_slptime = 0; p->p_slppri = prio & PRIMASK; + atomic_setbits_int(&p->p_flag, P_WSLEEP); TAILQ_INSERT_TAIL(&slpque[LOOKUP(ident)], p, p_runq); + if (prio & PCATCH) + atomic_setbits_int(&p->p_flag, P_SINTR); + p->p_stat = SSLEEP; + SCHED_UNLOCK(s); } int sleep_finish(struct sleep_state *sls, int prio, int timo, int do_sleep) { struct proc *p = curproc; - int catch, error = 0, error1 = 0; + int s, catch, error = 0, error1 = 0; catch = prio & PCATCH; @@ -379,6 +377,7 @@ sleep_finish(struct sleep_state *sls, int prio, int timo, int do_sleep) timeout_add(&p->p_sleep_to, timo); } + SCHED_LOCK(s); if (catch != 0) { /* * We put ourselves on the sleep queue and start our @@ -388,28 +387,28 @@ sleep_finish(struct sleep_state *sls, int prio, int timo, int do_sleep) * us to be marked as SSLEEP without resuming us, thus * we must be ready for sleep when sleep_signal_check() is * called. - * If the wakeup happens while we're stopped, p->p_wchan - * will be NULL upon return from sleep_signal_check(). In - * that case we need to unwind immediately. */ - atomic_setbits_int(&p->p_flag, P_SINTR); if ((error = sleep_signal_check()) != 0) { - p->p_stat = SONPROC; - catch = 0; - do_sleep = 0; - } else if (p->p_wchan == NULL) { catch = 0; do_sleep = 0; } } + /* + * If the wakeup happens while going to sleep, p->p_wchan + * will be NULL. In that case unwind immediately but still + * check for possible signals and timeouts. + */ + if (p->p_wchan == NULL) + do_sleep = 0; + + atomic_clearbits_int(&p->p_flag, P_WSLEEP); if (do_sleep) { - p->p_stat = SSLEEP; p->p_ru.ru_nvcsw++; - SCHED_ASSERT_LOCKED(); mi_switch(); } else { unsleep(p); + p->p_stat = SONPROC; } #ifdef DIAGNOSTIC @@ -418,7 +417,7 @@ sleep_finish(struct sleep_state *sls, int prio, int timo, int do_sleep) #endif p->p_cpu->ci_schedstate.spc_curpriority = p->p_usrpri; - SCHED_UNLOCK(sls->sls_s); + SCHED_UNLOCK(s); /* * Even though this belongs to the signal handling part of sleep, @@ -482,8 +481,12 @@ wakeup_proc(struct proc *p, const volatile void *chan, int flags) atomic_setbits_int(&p->p_flag, flags); if (p->p_stat == SSLEEP) setrunnable(p); - else + else if (p->p_stat == SSTOP) unsleep(p); +#ifdef DIAGNOSTIC + else + panic("wakeup: p_stat is %d", (int)p->p_stat); +#endif } return awakened; @@ -538,12 +541,6 @@ wakeup_n(const volatile void *ident, int n) qp = &slpque[LOOKUP(ident)]; for (p = TAILQ_FIRST(qp); p != NULL && n != 0; p = pnext) { pnext = TAILQ_NEXT(p, p_runq); - /* - * This happens if wakeup(9) is called after enqueuing - * itself on the sleep queue and both `ident' collide. - */ - if (p == curproc) - continue; #ifdef DIAGNOSTIC if (p->p_stat != SSLEEP && p->p_stat != SSTOP) panic("wakeup: p_stat is %d", (int)p->p_stat); diff --git a/sys/kern/sched_bsd.c b/sys/kern/sched_bsd.c index 753edaed6f2..cdb2082729c 100644 --- a/sys/kern/sched_bsd.c +++ b/sys/kern/sched_bsd.c @@ -1,4 +1,4 @@ -/* $OpenBSD: sched_bsd.c,v 1.76 2023/06/21 21:16:21 cheloha Exp $ */ +/* $OpenBSD: sched_bsd.c,v 1.77 2023/07/11 07:02:43 claudio Exp $ */ /* $NetBSD: kern_synch.c,v 1.37 1996/04/22 01:38:37 christos Exp $ */ /*- @@ -440,6 +440,10 @@ setrunnable(struct proc *p) case SSLEEP: prio = p->p_slppri; unsleep(p); /* e.g. when sending signals */ + + /* if not yet asleep, don't add to runqueue */ + if (ISSET(p->p_flag, P_WSLEEP)) + return; break; } setrunqueue(NULL, p, prio); diff --git a/sys/sys/proc.h b/sys/sys/proc.h index 176fa3849db..619539e7f1b 100644 --- a/sys/sys/proc.h +++ b/sys/sys/proc.h @@ -1,4 +1,4 @@ -/* $OpenBSD: proc.h,v 1.344 2023/07/10 03:31:58 guenther Exp $ */ +/* $OpenBSD: proc.h,v 1.345 2023/07/11 07:02:43 claudio Exp $ */ /* $NetBSD: proc.h,v 1.44 1996/04/22 01:23:21 christos Exp $ */ /*- @@ -414,6 +414,7 @@ struct proc { #define P_ALRMPEND 0x00000004 /* SIGVTALRM needs to be posted */ #define P_SIGSUSPEND 0x00000008 /* Need to restore before-suspend mask*/ #define P_CANTSLEEP 0x00000010 /* insomniac thread */ +#define P_WSLEEP 0x00000020 /* Working on going to sleep. */ #define P_SINTR 0x00000080 /* Sleep is interruptible. */ #define P_SYSTEM 0x00000200 /* No sigs, stats or swapping. */ #define P_TIMEOUT 0x00000400 /* Timing out during sleep. */ @@ -428,7 +429,7 @@ struct proc { #define P_BITS \ ("\20" "\01INKTR" "\02PROFPEND" "\03ALRMPEND" "\04SIGSUSPEND" \ - "\05CANTSLEEP" "\010SINTR" "\012SYSTEM" "\013TIMEOUT" \ + "\05CANTSLEEP" "\06WSLEEP" "\010SINTR" "\012SYSTEM" "\013TIMEOUT" \ "\016WEXIT" "\020OWEUPC" "\024SUSPSINGLE" "\027XX" \ "\030CONTINUED" "\033THREAD" "\034SUSPSIG" "\035SOFTDEP" "\037CPUPEG") -- 2.20.1