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