Fix mutex assertion
authorSimon Wilkinson <sxw@your-file-system.com>
Sun, 21 Oct 2012 19:07:44 +0000 (20:07 +0100)
committerDerrick Brashear <shadow@your-file-system.com>
Tue, 30 Oct 2012 11:59:07 +0000 (04:59 -0700)
RX mutexes have two mechanisms for asserting ownership of a mutex:
MUTEX_ISMINE, which returns true if the caller is the owner of the
mutex in question, and osirx_AssertMutex which fires an assertion if
the calling thread doesn't own a specified mutex.

Neither of these work with pthread mutexes on all platforms, and the
kernel support for MUTEX_ISMINE is dubious in places. Because in some
implementations, MUTEX_ISMINE tries to lock the mutex in question, a
failing call to MUTEX_ISMINE can lead to the process holding an
additional, unexpected, lock.

This patch reworks all of this, and uses the new opr mutex framework
so that we can do mutex assertions in userspace, too. We remove the
unsafe MUTEX_ISMINE macro, and replace it with MUTEX_ASSERT which
simply asserts if the specified mutex is not held by the current
thread. osirx_AssertMutex is removed as it is now redundant.
MUTEX_ASSERT will always work in kernel code.

For userspace, we provide opr_mutex_assert, which is implemented using
POSIX error checking mutexes. As error checking mutexes have a runtime
overhead, this is only available when configured with
--enable-debug-locks, the rest of the time calls to opr_mutex_assert are
no-ops.

Change-Id: I285ee2b558389fa3d63b786e4bc4420fa2126c80
Reviewed-on: http://gerrit.openafs.org/8282
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Derrick Brashear <shadow@your-file-system.com>

19 files changed:
acinclude.m4
src/opr/lockstub.h
src/opr/opr_lock.h
src/rx/AIX/rx_kmutex.h
src/rx/DARWIN/rx_kmutex.h
src/rx/FBSD/rx_kmutex.h
src/rx/HPUX/rx_kmutex.h
src/rx/IRIX/rx_kmutex.h
src/rx/LINUX/rx_kmutex.h
src/rx/LINUX24/rx_kmutex.h
src/rx/NBSD/rx_kmutex.h
src/rx/OBSD/rx_kmutex.h
src/rx/SOLARIS/rx_kmutex.h
src/rx/UKERNEL/rx_kmutex.h
src/rx/rx.c
src/rx/rx_lwp.h
src/rx/rx_prototypes.h
src/rx/rx_pthread.h
src/vol/vutil.c

index 05a933f..a3362f0 100644 (file)
@@ -192,6 +192,11 @@ AC_ARG_ENABLE([checking],
         to disabled)])],
     [enable_checking="$enableval"],
     [enable_checking="no"])
+AC_ARG_ENABLE([debug-locks],
+    [AS_HELP_STRING([--enable-debug-locks],
+       [turn on lock debugging assertions (defaults to disabled)])],
+    [enable_debug_locks="$enableval"],
+    [enable_debug_locks="no"])
 AC_ARG_ENABLE([debug-kernel],
     [AS_HELP_STRING([--enable-debug-kernel],
         [enable compilation of the kernel module with debugging information
@@ -1412,6 +1417,10 @@ AS_IF([test "x$enable_gtx" != "xno"],
        
 OPENAFS_TEST_PACKAGE(libintl,[#include <libintl.h>],[-lintl],,,INTL)
 
+if test "$enable_debug_locks" = yes; then
+       AC_DEFINE(OPR_DEBUG_LOCKS, 1, [turn on lock debugging in opr])
+fi
+
 dnl Don't build PAM on IRIX; the interface doesn't work for us.
 if test "$ac_cv_header_security_pam_modules_h" = yes -a "$enable_pam" = yes; then
         case $AFS_SYSNAME in
index 2db610b..db375b2 100644 (file)
@@ -37,6 +37,7 @@
 typedef int opr_cv_t;
 
 # define opr_mutex_init(mutex)
+# define opr_mutex_assert(mutex)
 # define opr_mutex_destroy(mutex)
 # define opr_mutex_enter(mutex)
 # define opr_mutex_exit(mutex)
index 0d524e4..6051c5d 100644 (file)
 
 #include <pthread.h>
 
+/* Mutexes */
+
 typedef pthread_mutex_t opr_mutex_t;
 
-# define opr_mutex_init(mutex) \
+# ifdef OPR_DEBUG_LOCKS
+static_inline void
+opr_mutex_init(opr_mutex_t *mutex)
+{
+    pthread_mutexattr_t attr;
+
+    pthread_mutexattr_init(&attr);
+    pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_ERRORCHECK);
+
+    opr_Verify(pthread_mutex_init(mutex, &attr) == 0);
+    pthread_mutexattr_destroy(&attr);
+}
+
+#  define opr_mutex_assert(mutex) \
+    opr_Verify(pthread_mutex_lock(mutex) == EDEADLK)
+
+# else
+
+#  define opr_mutex_init(mutex) \
     opr_Verify(pthread_mutex_init(mutex, NULL) == 0)
 
+#  define opr_mutex_assert(mutex)
+
+# endif
+
 # define opr_mutex_destroy(mutex) \
     opr_Verify(pthread_mutex_destroy(mutex) == 0)
 
index 381922c..4e624c6 100644 (file)
@@ -132,11 +132,7 @@ typedef tid_t afs_kcondvar_t;
 
 #define        MUTEX_DEFAULT   0
 
-#undef MUTEX_ISMINE
-#define MUTEX_ISMINE(a)        (lock_mine((void *)(a)))
-
-#undef osirx_AssertMine
-extern void osirx_AssertMine(afs_kmutex_t * lockaddr, char *msg);
+#define MUTEX_ASSERT(a)        osi_Assert(lock_mine((void *)(a)))
 
 #endif /* AFS_AIX41_ENV */
 
index 31e97c8..0fe6aa8 100644 (file)
@@ -170,8 +170,7 @@ extern lck_grp_t * openafs_lck_grp;
         lck_mtx_unlock((a)->meta); \
     } while(0)
 
-#undef MUTEX_ISMINE
-#define MUTEX_ISMINE(a) (((afs_kmutex_t *)(a))->owner == current_thread())
+#define MUTEX_ASSERT(a) osi_Assert(((afs_kmutex_t *)(a))->owner == current_thread())
 #else
 typedef struct {
     struct lock__bsd__ lock;
@@ -208,11 +207,7 @@ typedef int afs_kcondvar_t;
        lockmgr(&(a)->lock, LK_RELEASE, 0, current_proc()); \
     } while(0);
 
-#undef MUTEX_ISMINE
-#define MUTEX_ISMINE(a) (((afs_kmutex_t *)(a))->owner == current_thread())
+#define MUTEX_ASSERT(a) osi_Assert(((afs_kmutex_t *)(a))->owner == current_thread())
 #endif
 
-#undef osirx_AssertMine
-extern void osirx_AssertMine(afs_kmutex_t * lockaddr, char *msg);
-
 #endif /* _RX_KMUTEX_H_ */
index 2b98343..a694a52 100644 (file)
@@ -56,8 +56,7 @@ typedef struct {
        (a)->owner = 0; \
     } while(0);
 
-#undef MUTEX_ISMINE
-#define MUTEX_ISMINE(a) (((afs_kmutex_t *)(a))->owner == curproc)
+#define MUTEX_ASSERT(a) osi_Assert(((afs_kmutex_t *)(a))->owner == curproc)
 
 #elif defined(AFS_FBSD70_ENV) /* dunno about 6.x */
 
@@ -94,9 +93,8 @@ typedef struct mtx afs_kmutex_t;
        mtx_unlock((a)); \
     } while(0);
 
-#undef MUTEX_ISMINE
-#define MUTEX_ISMINE(a)                                \
-    ( mtx_owned((a)) )
+#define MUTEX_ASSERT(a)                                \
+    osi_Assert(mtx_owned((a)))
 
 #else
 
@@ -132,15 +130,9 @@ typedef struct {
        lockmgr(&(a)->lock, LK_RELEASE, 0, curthread); \
     } while(0);
 
-#undef MUTEX_ISMINE
-#define MUTEX_ISMINE(a) (((afs_kmutex_t *)(a))->owner == curthread)
+#define MUTEX_ASSERT(a) osi_Assert(((afs_kmutex_t *)(a))->owner == curthread)
 #endif
 
-
-#undef osirx_AssertMine
-extern void osirx_AssertMine(afs_kmutex_t * lockaddr, char *msg);
-
-
 /*
  * Condition variables
  *
index 3e98f0f..89ebaf0 100644 (file)
@@ -132,8 +132,6 @@ typedef caddr_t afs_kcondvar_t;
 #ifdef AFS_HPUX102_ENV
 
 #if defined(AFS_HPUX110_ENV)
-#undef osirx_AssertMine
-extern void osirx_AssertMine(afs_kmutex_t * lockaddr, char *msg);
 
 #define AFS_RX_ORDER 30
 
@@ -155,18 +153,16 @@ extern void osirx_AssertMine(afs_kmutex_t * lockaddr, char *msg);
     ((b_owns_sema(a)) ? b_vsema(a) : (osi_Panic("mutex not held"), 0))
 #endif
 
-#undef MUTEX_ISMINE
-#define MUTEX_ISMINE(a) b_owns_sema(a)
+#define MUTEX_ASSERT(a) osi_Assert(b_owns_sema(a))
 
 #else /* AFS_HPUX110_ENV */
 
-#define osirx_AssertMine(addr, msg)
-
 #define MUTEX_DESTROY(a)
 #define MUTEX_ENTER(a)
 #define MUTEX_TRYENTER(a) 1
 #define MUTEX_EXIT(a)
 #define MUTEX_INIT(a,b,c,d)
+#define MUTEX_ASSERT(a)
 
 #endif /* else AFS_HPUX110_ENV */
 #endif /* AFS_HPUX102_ENV */
index 7c48725..5767a55 100644 (file)
@@ -42,14 +42,11 @@ typedef kcondvar_t afs_kcondvar_t;
 #define MUTEX_INIT(a,b,c,d)  mutex_init(a,b,c,d)
 #endif
 #define MUTEX_DESTROY(a) mutex_destroy(a)
-#undef MUTEX_ISMINE
-#define MUTEX_ISMINE(a)        1
+#define MUTEX_ASSERT(a)
 #define CV_INIT(cv, a,b,c)     cv_init(cv, a, b, c)
 #define CV_SIGNAL(_cv)         cv_signal(_cv)
 #define CV_BROADCAST(_cv)      cv_broadcast(_cv)
 #define CV_DESTROY(_cv)                cv_destroy(_cv)
-#undef osirx_AssertMine
-extern void osirx_AssertMine(afs_kmutex_t * lockaddr, char *msg);
 #ifdef AFS_SGI64_ENV
 /* Add PLTWAIT for afsd's to wait so we don't rack up the load average. */
 #ifdef AFS_SGI65_ENV
@@ -144,13 +141,11 @@ extern void osirx_AssertMine(afs_kmutex_t * lockaddr, char *msg);
 #else /* MP */
 #define MUTEX_INIT(m, nm, type , a)
 #define MUTEX_DESTROY(a)
-#define MUTEX_ISMINE(a)        1
+#define MUTEX_ASSERT(a)
 #define MUTEX_ENTER(a)
 #define MUTEX_TRYENTER(a) 1
 #define MUTEX_EXIT(a)
 
-#define osirx_AssertMine(addr, msg)
-
 #define CV_INIT(cv, a,b,c)
 #define CV_SIGNAL(_cv)
 #define CV_BROADCAST(_cv)
index d9f38a9..eb396e3 100644 (file)
@@ -52,10 +52,10 @@ typedef struct afs_kcondvar {
     wait_queue_head_t waitq;
 } afs_kcondvar_t;
 
-static inline int
-MUTEX_ISMINE(afs_kmutex_t * l)
+static inline void
+MUTEX_ASSERT(afs_kmutex_t * l)
 {
-    return l->owner == current->pid;
+    osi_Assert(l->owner == current->pid);
 }
 
 #define MUTEX_INIT(a,b,c,d)    afs_mutex_init(a)
index 18e65f9..c3590b9 100644 (file)
@@ -56,10 +56,10 @@ typedef struct afs_kcondvar {
 #endif
 } afs_kcondvar_t;
 
-static inline int
-MUTEX_ISMINE(afs_kmutex_t * l)
+static inline void
+MUTEX_ASSERT(afs_kmutex_t * l)
 {
-    return l->owner == current->pid;
+    osi_Assert(l->owner == current->pid);
 }
 
 #define MUTEX_INIT(a,b,c,d)    afs_mutex_init(a)
index a3240ff..80f588d 100644 (file)
@@ -43,7 +43,7 @@ typedef kmutex_t afs_kmutex_t;
 #define MUTEX_ENTER(a) mutex_enter((a));
 #define MUTEX_TRYENTER(a) mutex_tryenter((a))
 #define MUTEX_EXIT(a) mutex_exit((a))
-#define MUTEX_ISMINE(a) mutex_owned((a))
+#define MUTEX_ASSERT(a) osi_Assert(mutex_owned((a)))
 
 typedef kcondvar_t afs_kcondvar_t;
 int afs_cv_wait(afs_kcondvar_t *, afs_kmutex_t *, int);
@@ -149,8 +149,8 @@ typedef struct {
     } while(0);
 #endif  /* LOCKDEBUG */
 
-#define MUTEX_ISMINE(a) \
-    (lockstatus(a) == LK_EXCLUSIVE)
+#define MUTEX_ASSERT(a) \
+    osi_Assert((lockstatus(a) == LK_EXCLUSIVE))
 #define MUTEX_LOCKED(a) \
     (lockstatus(a) == LK_EXCLUSIVE)
 
index 7034613..a9e57bc 100644 (file)
@@ -64,6 +64,6 @@ typedef struct {
        osi_Assert((a)->owner == curproc); \
        (a)->owner = 0; \
     } while(0);
-#define MUTEX_ISMINE(a) (((afs_kmutex_t *)(a))->owner == curproc)
+#define MUTEX_ASSERT(a) osi_Assert(((afs_kmutex_t *)(a))->owner == curproc)
 
 #endif /* _RX_KMUTEX_H_ */
index 3e74ae6..c570025 100644 (file)
 typedef kmutex_t afs_kmutex_t;
 typedef kcondvar_t afs_kcondvar_t;
 
-#undef osirx_AssertMine
-extern void osirx_AssertMine(afs_kmutex_t * lockaddr, char *msg);
-
 #define MUTEX_DESTROY(a)       mutex_destroy(a)
 #define MUTEX_INIT(a,b,c,d)    mutex_init(a, b, c, d)
-#define MUTEX_ISMINE(a)                mutex_owned((afs_kmutex_t *)(a))
+#define MUTEX_ASSERT(a)                osi_Assert(mutex_owned((afs_kmutex_t *)(a)))
 #define CV_INIT(a,b,c,d)       cv_init(a, b, c, d)
 #define CV_DESTROY(a)          cv_destroy(a)
 #define CV_SIGNAL(a)           cv_signal(a)
index 95668f6..0602de6 100644 (file)
@@ -23,9 +23,9 @@
 #define MUTEX_INIT(A,B,C,D)    usr_mutex_init(A)
 #define MUTEX_ENTER(A)         usr_mutex_lock(A)
 #define MUTEX_TRYENTER(A)      usr_mutex_trylock(A)
-#define MUTEX_ISMINE(A)                (1)
 #define MUTEX_EXIT(A)          usr_mutex_unlock(A)
 #define MUTEX_DESTROY(A)       usr_mutex_destroy(A)
+#define MUTEX_ASSERT(A)
 #define CV_INIT(A,B,C,D)       usr_cond_init(A)
 #define CV_TIMEDWAIT(A,B,C)    usr_cond_timedwait(A,B,C)
 #define CV_SIGNAL(A)           usr_cond_signal(A)
                                    } \
                                }
 
-#ifndef UKERNEL
-extern void osirx_AssertMine(afs_kmutex_t * lockaddr, char *msg);
-#endif
-
 #define SPLVAR
 #define NETPRI
 #define USERPRI
index bb3a852..45861e5 100644 (file)
@@ -1414,7 +1414,7 @@ rxi_WaitforTQBusy(struct rx_call *call) {
     while (!call->error && (call->flags & RX_CALL_TQ_BUSY)) {
        call->flags |= RX_CALL_TQ_WAIT;
        call->tqWaiters++;
-       osirx_AssertMine(&call->lock, "rxi_WaitforTQ lock");
+       MUTEX_ASSERT(&call->lock);
        CV_WAIT(&call->cv_tq, &call->lock);
        call->tqWaiters--;
        if (call->tqWaiters == 0) {
@@ -1431,7 +1431,7 @@ rxi_WakeUpTransmitQueue(struct rx_call *call)
        dpf(("call %"AFS_PTR_FMT" has %d waiters and flags %d\n",
             call, call->tqWaiters, call->flags));
 #ifdef RX_ENABLE_LOCKS
-       osirx_AssertMine(&call->lock, "rxi_Start start");
+       MUTEX_ASSERT(&call->lock);
        CV_BROADCAST(&call->cv_tq);
 #else /* RX_ENABLE_LOCKS */
        osi_rxWakeup(&call->tq);
@@ -3544,7 +3544,7 @@ rxi_ReceivePacket(struct rx_packet *np, osi_socket socket,
        return np;
     }
 
-    osirx_AssertMine(&call->lock, "rxi_ReceivePacket middle");
+    MUTEX_ASSERT(&call->lock);
     /* Set remote user defined status from packet */
     call->remoteStatus = np->header.userStatus;
 
@@ -5220,9 +5220,7 @@ rx_InterruptCall(struct rx_call *call, afs_int32 error)
 void
 rxi_CallError(struct rx_call *call, afs_int32 error)
 {
-#ifdef DEBUG
-    osirx_AssertMine(&call->lock, "rxi_CallError");
-#endif
+    MUTEX_ASSERT(&call->lock);
     dpf(("rxi_CallError call %"AFS_PTR_FMT" error %d call->error %d\n", call, error, call->error));
     if (call->error)
        error = call->error;
@@ -5250,9 +5248,8 @@ rxi_ResetCall(struct rx_call *call, int newcall)
     int flags;
     struct rx_peer *peer;
     struct rx_packet *packet;
-#ifdef DEBUG
-    osirx_AssertMine(&call->lock, "rxi_ResetCall");
-#endif
+
+    MUTEX_ASSERT(&call->lock);
     dpf(("rxi_ResetCall(call %"AFS_PTR_FMT", newcall %d)\n", call, newcall));
 
     /* Notify anyone who is waiting for asynchronous packet arrival */
@@ -7972,15 +7969,6 @@ shutdown_rx(void)
     UNLOCK_RX_INIT;
 }
 
-#ifdef RX_ENABLE_LOCKS
-void
-osirx_AssertMine(afs_kmutex_t * lockaddr, char *msg)
-{
-    if (!MUTEX_ISMINE(lockaddr))
-       osi_Panic("Lock not held: %s", msg);
-}
-#endif /* RX_ENABLE_LOCKS */
-
 #ifndef KERNEL
 
 /*
index 3bfe8c9..6ce407f 100644 (file)
@@ -29,14 +29,13 @@ typedef int afs_kmutex_t;
 #define MUTEX_TRYENTER(a) 1
 #define MUTEX_EXIT(a)
 #define MUTEX_INIT(a,b,c,d)
-#define MUTEX_ISMINE(a)
+#define MUTEX_ASSERT(a)
 #define CV_INIT(a,b,c,d)
 #define CV_DESTROY(a)
 #define CV_WAIT(cv, l)
 #define CV_SIGNAL(cv)
 #define CV_BROADCAST(cv)
 #define CV_TIMEDWAIT(cv, l, t)
-#define osirx_AssertMine(a, b)
 
 #endif /* KERNEL */
 
index eebee69..f83f181 100644 (file)
@@ -122,9 +122,6 @@ extern afs_int32 rx_GetServerPeers(osi_socket socket, afs_uint32 remoteAddr,
 extern afs_int32 rx_GetLocalPeers(afs_uint32 peerHost, afs_uint16 peerPort,
                                      struct rx_debugPeer * peerStats);
 extern void shutdown_rx(void);
-#ifndef osirx_AssertMine
-extern void osirx_AssertMine(afs_kmutex_t * lockaddr, char *msg);
-#endif
 #ifndef KERNEL
 extern int rx_KeyCreate(rx_destructor_t rtn);
 #endif
index 1eec91d..4b38ef8 100644 (file)
@@ -8,7 +8,7 @@
  */
 
 /* rx_pthread.h defines the lock and cv primitives required for a thread
- * safe user mode rx. The current implemenation is only tested on Solaris.
+ * safe user mode rx.
  */
 
 #ifndef RX_PTHREAD_H
 
 typedef pthread_mutex_t afs_kmutex_t;
 typedef pthread_cond_t afs_kcondvar_t;
-#ifdef RX_ENABLE_LOCKS
-#define MUTEX_ISMINE(l) (pthread_mutex_trylock(l) == EDEADLK)
-#else
-#define MUTEX_ISMINE(l) (1)
-#endif
 
 #define pthread_yield() Sleep(0)
 
@@ -54,27 +49,14 @@ typedef pthread_cond_t afs_kcondvar_t;
 #if !defined(pthread_yield) && (_XOPEN_SOURCE + 0) >= 500
 #define pthread_yield() sched_yield()
 #endif
-
-
-#ifndef MUTEX_ISMINE
-/* Only used for debugging. */
-#ifdef AFS_SUN5_ENV
-/* synch.h says mutex_t and pthread_mutex_t are always the same */
-#include <synch.h>
-#define MUTEX_ISMINE(l) MUTEX_HELD((mutex_t *) l)
-#else /* AFS_SUN5_ENV */
-#define MUTEX_ISMINE(l) (1)
-#endif /* AFS_SUN5_ENV */
-#endif /* !MUTEX_ISMINE */
 #endif /* AFS_NT40_ENV */
 
-extern void osirx_AssertMine(afs_kmutex_t * lockaddr, char *msg);
-
 #define MUTEX_INIT(a, b, c, d) opr_mutex_init(a)
 #define MUTEX_DESTROY(l) opr_mutex_destroy(l)
 #define MUTEX_ENTER(l) opr_mutex_enter(l)
 #define MUTEX_TRYENTER(l) opr_mutex_tryenter(l)
 #define MUTEX_EXIT(l) opr_mutex_exit(l)
+#define MUTEX_ASSERT(l) opr_mutex_assert(l)
 #define CV_INIT(cv, a, b, c) opr_cv_init(cv)
 #define CV_DESTROY(cv) opr_cv_destroy(cv)
 #define CV_WAIT(cv, l) opr_cv_wait(cv, l)
index 9db42a4..5f0e0b6 100644 (file)
@@ -18,6 +18,7 @@
 #include <afs/param.h>
 
 #include <roken.h>
+#include <afs/opr.h>
 
 #ifdef HAVE_SYS_FILE_H
 #include <sys/file.h>
@@ -33,7 +34,6 @@
 # include <opr/lockstub.h>
 #endif
 
-#include <afs/opr.h>
 #include <rx/rx_queue.h>
 #include <rx/xdr.h>
 #include <afs/afsint.h>