On Fri, Jul 26, 2013 at 06:07:02PM -0700, Ethan Jackson wrote:
> This commit adds annotations for thread safety check. And the
> check can be conducted by using -Wthread-safety flag in clang.
I would think that the commit should also explain where to get clang
with this support, probably by adding it to INSTALL. Also, I'd guess
that it would be worth turning it on by default if it is available, by
adding
OVS_ENABLE_OPTION([-Wthread-safety])
to configure.ac.
Oh, I see Alex sent a separate patch for that. Any reason not to fold
it in?
> +#if __has_feature(c_thread_safety_attributes)
> +/* "clang" annotations for thread safety check.
> + *
> + * OVS_LOCKABLE indicates that the struct contains mutex element
> + * which can be locked by ovs_mutex_lock().
> + *
What does the following sentence mean? I do not understand it. Also,
s/sturct/struct/.
> + * MUTEX below can be more than one OVS_LOCKABLE sturcts.
> + *
> + * In a global variable declaration:
> *
Here, s/must be access/may only be accessed/:
> + * OVS_GUARDED_VAR indicates that the variable must be access while
> + * some MUTEX is acquired.
> *
> + * OVS_GUARDED_BY(...) indicates that the variable must be access while
> + * MUTEX is acquired.
> + *
> + *
> + * In a function prototype:
> + *
> + * OVS_ACQ_RDLOCK(...) and OVS_ACQ_WRLOCK(...) indicate that the
> + * function must be called without MUTEX acquired and that it returns
> + * with MUTEX acquired. OVS_RELEASES(...) indicates the reverse.
OVS_RELEASES applies to both read-locks and write-locks?
I guess that an ordinary mutex is considered a write-lock?
> + * OVS_TRY_RDLOCK(...) and OVS_TRY_WRLOCK(...) indicate that the
> + * function will try to acquire MUTEX. The macro takes one or more
> + * arguments. The first argument is an interger or boolean value
> + * specifying the return value of a successful lock acquisition.
> + * The remaining arguments are MUTEX.
> + *
> + * OVS_REQ_RDLOCK(...) and OVS_REQ_WRLOCK(...) indicate that the
> + * function must be called with MUTEX acquired by the caller and that
> + * the function does not release MUTEX.
> + *
> + * OVS_LOCKS_EXCLUDED(MUTEX) indicates that the function must be called
> + * without MUTEX acquired. */
In lockfile.c, I guess that clang can only handle guarded-by on
pointers? Otherwise I don't see why one would introduce the new
lock_table variable as a level of indirection. Assuming that's true,
can we declare lock_table as a const pointer, e.g.
static struct hmap *const lock_table OVS_GUARDED_BY(lock_table_mutex)
= &lock_table__;
(That is, the pointer is const, not what it points to.)
This commit adds more of the "#define macro(x) macro(x,
SOURCE_LOCATOR)" trick that Ed Maste rightly reported as confusing. I
wrote a patch to get rid of that some time ago, but I guess it didn't
make it to email somehow (it *was* in my reviews repository). I've
re-sent it now
(http://openvswitch.org/pipermail/dev/2013-July/030009.html), and it
would be nice not to introduce more instances in the meantime.
In ovs-thread.c, I see a number of casts from const to nonconst that
don't use CONST_CAST. I suggest that they should.
I'd add semicolons at the ends of these lines unless it makes the
compiler complain:
> +INIT_FUNCTION(mutex)
> +INIT_FUNCTION(rwlock)
The big macros in ovs-thread.c might be slightly more readable if the
names of the arguments were shortened (e.g. LOCK_TYPE => TYPE,
LOCK_FUN => FUN).
I see a little bit of an abstraction mismatch in ovs-thread.h. The
changes to OVS_MUTEX_ADAPTIVE don't make sense, because the new value
isn't useful as an argument to pthread_mutexattr_settype(), as the
previous value was. Also, I'm not sure that it makes sense to
maintain the viewpoint of the big comment, since we're defining a new
type here, not supplementing standard types anymore. Also, the
ordering and arrangement seemed a little odd. I'm appending a
suggested incremental to fold in.
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index 54e1791..89790ff 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -22,59 +22,55 @@
#include <sys/types.h>
#include "ovs-atomic.h"
#include "util.h"
+
+/* Mutex. */
struct OVS_LOCKABLE ovs_mutex {
pthread_mutex_t lock;
const char *where;
};
-struct OVS_LOCKABLE ovs_rwlock {
- pthread_rwlock_t lock;
- const char *where;
-};
-
-#define OVS_RWLOCK_INITIALIZER { PTHREAD_RWLOCK_INITIALIZER, NULL }
-#define OVS_MUTEX_INITIALIZER { PTHREAD_MUTEX_INITIALIZER, NULL }
-
-/* glibc has some non-portable mutex types and initializers:
+/* "struct ovs_mutex" initializers:
*
- * - PTHREAD_MUTEX_ADAPTIVE_NP is a mutex type that works as a spinlock that
- * falls back to a mutex after spinning for some number of iterations.
+ * - OVS_MUTEX_INITIALIZER: common case.
*
- * - PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP is a non-portable initializer
- * for an error-checking mutex.
+ * - OVS_ADAPTIVE_MUTEX_INITIALIZER for a mutex that spins briefly then goes
+ * to sleeps after some number of iterations.
*
- * We use these definitions to fall back to PTHREAD_MUTEX_NORMAL instead in
- * these cases.
- *
- * (glibc has other non-portable initializers, but we can't reasonably
- * substitute for them here.) */
+ * - OVS_ERRORCHECK_MUTEX_INITIALIZER for a mutex that . */
+#define OVS_MUTEX_INITIALIZER { PTHREAD_MUTEX_INITIALIZER, NULL }
#ifdef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
-#define OVS_MUTEX_ADAPTIVE { PTHREAD_MUTEX_ADAPTIVE_NP, NULL }
#define OVS_ADAPTIVE_MUTEX_INITIALIZER \
{ PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP, NULL }
#else
-#define OVS_MUTEX_ADAPTIVE { PTHREAD_MUTEX_NORMAL }
#define OVS_ADAPTIVE_MUTEX_INITIALIZER { PTHREAD_MUTEX_INITIALIZER, NULL }
#endif
-
#ifdef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
#define OVS_ERRORCHECK_MUTEX_INITIALIZER \
{ PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP, NULL }
#else
-#define OVS_ERRORCHECK_MUTEX_INITIALIZER \
- OVS_MUTEX_INITIALIZER
+#define OVS_ERRORCHECK_MUTEX_INITIALIZER OVS_MUTEX_INITIALIZER
#endif
-
-/* Simple wrappers for pthreads functions. Most of these functions abort the
- * process with an error message on any error. The *_trylock() functions are
- * exceptions: they pass through a 0 or EBUSY return value to the caller and
- * abort on any other error. */
+/* Mutex types, suitable for use with pthread_mutexattr_settype().
+ * There is only one nonstandard type:
+ *
+ * - PTHREAD_MUTEX_ADAPTIVE_NP, the type used for
+ * OVS_ADAPTIVE_MUTEX_INITIALIZER. */
+#ifdef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
+#define OVS_MUTEX_ADAPTIVE PTHREAD_MUTEX_ADAPTIVE_NP
+#else
+#define OVS_MUTEX_ADAPTIVE PTHREAD_MUTEX_NORMAL
+#endif
+
+/* ovs_mutex functions analogous to pthread_mutex_*() functions.
+ *
+ * Most of these functions abort the process with an error message on any
+ * error. ovs_mutex_trylock() is an exception: it passes through a 0 or EBUSY
+ * return value to the caller and aborts on any other error. */
void ovs_mutex_init(const struct ovs_mutex *, pthread_mutexattr_t *);
void ovs_mutex_destroy(const struct ovs_mutex *);
void ovs_mutex_unlock(const struct ovs_mutex *mutex) OVS_RELEASES(mutex);
-
void ovs_mutex_lock(const struct ovs_mutex *mutex, const char *where)
OVS_ACQ_WRLOCK(mutex);
#define ovs_mutex_lock(mutex) \
@@ -87,6 +83,26 @@ int ovs_mutex_trylock(const struct ovs_mutex *mutex, const
char *where)
void ovs_mutex_cond_wait(pthread_cond_t *, const struct ovs_mutex *);
+/* Wrappers for pthread_mutexattr_*() that abort the process on any error. */
+void xpthread_mutexattr_init(pthread_mutexattr_t *);
+void xpthread_mutexattr_destroy(pthread_mutexattr_t *);
+void xpthread_mutexattr_settype(pthread_mutexattr_t *, int type);
+void xpthread_mutexattr_gettype(pthread_mutexattr_t *, int *typep);
+
+/* Read-write lock. */
+struct OVS_LOCKABLE ovs_rwlock {
+ pthread_rwlock_t lock;
+ const char *where;
+};
+
+/* Initializer. */
+#define OVS_RWLOCK_INITIALIZER { PTHREAD_RWLOCK_INITIALIZER, NULL }
+
+/* ovs_rwlock functions analogous to pthread_rwlock_*() functions.
+ *
+ * Most of these functions abort the process with an error message on any
+ * error. The "trylock" functions are exception: they pass through a 0 or
+ * EBUSY return value to the caller and abort on any other error. */
void ovs_rwlock_init(const struct ovs_rwlock *, pthread_rwlockattr_t *);
void ovs_rwlock_destroy(const struct ovs_rwlock *);
void ovs_rwlock_unlock(const struct ovs_rwlock *rwlock) OVS_RELEASES(rwlock);
@@ -110,19 +126,15 @@ int ovs_rwlock_tryrdlock(const struct ovs_rwlock *rwlock,
const char *where)
OVS_TRY_RDLOCK(0, rwlock);
#define ovs_rwlock_tryrdlock(rwlock) \
ovs_rwlock_tryrdlock(rwlock, SOURCE_LOCATOR)
-
-void xpthread_mutexattr_init(pthread_mutexattr_t *);
-void xpthread_mutexattr_destroy(pthread_mutexattr_t *);
-void xpthread_mutexattr_settype(pthread_mutexattr_t *, int type);
-void xpthread_mutexattr_gettype(pthread_mutexattr_t *, int *typep);
-
+
+/* Wrappers for xpthread_cond_*() that abort the process on any error.
+ *
+ * Use ovs_mutex_cond_wait() to wait for a condition. */
void xpthread_cond_init(pthread_cond_t *, pthread_condattr_t *);
void xpthread_cond_destroy(pthread_cond_t *);
void xpthread_cond_signal(pthread_cond_t *);
void xpthread_cond_broadcast(pthread_cond_t *);
-void xpthread_cond_wait(pthread_cond_t *, const struct ovs_mutex *mutex)
- OVS_REQ_WRLOCK(mutex);
-
+
#ifdef __CHECKER__
/* Replace these functions by the macros already defined in the <pthread.h>
* annotations, because the macro definitions have correct semantics for the
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
