From ede0bdffb6d36915ad610b0bdf7d790f858f448c Mon Sep 17 00:00:00 2001 From: Brian Behlendorf Date: Mon, 28 Jun 2010 12:48:20 -0700 Subject: [PATCH] Treat mutex->owner as volatile When HAVE_MUTEX_OWNER is defined and we are directly accessing mutex->owner treat is as volative with the ACCESS_ONCE() helper. Without this you may get a stale cached value when accessing it from different cpus. This can result in incorrect behavior from mutex_owned() and mutex_owner(). This is not a problem for the !HAVE_MUTEX_OWNER case because in this case all the accesses are covered by a spin lock which similarly gaurentees we will not be accessing stale data. Secondly, check CONFIG_SMP before allowing access to mutex->owner. I see that for non-SMP setups the kernel does not track the owner so we cannot rely on it. Thirdly, check CONFIG_MUTEX_DEBUG when this is defined and the HAVE_MUTEX_OWNER is defined surprisingly the mutex->owner will not be cleared on mutex_exit(). When this is the case the SPL needs to make sure to do it to ensure MUTEX_HELD() behaves as expected or you will certainly assert in mutex_destroy(). Finally, improve the mutex regression tests. For mutex_owned() we now minimally check that it behaves correctly when checked from the owner thread or the non-owner thread. This subtle behaviour has bit me before and I'd like to catch it early next time if it reappears. As for mutex_owned() regression test additonally verify that mutex->owner is always cleared on mutex_exit(). --- include/sys/mutex.h | 38 ++++++++++---- module/splat/splat-mutex.c | 100 ++++++++++++++++++++++++++++++++----- 2 files changed, 116 insertions(+), 22 deletions(-) diff --git a/include/sys/mutex.h b/include/sys/mutex.h index b36c7e256..d33694766 100644 --- a/include/sys/mutex.h +++ b/include/sys/mutex.h @@ -34,20 +34,29 @@ typedef enum { MUTEX_ADAPTIVE = 2 } kmutex_type_t; -#ifdef HAVE_MUTEX_OWNER +#if defined(HAVE_MUTEX_OWNER) && defined(CONFIG_SMP) typedef struct mutex kmutex_t; static inline kthread_t * mutex_owner(kmutex_t *mp) { - if (mp->owner) - return (mp->owner)->task; + struct thread_info *owner; + + owner = ACCESS_ONCE(mp->owner); + if (owner) + return owner->task; return NULL; } -#define mutex_owned(mp) (mutex_owner(mp) == current) -#define MUTEX_HELD(mp) mutex_owned(mp) + +static inline int +mutex_owned(kmutex_t *mp) +{ + return (ACCESS_ONCE(mp->owner) == current_thread_info()); +} + +#define MUTEX_HELD(mp) mutex_owned(mp) #undef mutex_init #define mutex_init(mp, name, type, ibc) \ ({ \ @@ -60,13 +69,22 @@ mutex_owner(kmutex_t *mp) #undef mutex_destroy #define mutex_destroy(mp) \ ({ \ - VERIFY(!MUTEX_HELD(mp)); \ + VERIFY3P(mutex_owner(mp), ==, NULL); \ }) -#define mutex_tryenter(mp) mutex_trylock(mp) -#define mutex_enter(mp) mutex_lock(mp) -#define mutex_exit(mp) mutex_unlock(mp) +#define mutex_tryenter(mp) mutex_trylock(mp) +#define mutex_enter(mp) mutex_lock(mp) +/* mutex->owner is not cleared when CONFIG_DEBUG_MUTEXES is set */ +#ifdef CONFIG_DEBUG_MUTEXES +# define mutex_exit(mp) \ +({ \ + (mp)->owner = NULL; \ + mutex_unlock(mp); \ +}) +#else +# define mutex_exit(mp) mutex_unlock(mp) +#endif /* CONFIG_DEBUG_MUTEXES */ #ifdef HAVE_GPL_ONLY_SYMBOLS # define mutex_enter_nested(mp, sc) mutex_lock_nested(mp, sc) @@ -151,7 +169,7 @@ mutex_owner(kmutex_t *mp) #undef mutex_destroy #define mutex_destroy(mp) \ ({ \ - VERIFY(!MUTEX_HELD(mp)); \ + VERIFY3P(mutex_owner(mp), ==, NULL); \ }) #define mutex_tryenter(mp) \ diff --git a/module/splat/splat-mutex.c b/module/splat/splat-mutex.c index 96ed27297..d134e49ce 100644 --- a/module/splat/splat-mutex.c +++ b/module/splat/splat-mutex.c @@ -55,6 +55,7 @@ typedef struct mutex_priv { struct file *mp_file; kmutex_t mp_mtx; int mp_rc; + int mp_rc2; } mutex_priv_t; static void @@ -240,37 +241,96 @@ out: return rc; } +static void +splat_mutex_owned(void *priv) +{ + mutex_priv_t *mp = (mutex_priv_t *)priv; + + ASSERT(mp->mp_magic == SPLAT_MUTEX_TEST_MAGIC); + mp->mp_rc = mutex_owned(&mp->mp_mtx); + mp->mp_rc2 = MUTEX_HELD(&mp->mp_mtx); +} + static int splat_mutex_test3(struct file *file, void *arg) { - kmutex_t mtx; + mutex_priv_t mp; + taskq_t *tq; int rc = 0; - mutex_init(&mtx, SPLAT_MUTEX_TEST_NAME, MUTEX_DEFAULT, NULL); - mutex_enter(&mtx); + mp.mp_magic = SPLAT_MUTEX_TEST_MAGIC; + mp.mp_file = file; + mutex_init(&mp.mp_mtx, SPLAT_MUTEX_TEST_NAME, MUTEX_DEFAULT, NULL); + + if ((tq = taskq_create(SPLAT_MUTEX_TEST_NAME, 1, maxclsyspri, + 50, INT_MAX, TASKQ_PREPOPULATE)) == NULL) { + splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Taskq '%s' " + "create failed\n", SPLAT_MUTEX_TEST3_NAME); + return -EINVAL; + } + + mutex_enter(&mp.mp_mtx); /* Mutex should be owned by current */ - if (!mutex_owned(&mtx)) { + if (!mutex_owned(&mp.mp_mtx)) { splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Unowned mutex " - "should be owned by pid %d\n", current->pid); + "should be owned by pid %d\n", current->pid); + rc = -EINVAL; + goto out_exit; + } + + if (taskq_dispatch(tq, splat_mutex_owned, &mp, TQ_SLEEP) == 0) { + splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Failed to " + "dispatch function '%s' to taskq\n", + sym2str(splat_mutex_owned)); + rc = -EINVAL; + goto out_exit; + } + taskq_wait(tq); + + /* Mutex should not be owned which checked from a different thread */ + if (mp.mp_rc || mp.mp_rc2) { + splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Mutex owned by " + "pid %d not by taskq\n", current->pid); + rc = -EINVAL; + goto out_exit; + } + + mutex_exit(&mp.mp_mtx); + + /* Mutex should not be owned by current */ + if (mutex_owned(&mp.mp_mtx)) { + splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Mutex owned by " + "pid %d it should be unowned\b", current->pid); rc = -EINVAL; goto out; } - mutex_exit(&mtx); + if (taskq_dispatch(tq, splat_mutex_owned, &mp, TQ_SLEEP) == 0) { + splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Failed to " + "dispatch function '%s' to taskq\n", + sym2str(splat_mutex_owned)); + rc = -EINVAL; + goto out; + } + taskq_wait(tq); - /* Mutex should not be owned by any task */ - if (mutex_owned(&mtx)) { + /* Mutex should be owned by no one */ + if (mp.mp_rc || mp.mp_rc2) { splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Mutex owned by " - "pid %d should be unowned\b", current->pid); + "no one, %d/%d disagrees\n", mp.mp_rc, mp.mp_rc2); rc = -EINVAL; goto out; } splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "%s", "Correct mutex_owned() behavior\n"); + goto out; +out_exit: + mutex_exit(&mp.mp_mtx); out: - mutex_destroy(&mtx); + mutex_destroy(&mp.mp_mtx); + taskq_destroy(tq); return rc; } @@ -283,12 +343,28 @@ splat_mutex_test4(struct file *file, void *arg) int rc = 0; mutex_init(&mtx, SPLAT_MUTEX_TEST_NAME, MUTEX_DEFAULT, NULL); + + /* + * Verify mutex owner is cleared after being dropped. Depending + * on how you build your kernel this behavior changes, ensure the + * SPL mutex implementation is properly detecting this. + */ + mutex_enter(&mtx); + msleep(100); + mutex_exit(&mtx); + if (MUTEX_HELD(&mtx)) { + splat_vprint(file, SPLAT_MUTEX_TEST4_NAME, "Mutex should " + "not be held, bit is by %p\n", mutex_owner(&mtx)); + rc = -EINVAL; + goto out; + } + mutex_enter(&mtx); /* Mutex should be owned by current */ owner = mutex_owner(&mtx); if (current != owner) { - splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Mutex should " + splat_vprint(file, SPLAT_MUTEX_TEST4_NAME, "Mutex should " "be owned by pid %d but is owned by pid %d\n", current->pid, owner ? owner->pid : -1); rc = -EINVAL; @@ -300,7 +376,7 @@ splat_mutex_test4(struct file *file, void *arg) /* Mutex should not be owned by any task */ owner = mutex_owner(&mtx); if (owner) { - splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Mutex should not " + splat_vprint(file, SPLAT_MUTEX_TEST4_NAME, "Mutex should not " "be owned but is owned by pid %d\n", owner->pid); rc = -EINVAL; goto out; -- 2.39.5