The #include_next in sparse/pthread.h is a bit exotic. Would you please include a short comment explaining why it's necessary?
sparse/pthread.h has some trailing newlines at the end of the file. In ovs-thread.c any reason not to macro-ize the trylock functions as well? Acked-by: Ethan Jackson <et...@nicira.com> On Wed, Jun 19, 2013 at 1:17 PM, Ben Pfaff <b...@nicira.com> wrote: > The only tricky part here is that I'm throwing in annotations to allow > "sparse" to report unbalanced locking. > > Signed-off-by: Ben Pfaff <b...@nicira.com> > --- > include/sparse/automake.mk | 1 + > include/sparse/pthread.h | 61 +++++++++++++++++++++++++ > lib/automake.mk | 2 + > lib/compiler.h | 36 ++++++++++++++- > lib/ovs-thread.c | 108 > ++++++++++++++++++++++++++++++++++++++++++++ > lib/ovs-thread.h | 89 ++++++++++++++++++++++++++++++++++++ > 6 files changed, 296 insertions(+), 1 deletions(-) > create mode 100644 include/sparse/pthread.h > create mode 100644 lib/ovs-thread.c > create mode 100644 lib/ovs-thread.h > > diff --git a/include/sparse/automake.mk b/include/sparse/automake.mk > index 1a77500..45ae1f5 100644 > --- a/include/sparse/automake.mk > +++ b/include/sparse/automake.mk > @@ -4,5 +4,6 @@ noinst_HEADERS += \ > include/sparse/math.h \ > include/sparse/netinet/in.h \ > include/sparse/netinet/ip6.h \ > + include/sparse/pthread.h \ > include/sparse/sys/socket.h \ > include/sparse/sys/wait.h > diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h > new file mode 100644 > index 0000000..1e54e95 > --- /dev/null > +++ b/include/sparse/pthread.h > @@ -0,0 +1,61 @@ > +/* > + * Copyright (c) 2013 Nicira, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#ifndef __CHECKER__ > +#error "Use this header only with sparse. It is not a correct > implementation." > +#endif > + > +#include_next <pthread.h> > + > +#include "compiler.h" > + > +int pthread_mutex_lock(pthread_mutex_t *mutex) OVS_ACQUIRES(mutex); > +int pthread_mutex_unlock(pthread_mutex_t *mutex) OVS_RELEASES(mutex); > + > +int pthread_rwlock_rdlock(pthread_rwlock_t *rwlock) OVS_ACQUIRES(rwlock); > +int pthread_rwlock_wrlock(pthread_rwlock_t *rwlock) OVS_ACQUIRES(rwlock); > +int pthread_rwlock_unlock(pthread_rwlock_t *rwlock) OVS_RELEASES(rwlock); > + > +int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t *mutex) > + OVS_MUST_HOLD(mutex); > + > +#define pthread_mutex_trylock(MUTEX) \ > + ({ \ > + int retval = pthread_mutex_trylock(mutex); \ > + if (!retval) { \ > + OVS_ACQUIRE(MUTEX); \ > + } \ > + retval; \ > + }) > + > +#define pthread_rwlock_tryrdlock(RWLOCK) \ > + ({ \ > + int retval = pthread_rwlock_tryrdlock(rwlock); \ > + if (!retval) { \ > + OVS_ACQUIRE(RWLOCK); \ > + } \ > + retval; \ > + }) > +#define pthread_rwlock_trywrlock(RWLOCK) \ > + ({ \ > + int retval = pthread_rwlock_trywrlock(rwlock); \ > + if (!retval) { \ > + OVS_ACQUIRE(RWLOCK); \ > + } \ > + retval; \ > + }) > + > + > diff --git a/lib/automake.mk b/lib/automake.mk > index eec71dd..c6de4fe 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -121,6 +121,8 @@ lib_libopenvswitch_a_SOURCES = \ > lib/ofp-version-opt.c \ > lib/ofpbuf.c \ > lib/ofpbuf.h \ > + lib/ovs-thread.c \ > + lib/ovs-thread.h \ > lib/ovsdb-data.c \ > lib/ovsdb-data.h \ > lib/ovsdb-error.c \ > diff --git a/lib/compiler.h b/lib/compiler.h > index 760389d..f3cbe96 100644 > --- a/lib/compiler.h > +++ b/lib/compiler.h > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc. > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -41,6 +41,40 @@ > #define OVS_UNLIKELY(CONDITION) (!!(CONDITION)) > #endif > > +#ifdef __CHECKER__ > +/* "sparse" annotations for mutexes and mutex-like constructs. > + * > + * In a function prototype, OVS_ACQUIRES(MUTEX) indicates that the function > + * must be called without MUTEX acquired and that it returns with MUTEX > + * acquired. OVS_RELEASES(MUTEX) indicates the reverse. OVS_MUST_HOLD > + * indicates that the function must be called with MUTEX acquired by the > + * caller and that the function does not release MUTEX. > + * > + * In practice, sparse ignores the MUTEX argument. It need not even be a > + * valid expression. It is meant to indicate to human readers what mutex is > + * being acquired. > + * > + * Since sparse ignores MUTEX, it need not be an actual mutex. It can be > + * any construct on which paired acquire and release semantics make sense: > + * read/write locks, temporary memory allocations, whatever. > + * > + * OVS_ACQUIRE, OVS_RELEASE, and OVS_HOLDS are suitable for use within > macros, > + * where there is no function prototype to annotate. */ > +#define OVS_ACQUIRES(MUTEX) __attribute__((context(MUTEX, 0, 1))) > +#define OVS_RELEASES(MUTEX) __attribute__((context(MUTEX, 1, 0))) > +#define OVS_MUST_HOLD(MUTEX) __attribute__((context(MUTEX, 1, 1))) > +#define OVS_ACQUIRE(MUTEX) __context__(MUTEX, 0, 1) > +#define OVS_RELEASE(MUTEX) __context__(MUTEX, 1, 0) > +#define OVS_HOLDS(MUTEX) __context__(MUTEX, 1, 1) > +#else > +#define OVS_ACQUIRES(MUTEX) > +#define OVS_RELEASES(MUTEX) > +#define OVS_MUST_HOLD(MUTEX) > +#define OVS_ACQUIRE(MUTEX) > +#define OVS_RELEASE(MUTEX) > +#define OVS_HOLDS(MUTEX) > +#endif > + > /* ISO C says that a C implementation may choose any integer type for an enum > * that is sufficient to hold all of its values. Common ABIs (such as the > * System V ABI used on i386 GNU/Linux) always use a full-sized "int", even > diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c > new file mode 100644 > index 0000000..f48a659 > --- /dev/null > +++ b/lib/ovs-thread.c > @@ -0,0 +1,108 @@ > +/* > + * Copyright (c) 2013 Nicira, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#include <config.h> > +#include "ovs-thread.h" > +#include <errno.h> > +#include "compiler.h" > +#include "util.h" > + > +#ifdef __CHECKER__ > +/* Omit the definitions in this file because they are somewhat difficult to > + * write without prompting "sparse" complaints, without ugliness or > + * cut-and-paste. Since "sparse" is just a checker, not a compiler, it > + * doesn't matter that we don't define them. */ > +#else > +#define XPTHREAD_FUNC1(FUNCTION, PARAM1) \ > + void \ > + x##FUNCTION(PARAM1 arg1) \ > + { \ > + int error = FUNCTION(arg1); \ > + if (OVS_UNLIKELY(error)) { \ > + ovs_abort(error, "%s failed", #FUNCTION); \ > + } \ > + } > +#define XPTHREAD_FUNC2(FUNCTION, PARAM1, PARAM2) \ > + void \ > + x##FUNCTION(PARAM1 arg1, PARAM2 arg2) \ > + { \ > + int error = FUNCTION(arg1, arg2); \ > + if (OVS_UNLIKELY(error)) { \ > + ovs_abort(error, "%s failed", #FUNCTION); \ > + } \ > + } > + > +XPTHREAD_FUNC2(pthread_mutex_init, pthread_mutex_t *, pthread_mutexattr_t *); > +XPTHREAD_FUNC1(pthread_mutex_lock, pthread_mutex_t *); > +XPTHREAD_FUNC1(pthread_mutex_unlock, pthread_mutex_t *); > + > +int > +xpthread_mutex_trylock(pthread_mutex_t *mutex) > +{ > + int error = pthread_mutex_trylock(mutex); > + if (OVS_UNLIKELY(error && error != EBUSY)) { > + ovs_abort(error, "pthread_mutex_trylock failed"); > + } > + return error; > +} > + > +XPTHREAD_FUNC2(pthread_rwlock_init, > + pthread_rwlock_t *, pthread_rwlockattr_t *); > +XPTHREAD_FUNC1(pthread_rwlock_rdlock, pthread_rwlock_t *); > +XPTHREAD_FUNC1(pthread_rwlock_wrlock, pthread_rwlock_t *); > +XPTHREAD_FUNC1(pthread_rwlock_unlock, pthread_rwlock_t *); > + > +int > +xpthread_rwlock_tryrdlock(pthread_rwlock_t *rwlock) > +{ > + int error = pthread_rwlock_tryrdlock(rwlock); > + if (OVS_UNLIKELY(error && error != EBUSY)) { > + ovs_abort(error, "pthread_rwlock_tryrdlock failed"); > + } > + return error; > +} > + > +int > +xpthread_rwlock_trywrlock(pthread_rwlock_t *rwlock) > +{ > + int error = pthread_rwlock_trywrlock(rwlock); > + if (OVS_UNLIKELY(error && error != EBUSY)) { > + ovs_abort(error, "pthread_rwlock_trywrlock failed"); > + } > + return error; > +} > + > +XPTHREAD_FUNC2(pthread_cond_init, pthread_cond_t *, pthread_condattr_t *); > +XPTHREAD_FUNC1(pthread_cond_signal, pthread_cond_t *); > +XPTHREAD_FUNC1(pthread_cond_broadcast, pthread_cond_t *); > +XPTHREAD_FUNC2(pthread_cond_wait, pthread_cond_t *, pthread_mutex_t *); > + > +typedef void destructor_func(void *); > +XPTHREAD_FUNC2(pthread_key_create, pthread_key_t *, destructor_func *); > + > +void > +xpthread_create(pthread_t *threadp, pthread_attr_t *attr, > + void *(*start)(void *), void *arg) > +{ > + pthread_t thread; > + int error; > + > + error = pthread_create(threadp ? threadp : &thread, attr, start, arg); > + if (error) { > + ovs_abort(error, "pthread_create failed"); > + } > +} > +#endif > diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h > new file mode 100644 > index 0000000..cafeedf > --- /dev/null > +++ b/lib/ovs-thread.h > @@ -0,0 +1,89 @@ > +/* > + * Copyright (c) 2013 Nicira, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#ifndef OVS_THREAD_H > +#define OVS_THREAD_H 1 > + > +#include <pthread.h> > +#include "util.h" > + > +/* glibc has some non-portable mutex types and 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. > + * > + * - PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP is a non-portable initializer > + * for an error-checking mutex. > + * > + * 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.) */ > +#ifdef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP > +#define PTHREAD_MUTEX_ADAPTIVE PTHREAD_MUTEX_ADAPTIVE_NP > +#define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER \ > + PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP > +#else > +#define PTHREAD_MUTEX_ADAPTIVE PTHREAD_MUTEX_NORMAL > +#define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER PTHREAD_MUTEX_INITIALIZER > +#endif > + > +#ifdef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP > +#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER \ > + PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP > +#else > +#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER PTHREAD_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. */ > +void xpthread_mutex_init(pthread_mutex_t *, pthread_mutexattr_t *); > +void xpthread_mutex_lock(pthread_mutex_t *mutex) OVS_ACQUIRES(mutex); > +void xpthread_mutex_unlock(pthread_mutex_t *mutex) OVS_RELEASES(mutex); > +int xpthread_mutex_trylock(pthread_mutex_t *); > + > +void xpthread_rwlock_init(pthread_rwlock_t *, pthread_rwlockattr_t *); > +void xpthread_rwlock_rdlock(pthread_rwlock_t *rwlock) OVS_ACQUIRES(rwlock); > +void xpthread_rwlock_wrlock(pthread_rwlock_t *rwlock) OVS_ACQUIRES(rwlock); > +void xpthread_rwlock_unlock(pthread_rwlock_t *rwlock) OVS_RELEASES(rwlock); > +int xpthread_rwlock_tryrdlock(pthread_rwlock_t *); > +int xpthread_rwlock_trywrlock(pthread_rwlock_t *); > + > +void xpthread_cond_init(pthread_cond_t *, pthread_condattr_t *); > +void xpthread_cond_signal(pthread_cond_t *); > +void xpthread_cond_broadcast(pthread_cond_t *); > +void xpthread_cond_wait(pthread_cond_t *, pthread_mutex_t *mutex) > + OVS_MUST_HOLD(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 > + * conditional acquisition that can't be captured in a function annotation. > + * The difference in semantics from pthread_*() to xpthread_*() does not > matter > + * because sparse is not a compiler. */ > +#define xpthread_mutex_trylock pthread_mutex_trylock > +#define xpthread_rwlock_tryrdlock pthread_rwlock_tryrdlock > +#define xpthread_rwlock_trywrlock pthread_rwlock_trywrlock > +#endif > + > +void xpthread_key_create(pthread_key_t *, void (*destructor)(void *)); > + > +void xpthread_create(pthread_t *, pthread_attr_t *, void *(*)(void *), void > *); > + > +#endif /* ovs-thread.h */ > -- > 1.7.2.5 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
