| Message ID | 1563395014-41764-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,PATCHv17,1/2] ovs-thread: Add pthread spin lock support. | expand |
On 17.07.2019 23:23, William Tu wrote: > The patch adds the basic spin lock functions: > ovs_spin_{lock, try_lock, unlock, init, destroy}. > OSX does not support pthread spin lock, so make it > linux only. The last sentence seems redundant now and could be removed. The patch looks good to me. One thing I'd like to add is the additional AC_SEARCH_LIBS([pthread_spin_lock], [pthread]) This way we'll be sure that '-lpthread' will be passed to the function check if needed. So, I'm suggesting: diff --git a/configure.ac b/configure.ac index c33935499..398a390a5 100644 --- a/configure.ac +++ b/configure.ac @@ -79,6 +79,8 @@ AC_SEARCH_LIBS([clock_gettime], [rt]) AC_SEARCH_LIBS([timer_create], [rt]) AC_SEARCH_LIBS([pthread_rwlock_tryrdlock], [pthread]) AC_SEARCH_LIBS([pthread_rwlockattr_destroy], [pthread]) +AC_SEARCH_LIBS([pthread_spin_lock], [pthread]) +AC_CHECK_FUNCS([pthread_spin_lock]) AC_FUNC_STRERROR_R OVS_CHECK_WIN64 @@ -109,7 +111,6 @@ AC_CHECK_MEMBERS([struct sockaddr_in6.sin6_scope_id], [], [], #include <sys/types.h> #include <netinet/in.h>]]) AC_CHECK_FUNCS([mlockall strnlen getloadavg statvfs getmntent_r sendmmsg clock_gettime]) -AC_CHECK_FUNCS([pthread_spin_lock]) AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h]) AC_CHECK_HEADERS([linux/net_namespace.h stdatomic.h bits/floatn-common.h]) AC_CHECK_HEADERS([net/if_mib.h], [], [], [[#include <sys/types.h> --- If above diff is OK, I could apply it myself right before merging the patch. William, Ben, what do you think? Other than that, Acked-by: Ilya Maximets <i.maximets@samsung.com> Best regards, Ilya Maximets.
On Thu, Jul 18, 2019 at 9:38 AM Ilya Maximets <i.maximets@samsung.com> wrote: > > On 17.07.2019 23:23, William Tu wrote: > > The patch adds the basic spin lock functions: > > ovs_spin_{lock, try_lock, unlock, init, destroy}. > > OSX does not support pthread spin lock, so make it > > linux only. > > The last sentence seems redundant now and could be removed. > right, thanks! > The patch looks good to me. One thing I'd like to add is the additional > AC_SEARCH_LIBS([pthread_spin_lock], [pthread]) > This way we'll be sure that '-lpthread' will be passed to the function > check if needed. > > So, I'm suggesting: > > diff --git a/configure.ac b/configure.ac > index c33935499..398a390a5 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -79,6 +79,8 @@ AC_SEARCH_LIBS([clock_gettime], [rt]) > AC_SEARCH_LIBS([timer_create], [rt]) > AC_SEARCH_LIBS([pthread_rwlock_tryrdlock], [pthread]) > AC_SEARCH_LIBS([pthread_rwlockattr_destroy], [pthread]) > +AC_SEARCH_LIBS([pthread_spin_lock], [pthread]) > +AC_CHECK_FUNCS([pthread_spin_lock]) > AC_FUNC_STRERROR_R > > OVS_CHECK_WIN64 > @@ -109,7 +111,6 @@ AC_CHECK_MEMBERS([struct sockaddr_in6.sin6_scope_id], [], [], > #include <sys/types.h> > #include <netinet/in.h>]]) > AC_CHECK_FUNCS([mlockall strnlen getloadavg statvfs getmntent_r sendmmsg clock_gettime]) > -AC_CHECK_FUNCS([pthread_spin_lock]) > AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h]) > AC_CHECK_HEADERS([linux/net_namespace.h stdatomic.h bits/floatn-common.h]) > AC_CHECK_HEADERS([net/if_mib.h], [], [], [[#include <sys/types.h> > --- > > If above diff is OK, I could apply it myself right before merging the patch. > William, Ben, what do you think? > I'm OK. Regards, William > Other than that, > Acked-by: Ilya Maximets <i.maximets@samsung.com> > > Best regards, Ilya Maximets.
On Wed, Jul 17, 2019 at 01:23:33PM -0700, William Tu wrote: > --- a/include/openvswitch/thread.h > +++ b/include/openvswitch/thread.h > @@ -17,6 +17,8 @@ > #ifndef OPENVSWITCH_THREAD_H > #define OPENVSWITCH_THREAD_H 1 > > +#include <config.h> That shouldn't be there. Every .c file should already have #include <config.h> as its first (non-comment) line. Otherwise, Acked-by: Ben Pfaff <blp@ovn.org>
On Thu, Jul 18, 2019 at 07:38:03PM +0300, Ilya Maximets wrote: > On 17.07.2019 23:23, William Tu wrote: > > The patch adds the basic spin lock functions: > > ovs_spin_{lock, try_lock, unlock, init, destroy}. > > OSX does not support pthread spin lock, so make it > > linux only. > > The last sentence seems redundant now and could be removed. > > The patch looks good to me. One thing I'd like to add is the additional > AC_SEARCH_LIBS([pthread_spin_lock], [pthread]) > This way we'll be sure that '-lpthread' will be passed to the function > check if needed. > > So, I'm suggesting: > > diff --git a/configure.ac b/configure.ac > index c33935499..398a390a5 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -79,6 +79,8 @@ AC_SEARCH_LIBS([clock_gettime], [rt]) > AC_SEARCH_LIBS([timer_create], [rt]) > AC_SEARCH_LIBS([pthread_rwlock_tryrdlock], [pthread]) > AC_SEARCH_LIBS([pthread_rwlockattr_destroy], [pthread]) > +AC_SEARCH_LIBS([pthread_spin_lock], [pthread]) > +AC_CHECK_FUNCS([pthread_spin_lock]) > AC_FUNC_STRERROR_R > > OVS_CHECK_WIN64 > @@ -109,7 +111,6 @@ AC_CHECK_MEMBERS([struct sockaddr_in6.sin6_scope_id], [], [], > #include <sys/types.h> > #include <netinet/in.h>]]) > AC_CHECK_FUNCS([mlockall strnlen getloadavg statvfs getmntent_r sendmmsg clock_gettime]) > -AC_CHECK_FUNCS([pthread_spin_lock]) > AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h]) > AC_CHECK_HEADERS([linux/net_namespace.h stdatomic.h bits/floatn-common.h]) > AC_CHECK_HEADERS([net/if_mib.h], [], [], [[#include <sys/types.h> > --- > > If above diff is OK, I could apply it myself right before merging the patch. > William, Ben, what do you think? Seems OK to me.
On Thu, Jul 18, 2019 at 11:08 AM Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Jul 18, 2019 at 07:38:03PM +0300, Ilya Maximets wrote: > > On 17.07.2019 23:23, William Tu wrote: > > > The patch adds the basic spin lock functions: > > > ovs_spin_{lock, try_lock, unlock, init, destroy}. > > > OSX does not support pthread spin lock, so make it > > > linux only. > > > > The last sentence seems redundant now and could be removed. > > > > The patch looks good to me. One thing I'd like to add is the additional > > AC_SEARCH_LIBS([pthread_spin_lock], [pthread]) > > This way we'll be sure that '-lpthread' will be passed to the function > > check if needed. > > > > So, I'm suggesting: > > > > diff --git a/configure.ac b/configure.ac > > index c33935499..398a390a5 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -79,6 +79,8 @@ AC_SEARCH_LIBS([clock_gettime], [rt]) > > AC_SEARCH_LIBS([timer_create], [rt]) > > AC_SEARCH_LIBS([pthread_rwlock_tryrdlock], [pthread]) > > AC_SEARCH_LIBS([pthread_rwlockattr_destroy], [pthread]) > > +AC_SEARCH_LIBS([pthread_spin_lock], [pthread]) > > +AC_CHECK_FUNCS([pthread_spin_lock]) > > AC_FUNC_STRERROR_R > > > > OVS_CHECK_WIN64 > > @@ -109,7 +111,6 @@ AC_CHECK_MEMBERS([struct sockaddr_in6.sin6_scope_id], [], [], > > #include <sys/types.h> > > #include <netinet/in.h>]]) > > AC_CHECK_FUNCS([mlockall strnlen getloadavg statvfs getmntent_r sendmmsg clock_gettime]) > > -AC_CHECK_FUNCS([pthread_spin_lock]) > > AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h]) > > AC_CHECK_HEADERS([linux/net_namespace.h stdatomic.h bits/floatn-common.h]) > > AC_CHECK_HEADERS([net/if_mib.h], [], [], [[#include <sys/types.h> > > --- > > > > If above diff is OK, I could apply it myself right before merging the patch. > > William, Ben, what do you think? > > Seems OK to me. Thanks Ben and Ilya. I will let Ilya apply this patch. So next v18 version I will skip this one and only send out the netdev-afxdp patch, the second one. --William
On 18.07.2019 21:03, Ben Pfaff wrote: > On Wed, Jul 17, 2019 at 01:23:33PM -0700, William Tu wrote: >> --- a/include/openvswitch/thread.h >> +++ b/include/openvswitch/thread.h >> @@ -17,6 +17,8 @@ >> #ifndef OPENVSWITCH_THREAD_H >> #define OPENVSWITCH_THREAD_H 1 >> >> +#include <config.h> > > That shouldn't be there. Every .c file should already have #include > <config.h> as its first (non-comment) line. > > Otherwise, > Acked-by: Ben Pfaff <blp@ovn.org> > > Thanks William and Ben! I applied my diff, dropped config.h, removed redundant sentence from the commit message and pushed this to master. Best regards, Ilya Maximets.
diff --git a/configure.ac b/configure.ac index a9f0a06dc140..dd2a674af0c9 100644 --- a/configure.ac +++ b/configure.ac @@ -108,6 +108,7 @@ AC_CHECK_MEMBERS([struct sockaddr_in6.sin6_scope_id], [], [], #include <sys/types.h> #include <netinet/in.h>]]) AC_CHECK_FUNCS([mlockall strnlen getloadavg statvfs getmntent_r sendmmsg clock_gettime]) +AC_CHECK_FUNCS([pthread_spin_lock]) AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h]) AC_CHECK_HEADERS([linux/net_namespace.h stdatomic.h bits/floatn-common.h]) AC_CHECK_HEADERS([net/if_mib.h], [], [], [[#include <sys/types.h> diff --git a/include/openvswitch/thread.h b/include/openvswitch/thread.h index 2987db37c9dc..b07b7167b303 100644 --- a/include/openvswitch/thread.h +++ b/include/openvswitch/thread.h @@ -17,6 +17,8 @@ #ifndef OPENVSWITCH_THREAD_H #define OPENVSWITCH_THREAD_H 1 +#include <config.h> + #include <pthread.h> #include <stddef.h> #include <stdbool.h> @@ -33,6 +35,13 @@ struct OVS_LOCKABLE ovs_mutex { const char *where; /* NULL if and only if uninitialized. */ }; +#ifdef HAVE_PTHREAD_SPIN_LOCK +struct OVS_LOCKABLE ovs_spin { + pthread_spinlock_t lock; + const char *where; /* NULL if and only if uninitialized. */ +}; +#endif + /* "struct ovs_mutex" initializer. */ #ifdef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP #define OVS_MUTEX_INITIALIZER { PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP, \ @@ -70,6 +79,21 @@ int ovs_mutex_trylock_at(const struct ovs_mutex *mutex, const char *where) void ovs_mutex_cond_wait(pthread_cond_t *, const struct ovs_mutex *mutex) OVS_REQUIRES(mutex); + +#ifdef HAVE_PTHREAD_SPIN_LOCK +void ovs_spin_init(const struct ovs_spin *); +void ovs_spin_destroy(const struct ovs_spin *); +void ovs_spin_unlock(const struct ovs_spin *spin) OVS_RELEASES(spin); +void ovs_spin_lock_at(const struct ovs_spin *spin, const char *where) + OVS_ACQUIRES(spin); +#define ovs_spin_lock(spin) \ + ovs_spin_lock_at(spin, OVS_SOURCE_LOCATOR) + +int ovs_spin_trylock_at(const struct ovs_spin *spin, const char *where) + OVS_TRY_LOCK(0, spin); +#define ovs_spin_trylock(spin) \ + ovs_spin_trylock_at(spin, OVS_SOURCE_LOCATOR) +#endif /* Convenient once-only execution. * diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c index 159d87e5b0ca..b686e4548127 100644 --- a/lib/ovs-thread.c +++ b/lib/ovs-thread.c @@ -75,6 +75,9 @@ static bool multithreaded; LOCK_FUNCTION(mutex, lock); LOCK_FUNCTION(rwlock, rdlock); LOCK_FUNCTION(rwlock, wrlock); +#ifdef HAVE_PTHREAD_SPIN_LOCK +LOCK_FUNCTION(spin, lock); +#endif #define TRY_LOCK_FUNCTION(TYPE, FUN) \ int \ @@ -103,6 +106,9 @@ LOCK_FUNCTION(rwlock, wrlock); TRY_LOCK_FUNCTION(mutex, trylock); TRY_LOCK_FUNCTION(rwlock, tryrdlock); TRY_LOCK_FUNCTION(rwlock, trywrlock); +#ifdef HAVE_PTHREAD_SPIN_LOCK +TRY_LOCK_FUNCTION(spin, trylock); +#endif #define UNLOCK_FUNCTION(TYPE, FUN, WHERE) \ void \ @@ -125,6 +131,10 @@ UNLOCK_FUNCTION(mutex, unlock, "<unlocked>"); UNLOCK_FUNCTION(mutex, destroy, NULL); UNLOCK_FUNCTION(rwlock, unlock, "<unlocked>"); UNLOCK_FUNCTION(rwlock, destroy, NULL); +#ifdef HAVE_PTHREAD_SPIN_LOCK +UNLOCK_FUNCTION(spin, unlock, "<unlocked>"); +UNLOCK_FUNCTION(spin, destroy, NULL); +#endif #define XPTHREAD_FUNC1(FUNCTION, PARAM1) \ void \ @@ -268,6 +278,27 @@ ovs_mutex_cond_wait(pthread_cond_t *cond, const struct ovs_mutex *mutex_) } } +#ifdef HAVE_PTHREAD_SPIN_LOCK +static void +ovs_spin_init__(const struct ovs_spin *l_, int pshared) +{ + struct ovs_spin *l = CONST_CAST(struct ovs_spin *, l_); + int error; + + l->where = "<unlocked>"; + error = pthread_spin_init(&l->lock, pshared); + if (OVS_UNLIKELY(error)) { + ovs_abort(error, "pthread_spin_init failed"); + } +} + +void +ovs_spin_init(const struct ovs_spin *spin) +{ + ovs_spin_init__(spin, PTHREAD_PROCESS_PRIVATE); +} +#endif + /* Initializes the 'barrier'. 'size' is the number of threads * expected to hit the barrier. */ void
The patch adds the basic spin lock functions: ovs_spin_{lock, try_lock, unlock, init, destroy}. OSX does not support pthread spin lock, so make it linux only. Signed-off-by: William Tu <u9012063@gmail.com> --- configure.ac | 1 + include/openvswitch/thread.h | 24 ++++++++++++++++++++++++ lib/ovs-thread.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+)