> Date: Tue, 14 Jan 2020 10:34:05 +1100 > From: Jonathan Gray <j...@jsg.id.au> > > 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)
Must say I had reservations about the "awake" name as well, but jsg@ nails it here since it both a verb and an adjective which creates confusion. Both names suggester here are find I think. > > This diff introduces a change in behavior in the Linux compat: > > wake_up_process() will now return 1 if the thread was stopped and not > > sleeping. This should be fine since the only place this value is > > checked is with the combination of task_asleep(). > > > > While here I removed two unnecessary `p_wchan' check before unsleep(). > > > > ok? > > > > 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 13 Jan 2020 15:34:44 -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 awake(p, NULL); > > } > > > > void > > 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 13 Jan 2020 15:35:22 -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 (awake(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/kern_synch.c > > =================================================================== > > RCS file: /cvs/src/sys/kern/kern_synch.c,v > > retrieving revision 1.156 > > diff -u -p -r1.156 kern_synch.c > > --- kern/kern_synch.c 12 Jan 2020 00:01:12 -0000 1.156 > > +++ kern/kern_synch.c 13 Jan 2020 15:41:00 -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 > > +awake(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), > > @@ -515,17 +533,9 @@ void > > endtsleep(void *arg) > > { > > struct proc *p = arg; > > - int s; > > > > - SCHED_LOCK(s); > > - if (p->p_wchan) { > > - if (p->p_stat == SSLEEP) > > - setrunnable(p); > > - else > > - unsleep(p); > > + if (awake(p, NULL)) > > atomic_setbits_int(&p->p_flag, P_TIMEOUT); > > - } > > - SCHED_UNLOCK(s); > > } > > > > /* > > @@ -536,7 +546,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 +580,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 (awake(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/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 13 Jan 2020 15:25:02 -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) > > + awake(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 13 Jan 2020 15:35:52 -0000 > > @@ -564,6 +564,7 @@ void procinit(void); > > void setpriority(struct proc *, uint32_t, uint8_t); > > void setrunnable(struct proc *); > > void endtsleep(void *); > > +int awake(struct proc *, const volatile void *); > > void unsleep(struct proc *); > > void reaper(void *); > > void exit1(struct proc *, int, int, int); > > Index: kern/kern_sig.c > > =================================================================== > > RCS file: /cvs/src/sys/kern/kern_sig.c,v > > retrieving revision 1.240 > > diff -u -p -r1.240 kern_sig.c > > --- kern/kern_sig.c 8 Jan 2020 16:27:41 -0000 1.240 > > +++ kern/kern_sig.c 13 Jan 2020 15:36:37 -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; > > > > > > > >