Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 15 May 2015 13:50:38 +0000 (UTC)
From:      John Baldwin <jhb@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r282971 - in head/sys: kern sys
Message-ID:  <201505151350.t4FDocQT054144@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: jhb
Date: Fri May 15 13:50:37 2015
New Revision: 282971
URL: https://svnweb.freebsd.org/changeset/base/282971

Log:
  Previously, cv_waiters was only updated by cv_signal or cv_wait. If a
  thread awakened due to a time out, then cv_waiters was not decremented.
  If INT_MAX threads timed out on a cv without an intervening cv_broadcast,
  then cv_waiters could overflow. To fix this, have each sleeping thread
  decrement cv_waiters when it resumes.
  
  Note that previously cv_waiters was protected by the sleepq chain lock.
  However, that lock is not held when threads resume from sleep. In
  addition, the interlock is also not always reacquired after resuming
  (cv_wait_unlock), nor is it always held by callers of cv_signal() or
  cv_broadcast(). Instead, use atomic ops to update cv_waiters. Since
  the sleepq chain lock is still held on every increment, it should
  still be safe to compare cv_waiters against zero while holding the
  lock in the wakeup routines as the only way the race should be lost
  would result in extra calls to sleepq_signal() or sleepq_broadcast().
  
  Differential Revision:	https://reviews.freebsd.org/D2427
  Reviewed by:	benno
  Reported by:	benno (wrap of cv_waiters in the field)
  MFC after:	2 weeks

Modified:
  head/sys/kern/kern_condvar.c
  head/sys/sys/condvar.h

Modified: head/sys/kern/kern_condvar.c
==============================================================================
--- head/sys/kern/kern_condvar.c	Fri May 15 13:36:50 2015	(r282970)
+++ head/sys/kern/kern_condvar.c	Fri May 15 13:50:37 2015	(r282971)
@@ -122,7 +122,7 @@ _cv_wait(struct cv *cvp, struct lock_obj
 
 	sleepq_lock(cvp);
 
-	cvp->cv_waiters++;
+	atomic_add_int(&cvp->cv_waiters, 1);
 	if (lock == &Giant.lock_object)
 		mtx_assert(&Giant, MA_OWNED);
 	DROP_GIANT();
@@ -137,6 +137,7 @@ _cv_wait(struct cv *cvp, struct lock_obj
 			sleepq_lock(cvp);
 	}
 	sleepq_wait(cvp, 0);
+	atomic_subtract_int(&cvp->cv_waiters, 1);
 
 #ifdef KTRACE
 	if (KTRPOINT(td, KTR_CSW))
@@ -184,7 +185,7 @@ _cv_wait_unlock(struct cv *cvp, struct l
 
 	sleepq_lock(cvp);
 
-	cvp->cv_waiters++;
+	atomic_add_int(&cvp->cv_waiters, 1);
 	DROP_GIANT();
 
 	sleepq_add(cvp, lock, cvp->cv_description, SLEEPQ_CONDVAR, 0);
@@ -194,6 +195,7 @@ _cv_wait_unlock(struct cv *cvp, struct l
 	if (class->lc_flags & LC_SLEEPABLE)
 		sleepq_lock(cvp);
 	sleepq_wait(cvp, 0);
+	atomic_subtract_int(&cvp->cv_waiters, 1);
 
 #ifdef KTRACE
 	if (KTRPOINT(td, KTR_CSW))
@@ -240,7 +242,7 @@ _cv_wait_sig(struct cv *cvp, struct lock
 
 	sleepq_lock(cvp);
 
-	cvp->cv_waiters++;
+	atomic_add_int(&cvp->cv_waiters, 1);
 	if (lock == &Giant.lock_object)
 		mtx_assert(&Giant, MA_OWNED);
 	DROP_GIANT();
@@ -256,6 +258,7 @@ _cv_wait_sig(struct cv *cvp, struct lock
 			sleepq_lock(cvp);
 	}
 	rval = sleepq_wait_sig(cvp, 0);
+	atomic_subtract_int(&cvp->cv_waiters, 1);
 
 #ifdef KTRACE
 	if (KTRPOINT(td, KTR_CSW))
@@ -307,7 +310,7 @@ _cv_timedwait_sbt(struct cv *cvp, struct
 
 	sleepq_lock(cvp);
 
-	cvp->cv_waiters++;
+	atomic_add_int(&cvp->cv_waiters, 1);
 	if (lock == &Giant.lock_object)
 		mtx_assert(&Giant, MA_OWNED);
 	DROP_GIANT();
@@ -323,6 +326,7 @@ _cv_timedwait_sbt(struct cv *cvp, struct
 			sleepq_lock(cvp);
 	}
 	rval = sleepq_timedwait(cvp, 0);
+	atomic_subtract_int(&cvp->cv_waiters, 1);
 
 #ifdef KTRACE
 	if (KTRPOINT(td, KTR_CSW))
@@ -376,7 +380,7 @@ _cv_timedwait_sig_sbt(struct cv *cvp, st
 
 	sleepq_lock(cvp);
 
-	cvp->cv_waiters++;
+	atomic_add_int(&cvp->cv_waiters, 1);
 	if (lock == &Giant.lock_object)
 		mtx_assert(&Giant, MA_OWNED);
 	DROP_GIANT();
@@ -393,6 +397,7 @@ _cv_timedwait_sig_sbt(struct cv *cvp, st
 			sleepq_lock(cvp);
 	}
 	rval = sleepq_timedwait_sig(cvp, 0);
+	atomic_subtract_int(&cvp->cv_waiters, 1);
 
 #ifdef KTRACE
 	if (KTRPOINT(td, KTR_CSW))
@@ -421,10 +426,8 @@ cv_signal(struct cv *cvp)
 
 	wakeup_swapper = 0;
 	sleepq_lock(cvp);
-	if (cvp->cv_waiters > 0) {
-		cvp->cv_waiters--;
+	if (cvp->cv_waiters > 0)
 		wakeup_swapper = sleepq_signal(cvp, SLEEPQ_CONDVAR, 0, 0);
-	}
 	sleepq_release(cvp);
 	if (wakeup_swapper)
 		kick_proc0();
@@ -447,10 +450,8 @@ cv_broadcastpri(struct cv *cvp, int pri)
 	if (pri == -1)
 		pri = 0;
 	sleepq_lock(cvp);
-	if (cvp->cv_waiters > 0) {
-		cvp->cv_waiters = 0;
+	if (cvp->cv_waiters > 0)
 		wakeup_swapper = sleepq_broadcast(cvp, SLEEPQ_CONDVAR, pri, 0);
-	}
 	sleepq_release(cvp);
 	if (wakeup_swapper)
 		kick_proc0();

Modified: head/sys/sys/condvar.h
==============================================================================
--- head/sys/sys/condvar.h	Fri May 15 13:36:50 2015	(r282970)
+++ head/sys/sys/condvar.h	Fri May 15 13:50:37 2015	(r282971)
@@ -45,7 +45,7 @@ TAILQ_HEAD(cv_waitq, thread);
  */
 struct cv {
 	const char	*cv_description;
-	int		cv_waiters;
+	volatile int	cv_waiters;
 };
 
 #ifdef _KERNEL

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201505151350.t4FDocQT054144>