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);