On 14/01/20(Tue) 10:34, Jonathan Gray wrote:
> On Mon, Jan 13, 2020 at 05:02:12PM +0100, Martin Pieuchot wrote:
> > I'm facing a lock ordering issue while profiling the scheduler which
> > cannot be solved without using a different lock for the global sleepqueue
> > and the runqueues.
> > 
> > The SCHED_LOCK() currently protects both data structures as well as the
> > related fields in 'struct proc'.  One of this fields is `p_wchan' which
> > is obviously related to the global sleepqueue.
> > 
> > The cleaning diff below introduces a new function, awake(), that unify
> > the multiple uses of the following idiom:
> > 
> >     if (p->p_stat == SSLEEP)
> >             setrunnable(p);
> >     else
> >             unsleep(p);
> > 
> > This is generally done after checking if the thread `p' is on the
> > sleepqueue.
> 
> Why not name it wakeup_proc() instead or something like endsleep()?
> 
> awake() does not describe the action that is being done and
> if (awake()) reads like it could be
> if (p->p_stat != SSLEEP && p->p_stat != SSTOP)

Sure, updated diff that also keep the SCHED_LOCK() in endtsleep() as
pointed out by visa@.  The reason is that sleep_finish_timeout() is
executed under the SCHED_LOCK() and we want to ensure the P_TIMEOUT
flag is set and check under this lock to avoid races.

Index: dev/pci/drm/drm_linux.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.c,v
retrieving revision 1.55
diff -u -p -r1.55 drm_linux.c
--- dev/pci/drm/drm_linux.c     5 Jan 2020 13:46:02 -0000       1.55
+++ dev/pci/drm/drm_linux.c     14 Jan 2020 16:22:38 -0000
@@ -115,20 +115,8 @@ schedule_timeout(long timeout)
 int
 wake_up_process(struct proc *p)
 {
-       int s, r = 0;
-
-       SCHED_LOCK(s);
        atomic_cas_ptr(&sch_proc, p, NULL);
-       if (p->p_wchan) {
-               if (p->p_stat == SSLEEP) {
-                       setrunnable(p);
-                       r = 1;
-               } else
-                       unsleep(p);
-       }
-       SCHED_UNLOCK(s);
-
-       return r;
+       return wakeup_proc(p, NULL);
 }
 
 void
Index: kern/kern_sig.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.241
diff -u -p -r1.241 kern_sig.c
--- kern/kern_sig.c     14 Jan 2020 08:52:18 -0000      1.241
+++ kern/kern_sig.c     14 Jan 2020 16:20:42 -0000
@@ -1109,7 +1109,7 @@ ptsignal(struct proc *p, int signum, enu
                 * runnable and can look at the signal.  But don't make
                 * the process runnable, leave it stopped.
                 */
-               if (p->p_wchan && p->p_flag & P_SINTR)
+               if (p->p_flag & P_SINTR)
                        unsleep(p);
                goto out;
 
Index: kern/kern_synch.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_synch.c,v
retrieving revision 1.157
diff -u -p -r1.157 kern_synch.c
--- kern/kern_synch.c   14 Jan 2020 08:52:18 -0000      1.157
+++ kern/kern_synch.c   14 Jan 2020 16:23:02 -0000
@@ -469,8 +469,7 @@ sleep_setup_signal(struct sleep_state *s
         */
        atomic_setbits_int(&p->p_flag, P_SINTR);
        if (p->p_p->ps_single != NULL || (sls->sls_sig = CURSIG(p)) != 0) {
-               if (p->p_wchan)
-                       unsleep(p);
+               unsleep(p);
                p->p_stat = SONPROC;
                sls->sls_do_sleep = 0;
        } else if (p->p_wchan == 0) {
@@ -505,6 +504,25 @@ sleep_finish_signal(struct sleep_state *
        return (error);
 }
 
+int
+wakeup_proc(struct proc *p, const volatile void *chan)
+{
+       int s, awakened = 0;
+
+       SCHED_LOCK(s);
+       if (p->p_wchan != NULL &&
+          ((chan == NULL) || (p->p_wchan == chan))) {
+               awakened = 1;
+               if (p->p_stat == SSLEEP)
+                       setrunnable(p);
+               else
+                       unsleep(p);
+       }
+       SCHED_UNLOCK(s);
+
+       return awakened;
+}
+
 /*
  * Implement timeout for tsleep.
  * If process hasn't been awakened (wchan non-zero),
@@ -518,13 +536,8 @@ endtsleep(void *arg)
        int s;
 
        SCHED_LOCK(s);
-       if (p->p_wchan) {
-               if (p->p_stat == SSLEEP)
-                       setrunnable(p);
-               else
-                       unsleep(p);
+       if (wakeup_proc(p, NULL))
                atomic_setbits_int(&p->p_flag, P_TIMEOUT);
-       }
        SCHED_UNLOCK(s);
 }
 
@@ -536,7 +549,7 @@ unsleep(struct proc *p)
 {
        SCHED_ASSERT_LOCKED();
 
-       if (p->p_wchan) {
+       if (p->p_wchan != NULL) {
                TAILQ_REMOVE(&slpque[LOOKUP(p->p_wchan)], p, p_runq);
                p->p_wchan = NULL;
        }
@@ -570,13 +583,8 @@ wakeup_n(const volatile void *ident, int
                if (p->p_stat != SSLEEP && p->p_stat != SSTOP)
                        panic("wakeup: p_stat is %d", (int)p->p_stat);
 #endif
-               if (p->p_wchan == ident) {
+               if (wakeup_proc(p, ident))
                        --n;
-                       p->p_wchan = 0;
-                       TAILQ_REMOVE(qp, p, p_runq);
-                       if (p->p_stat == SSLEEP)
-                               setrunnable(p);
-               }
        }
        SCHED_UNLOCK(s);
 }
Index: kern/sys_generic.c
===================================================================
RCS file: /cvs/src/sys/kern/sys_generic.c,v
retrieving revision 1.127
diff -u -p -r1.127 sys_generic.c
--- kern/sys_generic.c  8 Jan 2020 16:27:41 -0000       1.127
+++ kern/sys_generic.c  14 Jan 2020 16:22:43 -0000
@@ -767,7 +767,6 @@ void
 selwakeup(struct selinfo *sip)
 {
        struct proc *p;
-       int s;
 
        KNOTE(&sip->si_note, NOTE_SUBMIT);
        if (sip->si_seltid == 0)
@@ -780,15 +779,10 @@ selwakeup(struct selinfo *sip)
        p = tfind(sip->si_seltid);
        sip->si_seltid = 0;
        if (p != NULL) {
-               SCHED_LOCK(s);
-               if (p->p_wchan == (caddr_t)&selwait) {
-                       if (p->p_stat == SSLEEP)
-                               setrunnable(p);
-                       else
-                               unsleep(p);
+               if (wakeup_proc(p, &selwait)) {
+                       /* nothing else to do */
                } else if (p->p_flag & P_SELECT)
                        atomic_clearbits_int(&p->p_flag, P_SELECT);
-               SCHED_UNLOCK(s);
        }
 }
 
Index: kern/vfs_sync.c
===================================================================
RCS file: /cvs/src/sys/kern/vfs_sync.c,v
retrieving revision 1.61
diff -u -p -r1.61 vfs_sync.c
--- kern/vfs_sync.c     8 Dec 2019 12:29:42 -0000       1.61
+++ kern/vfs_sync.c     14 Jan 2020 16:23:06 -0000
@@ -241,12 +241,8 @@ syncer_thread(void *arg)
 int
 speedup_syncer(void)
 {
-       int s;
-
-       SCHED_LOCK(s);
-       if (syncerproc && syncerproc->p_wchan == &lbolt)
-               setrunnable(syncerproc);
-       SCHED_UNLOCK(s);
+       if (syncerproc)
+               wakeup_proc(syncerproc, &lbolt);
        if (rushjob < syncdelay / 2) {
                rushjob += 1;
                stat_rush_requests += 1;
Index: sys/proc.h
===================================================================
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.284
diff -u -p -r1.284 proc.h
--- sys/proc.h  6 Jan 2020 10:25:10 -0000       1.284
+++ sys/proc.h  14 Jan 2020 16:23:16 -0000
@@ -564,6 +564,7 @@ void        procinit(void);
 void   setpriority(struct proc *, uint32_t, uint8_t);
 void   setrunnable(struct proc *);
 void   endtsleep(void *);
+int    wakeup_proc(struct proc *, const volatile void *);
 void   unsleep(struct proc *);
 void   reaper(void *);
 void   exit1(struct proc *, int, int, int);

Reply via email to