Skip site navigation (1)Skip section navigation (2)
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>