Clear cv->cv_mutex when not in use

For debugging purposes the condition varaibles keep track of the
mutex used during a wait.  The idea is to validate that all callers
always use the same mutex.  Unfortunately, we have seen cases where
the caller reuses the condition variable with a different mutex but
in a way which is known to be safe.  My reading of the man pages
suggests you should not do this and always cv_destroy()/cv_init()
a new mutex.  However, there is overhead in doing this and it does
appear to be allowed under Solaris.

To accomidate this behavior cv_wait_common() and __cv_timedwait()
have been modified to clear the associated mutex when the last
waiter is dropped.  This ensures that while the condition variable
is in use the incorrect mutex case is detected.  It also allows the
condition variable to be safely recycled without requiring the
overhead of a cv_destroy()/cv_init() as long as it isn't currently
in use.

Finally, spin lock cv->cv_lock was removed because it is not required.
When the condition variable is used properly the caller will always
be holding the mutex so the spin lock is redundant.  The lock was
originally added because I expected to need to protect more than
just the cv->cv_mutex.  It turns out that was not the case.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

diff --git a/include/sys/condvar.h b/include/sys/condvar.h
index d854026accce423fc8c985a843dce7e988f87ec5..18e5a6fcf08e43d6cb7049dff36174f032bd695e 100644 (file)
--- a/include/sys/condvar.h
+++ b/include/sys/condvar.h
@@ -44,7 +44,6 @@ typedef struct {
        wait_queue_head_t cv_event;
        atomic_t cv_waiters;
        kmutex_t *cv_mutex;
-       spinlock_t cv_lock;
 } kcondvar_t;
 
 typedef enum { CV_DEFAULT=0, CV_DRIVER } kcv_type_t;

diff --git a/module/spl/spl-condvar.c b/module/spl/spl-condvar.c
index 6b4512472c2b25b8db8dbd5656a9a9e8bfd8497e..edf048bb7a619394437c5b7cf63d2239f2850907 100644 (file)
--- a/module/spl/spl-condvar.c
+++ b/module/spl/spl-condvar.c
@@ -46,7 +46,6 @@ __cv_init(kcondvar_t *cvp, char *name, kcv_type_t type, void *arg)
 
        cvp->cv_magic = CV_MAGIC;
        init_waitqueue_head(&cvp->cv_event);
-       spin_lock_init(&cvp->cv_lock);
        atomic_set(&cvp->cv_waiters, 0);
        cvp->cv_mutex = NULL;
        cvp->cv_name = NULL;
@@ -72,15 +71,14 @@ __cv_destroy(kcondvar_t *cvp)
        SENTRY;
        ASSERT(cvp);
        ASSERT(cvp->cv_magic == CV_MAGIC);
-       spin_lock(&cvp->cv_lock);
+       ASSERT(cvp->cv_mutex == NULL);
        ASSERT(atomic_read(&cvp->cv_waiters) == 0);
        ASSERT(!waitqueue_active(&cvp->cv_event));
 
        if (cvp->cv_name)
                kmem_free(cvp->cv_name, cvp->cv_name_size);
 
-       spin_unlock(&cvp->cv_lock);
-       memset(cvp, CV_POISON, sizeof(*cvp));
+       ASSERT3P(memset(cvp, CV_POISON, sizeof(*cvp)), ==, cvp);
        SEXIT;
 }
 EXPORT_SYMBOL(__cv_destroy);
@@ -94,7 +92,6 @@ cv_wait_common(kcondvar_t *cvp, kmutex_t *mp, int state)
        ASSERT(cvp);
         ASSERT(mp);
        ASSERT(cvp->cv_magic == CV_MAGIC);
-       spin_lock(&cvp->cv_lock);
        ASSERT(mutex_owned(mp));
 
        if (cvp->cv_mutex == NULL)
@@ -102,7 +99,6 @@ cv_wait_common(kcondvar_t *cvp, kmutex_t *mp, int state)
 
        /* Ensure the same mutex is used by all callers */
        ASSERT(cvp->cv_mutex == mp);
-       spin_unlock(&cvp->cv_lock);
 
        prepare_to_wait_exclusive(&cvp->cv_event, &wait, state);
        atomic_inc(&cvp->cv_waiters);
@@ -114,7 +110,10 @@ cv_wait_common(kcondvar_t *cvp, kmutex_t *mp, int state)
        schedule();
        mutex_enter(mp);
 
-       atomic_dec(&cvp->cv_waiters);
+       /* No more waiters a different mutex could be used */
+       if (atomic_dec_and_test(&cvp->cv_waiters))
+               cvp->cv_mutex = NULL;
+
        finish_wait(&cvp->cv_event, &wait);
        SEXIT;
 }
@@ -146,7 +145,6 @@ __cv_timedwait(kcondvar_t *cvp, kmutex_t *mp, clock_t expire_time)
        ASSERT(cvp);
         ASSERT(mp);
        ASSERT(cvp->cv_magic == CV_MAGIC);
-       spin_lock(&cvp->cv_lock);
        ASSERT(mutex_owned(mp));
 
        if (cvp->cv_mutex == NULL)
@@ -154,7 +152,6 @@ __cv_timedwait(kcondvar_t *cvp, kmutex_t *mp, clock_t expire_time)
 
        /* Ensure the same mutex is used by all callers */
        ASSERT(cvp->cv_mutex == mp);
-       spin_unlock(&cvp->cv_lock);
 
        /* XXX - Does not handle jiffie wrap properly */
        time_left = expire_time - jiffies;
@@ -172,7 +169,10 @@ __cv_timedwait(kcondvar_t *cvp, kmutex_t *mp, clock_t expire_time)
        time_left = schedule_timeout(time_left);
        mutex_enter(mp);
 
-       atomic_dec(&cvp->cv_waiters);
+       /* No more waiters a different mutex could be used */
+       if (atomic_dec_and_test(&cvp->cv_waiters))
+               cvp->cv_mutex = NULL;
+
        finish_wait(&cvp->cv_event, &wait);
 
        SRETURN(time_left > 0 ? time_left : -1);