From d599e4fa793b304609d32e336c668b70831b6b3b Mon Sep 17 00:00:00 2001 From: Brian Behlendorf Date: Fri, 4 Feb 2011 14:09:08 -0800 Subject: [PATCH] Block in cv_destroy() on all waiters Previously we would ASSERT in cv_destroy() if it was ever called with active waiters. However, I've now seen several instances in OpenSolaris code where they do the following: cv_broadcast(); cv_destroy(); This leaves no time for active waiters to be woken up and scheduled and we trip the ASSERT. This has not been observed to be an issue on OpenSolaris because their cv_destroy() basically does nothing. They still do run the risk of the memory being free'd after the cv_destroy() and hitting a bad paging request. But in practice this race is so small and unlikely it either doesn't happen, or is so unlikely when it does happen the root cause has not yet been identified. Rather than risk the same issue in our code this change updates cv_destroy() to block until all waiters have been woken and scheduled. This may take some time because each waiter must acquire the mutex. This change may have an impact on performance for frequently created and destroyed condition variables. That however is a price worth paying it avoid crashing your system. If performance issues are observed they can be addressed by the caller. --- include/sys/condvar.h | 1 + module/spl/spl-condvar.c | 26 +++++++++++++++++++++++--- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/include/sys/condvar.h b/include/sys/condvar.h index 7566dae..ca9dd0f 100644 --- a/include/sys/condvar.h +++ b/include/sys/condvar.h @@ -42,6 +42,7 @@ typedef struct { char *cv_name; int cv_name_size; wait_queue_head_t cv_event; + wait_queue_head_t cv_destroy; atomic_t cv_waiters; kmutex_t *cv_mutex; } kcondvar_t; diff --git a/module/spl/spl-condvar.c b/module/spl/spl-condvar.c index 421f880..2730435 100644 --- a/module/spl/spl-condvar.c +++ b/module/spl/spl-condvar.c @@ -46,6 +46,7 @@ __cv_init(kcondvar_t *cvp, char *name, kcv_type_t type, void *arg) cvp->cv_magic = CV_MAGIC; init_waitqueue_head(&cvp->cv_event); + init_waitqueue_head(&cvp->cv_destroy); atomic_set(&cvp->cv_waiters, 0); cvp->cv_mutex = NULL; cvp->cv_name = NULL; @@ -65,12 +66,27 @@ __cv_init(kcondvar_t *cvp, char *name, kcv_type_t type, void *arg) } EXPORT_SYMBOL(__cv_init); +static int +cv_destroy_wakeup(kcondvar_t *cvp) +{ + if ((waitqueue_active(&cvp->cv_event)) || + (atomic_read(&cvp->cv_waiters) > 0)) + return 0; + + return 1; +} + void __cv_destroy(kcondvar_t *cvp) { SENTRY; ASSERT(cvp); ASSERT(cvp->cv_magic == CV_MAGIC); + + /* Block until all waiters have woken */ + while (cv_destroy_wakeup(cvp) == 0) + wait_event_timeout(cvp->cv_destroy, cv_destroy_wakeup(cvp), 1); + ASSERT(cvp->cv_mutex == NULL); ASSERT(atomic_read(&cvp->cv_waiters) == 0); ASSERT(!waitqueue_active(&cvp->cv_event)); @@ -78,7 +94,6 @@ __cv_destroy(kcondvar_t *cvp) if (cvp->cv_name) kmem_free(cvp->cv_name, cvp->cv_name_size); - ASSERT3P(memset(cvp, CV_POISON, sizeof(*cvp)), ==, cvp); SEXIT; } EXPORT_SYMBOL(__cv_destroy); @@ -111,10 +126,13 @@ cv_wait_common(kcondvar_t *cvp, kmutex_t *mp, int state) mutex_enter(mp); /* No more waiters a different mutex could be used */ - if (atomic_dec_and_test(&cvp->cv_waiters)) + if (atomic_dec_and_test(&cvp->cv_waiters)) { cvp->cv_mutex = NULL; + wake_up(&cvp->cv_destroy); + } finish_wait(&cvp->cv_event, &wait); + SEXIT; } @@ -170,8 +188,10 @@ __cv_timedwait_common(kcondvar_t *cvp, kmutex_t *mp, mutex_enter(mp); /* No more waiters a different mutex could be used */ - if (atomic_dec_and_test(&cvp->cv_waiters)) + if (atomic_dec_and_test(&cvp->cv_waiters)) { cvp->cv_mutex = NULL; + wake_up(&cvp->cv_destroy); + } finish_wait(&cvp->cv_event, &wait); -- 2.40.0