Date: Sun, 24 Jan 2010 15:07:00 +0000 (UTC) From: Attilio Rao <attilio@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r202933 - in head: share/man/man9 sys/kern sys/sys Message-ID: <201001241507.o0OF70wG063398@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: attilio Date: Sun Jan 24 15:07:00 2010 New Revision: 202933 URL: http://svn.freebsd.org/changeset/base/202933 Log: - 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 Modified: head/share/man/man9/kthread.9 head/sys/kern/kern_kthread.c head/sys/sys/kthread.h Modified: head/share/man/man9/kthread.9 ============================================================================== --- head/share/man/man9/kthread.9 Sun Jan 24 14:58:49 2010 (r202932) +++ head/share/man/man9/kthread.9 Sun Jan 24 15:07:00 2010 (r202933) @@ -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 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. Modified: head/sys/kern/kern_kthread.c ============================================================================== --- head/sys/kern/kern_kthread.c Sun Jan 24 14:58:49 2010 (r202932) +++ head/sys/kern/kern_kthread.c Sun Jan 24 15:07:00 2010 (r202933) @@ -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 Modified: head/sys/sys/kthread.h ============================================================================== --- head/sys/sys/kthread.h Sun Jan 24 14:58:49 2010 (r202932) +++ head/sys/sys/kthread.h Sun Jan 24 15:07:00 2010 (r202933) @@ -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_ */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201001241507.o0OF70wG063398>