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@

diff --git a/sys/kern/kern_sched.c b/sys/kern/kern_sched.c
index fe179e4..7fae2f8 100644 (file)
--- 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 <art@openbsd.org>
  *
@@ -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 98683fe..d46b229 100644 (file)
--- 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 e6e1a47..c856ab7 100644 (file)
--- 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 753edae..cdb2082 100644 (file)
--- 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 176fa38..619539e 100644 (file)
--- 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")