[ovs-dev,PATCHv17,1/2] ovs-thread: Add pthread spin lock support.
diff mbox series

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.
Related show

Commit Message

William Tu July 17, 2019, 8:23 p.m. UTC
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(+)

Comments

Ilya Maximets July 18, 2019, 4:38 p.m. UTC | #1
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.
William Tu July 18, 2019, 4:42 p.m. UTC | #2
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.
Ben Pfaff July 18, 2019, 6:03 p.m. UTC | #3
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>
Ben Pfaff July 18, 2019, 6:08 p.m. UTC | #4
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.
William Tu July 18, 2019, 7:17 p.m. UTC | #5
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
Ilya Maximets July 19, 2019, 2:46 p.m. UTC | #6
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.

Patch
diff mbox series

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