Re: Towards splitting SCHED_LOCK()
On 14/01/20(Tue) 17:30, Martin Pieuchot wrote:
> 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.
Any strong preference for the name or should I move forward with
wakeup_proc()?
> 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 - 1.55
> +++ dev/pci/drm/drm_linux.c 14 Jan 2020 16:22:38 -
> @@ -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 - 1.241
> +++ kern/kern_sig.c 14 Jan 2020 16:20:42 -
> @@ -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 - 1.157
> +++ kern/kern_synch.c 14 Jan 2020 16:23:02 -
> @@ -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_r
Re: Towards splitting SCHED_LOCK()
Mark Kettenis wrote: > > Date: Tue, 14 Jan 2020 10:34:05 +1100 > > From: Jonathan Gray > > > > 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. If you want an active verb that avoids the adjective confusion, you can consider awaken()
Re: Towards splitting SCHED_LOCK()
> Date: Tue, 14 Jan 2020 10:34:05 +1100
> From: Jonathan Gray
>
> 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 - 1.55
> > +++ dev/pci/drm/drm_linux.c 13 Jan 2020 15:34:44 -
> > @@ -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 - 1.127
> > +++ kern/sys_generic.c 13 Jan 2020 15:35:22 -
> > @@ -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 - 1.156
> > +++ kern/kern_synch.c 13 Jan 2020 15:41:00 -
> > @@ -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 awake
Re: Towards splitting SCHED_LOCK()
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 - 1.55
+++ dev/pci/drm/drm_linux.c 14 Jan 2020 16:22:38 -
@@ -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 - 1.241
+++ kern/kern_sig.c 14 Jan 2020 16:20:42 -
@@ -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 - 1.157
+++ kern/kern_synch.c 14 Jan 2020 16:23:02 -
@@ -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
Re: Towards splitting SCHED_LOCK()
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)
>
> 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 - 1.55
> +++ dev/pci/drm/drm_linux.c 13 Jan 2020 15:34:44 -
> @@ -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.c8 Jan 2020 16:27:41 - 1.127
> +++ kern/sys_generic.c13 Jan 2020 15:35:22 -
> @@ -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 - 1.156
> +++ kern/kern_synch.c 13 Jan 2020 15:41:00 -
> @@ -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);
> +
Re: Towards splitting SCHED_LOCK()
> Date: Mon, 13 Jan 2020 17:02:12 +0100
> From: Martin Pieuchot
>
> 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.
>
> 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().
I think that is actually ok. Can't be 100% sure.
> While here I removed two unnecessary `p_wchan' check before unsleep().
>
> ok?
I think so.
> 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 - 1.55
> +++ dev/pci/drm/drm_linux.c 13 Jan 2020 15:34:44 -
> @@ -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.c8 Jan 2020 16:27:41 - 1.127
> +++ kern/sys_generic.c13 Jan 2020 15:35:22 -
> @@ -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 - 1.156
> +++ kern/kern_synch.c 13 Jan 2020 15:41:00 -
> @@ -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 @
