d/freebsd-nq
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Treat mutex->owner as volatile

Browse Source 
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().
This commit is contained in:
Brian Behlendorf 2010-06-28 12:48:20 -07:00
parent 616df2dd8b
commit ede0bdffb6
2 changed files with 116 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

38
include/sys/mutex.h
View File

@ -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) \

100
module/splat/splat-mutex.c
View File

@ -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 @@ splat_mutex_test2(struct file *file, void *arg)
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;