- Fix the kthread_{suspend, resume, suspend_check}() locking.
In the current code, the locking is completely broken and may lead easilly to deadlocks. Fix it by using the proc_mtx, linked to the suspending thread, as lock for the operation. Keep using the thread_lock for setting and reading the flag even if it is not entirely necessary (atomic ops may do it as well, but this way the code is more readable). - Fix a deadlock within kthread_suspend(). The suspender should not sleep on a different channel wrt the suspended thread, or, otherwise, the awaker should wakeup both. Uniform the interface to what the kproc_* counterparts do (sleeping on the same channel). - Change the kthread_suspend_check() prototype. kthread_suspend_check() always assumes curthread and must only refer to it, so skip the thread pointer as it may be easilly mistaken. If curthread is not a kthread, the system will panic. In collabouration with: jhb Tested by: Giovanni Trematerra <giovanni dot trematerra at gmail dot com> MFC: 2 weeks
This commit is contained in:
parent
ef74982dc0
commit
a50e80dcdd
Notes:
svn2git
2020-12-20 02:59:44 +00:00
svn path=/head/; revision=202933
@ -25,7 +25,7 @@
|
||||
.\"
|
||||
.\" $FreeBSD$
|
||||
.\"
|
||||
.Dd January 26, 2009
|
||||
.Dd January 24, 2010
|
||||
.Dt KTHREAD 9
|
||||
.Os
|
||||
.Sh NAME
|
||||
@ -50,7 +50,7 @@
|
||||
.Ft int
|
||||
.Fn kthread_suspend "struct thread *td" "int timo"
|
||||
.Ft void
|
||||
.Fn kthread_suspend_check "struct thread *td"
|
||||
.Fn kthread_suspend_check "void"
|
||||
.In sys/unistd.h
|
||||
.Ft int
|
||||
.Fo kthread_add
|
||||
@ -208,12 +208,9 @@ functions are used to suspend and resume a kernel thread.
|
||||
During the main loop of its execution, a kernel thread that wishes to allow
|
||||
itself to be suspended should call
|
||||
.Fn kthread_suspend_check
|
||||
passing in
|
||||
.Va curthread
|
||||
as the only argument.
|
||||
This function checks to see if the kernel thread has been asked to suspend.
|
||||
in order to check if the it has been asked to suspend.
|
||||
If it has, it will
|
||||
.Xr tsleep 9
|
||||
.Xr msleep 9
|
||||
until it is told to resume.
|
||||
Once it has been told to resume it will return allowing execution of the
|
||||
kernel thread to continue.
|
||||
|
@ -335,34 +335,55 @@ kthread_exit(void)
|
||||
int
|
||||
kthread_suspend(struct thread *td, int timo)
|
||||
{
|
||||
if ((td->td_pflags & TDP_KTHREAD) == 0) {
|
||||
struct proc *p;
|
||||
|
||||
p = td->td_proc;
|
||||
|
||||
/*
|
||||
* td_pflags should not be ready by any other thread different by
|
||||
* curthread, but as long as this flag is invariant during the
|
||||
* thread lifetime, it is ok to check for it now.
|
||||
*/
|
||||
if ((td->td_pflags & TDP_KTHREAD) == 0)
|
||||
return (EINVAL);
|
||||
}
|
||||
|
||||
/*
|
||||
* The caller of the primitive should have already checked that the
|
||||
* thread is up and running, thus not being blocked by other
|
||||
* conditions.
|
||||
*/
|
||||
PROC_LOCK(p);
|
||||
thread_lock(td);
|
||||
td->td_flags |= TDF_KTH_SUSP;
|
||||
thread_unlock(td);
|
||||
/*
|
||||
* If it's stopped for some other reason,
|
||||
* kick it to notice our request
|
||||
* or we'll end up timing out
|
||||
*/
|
||||
wakeup(td); /* traditional place for kernel threads to sleep on */ /* XXX ?? */
|
||||
return (tsleep(&td->td_flags, PPAUSE | PDROP, "suspkt", timo));
|
||||
return (msleep(&td->td_flags, &p->p_mtx, PPAUSE | PDROP, "suspkt",
|
||||
timo));
|
||||
}
|
||||
|
||||
/*
|
||||
* let the kthread it can keep going again.
|
||||
* Resume a thread previously put asleep with kthread_suspend().
|
||||
*/
|
||||
int
|
||||
kthread_resume(struct thread *td)
|
||||
{
|
||||
if ((td->td_pflags & TDP_KTHREAD) == 0) {
|
||||
struct proc *p;
|
||||
|
||||
p = td->td_proc;
|
||||
|
||||
/*
|
||||
* td_pflags should not be ready by any other thread different by
|
||||
* curthread, but as long as this flag is invariant during the
|
||||
* thread lifetime, it is ok to check for it now.
|
||||
*/
|
||||
if ((td->td_pflags & TDP_KTHREAD) == 0)
|
||||
return (EINVAL);
|
||||
}
|
||||
|
||||
PROC_LOCK(p);
|
||||
thread_lock(td);
|
||||
td->td_flags &= ~TDF_KTH_SUSP;
|
||||
thread_unlock(td);
|
||||
wakeup(&td->td_name);
|
||||
wakeup(&td->td_flags);
|
||||
PROC_UNLOCK(p);
|
||||
return (0);
|
||||
}
|
||||
|
||||
@ -371,15 +392,28 @@ kthread_resume(struct thread *td)
|
||||
* and notify the caller that is has happened.
|
||||
*/
|
||||
void
|
||||
kthread_suspend_check(struct thread *td)
|
||||
kthread_suspend_check()
|
||||
{
|
||||
struct proc *p;
|
||||
struct thread *td;
|
||||
|
||||
td = curthread;
|
||||
p = td->td_proc;
|
||||
|
||||
if ((td->td_pflags & TDP_KTHREAD) == 0)
|
||||
panic("%s: curthread is not a valid kthread", __func__);
|
||||
|
||||
/*
|
||||
* As long as the double-lock protection is used when accessing the
|
||||
* TDF_KTH_SUSP flag, synchronizing the read operation via proc mutex
|
||||
* is fine.
|
||||
*/
|
||||
PROC_LOCK(p);
|
||||
while (td->td_flags & TDF_KTH_SUSP) {
|
||||
/*
|
||||
* let the caller know we got the message then sleep
|
||||
*/
|
||||
wakeup(&td->td_flags);
|
||||
tsleep(&td->td_name, PPAUSE, "ktsusp", 0);
|
||||
msleep(&td->td_flags, &p->p_mtx, PPAUSE, "ktsusp", 0);
|
||||
}
|
||||
PROC_UNLOCK(p);
|
||||
}
|
||||
|
||||
int
|
||||
|
@ -73,7 +73,7 @@ int kthread_resume(struct thread *);
|
||||
void kthread_shutdown(void *, int);
|
||||
void kthread_start(const void *);
|
||||
int kthread_suspend(struct thread *, int);
|
||||
void kthread_suspend_check(struct thread *);
|
||||
void kthread_suspend_check(void);
|
||||
|
||||
|
||||
#endif /* !_SYS_KTHREAD_H_ */
|
||||
|
Loading…
Reference in New Issue
Block a user