From ede0bdffb6d36915ad610b0bdf7d790f858f448c Mon Sep 17 00:00:00 2001 From: Brian Behlendorf Date: Mon, 28 Jun 2010 12:48:20 -0700 Subject: 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(). --- module/splat/splat-mutex.c | 100 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 88 insertions(+), 12 deletions(-) (limited to 'module/splat/splat-mutex.c') 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; -- cgit v1.2.3