diff mbox series

[ovs-dev,PATCHv16,1/2] ovs-thread: Add pthread spin lock support.

Message ID 1562975456-97888-1-git-send-email-u9012063@gmail.com
State Changes Requested
Headers show
Series [ovs-dev,PATCHv16,1/2] ovs-thread: Add pthread spin lock support. | expand

Commit Message

William Tu July 12, 2019, 11:50 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>
---
 include/openvswitch/thread.h | 22 ++++++++++++++++++++++
 lib/ovs-thread.c             | 31 +++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

Comments

Ilya Maximets July 15, 2019, 2:28 p.m. UTC | #1 
On 13.07.2019 2:50, 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.

IIUC, pthread spinlock requires some specific glibc verions (>= 2.2)
so it could be not supported even on Linux. Instead of checking
_POSIX_C_SOURCE version, I think it's better to just check for
pthread_spin_lock function in configure script with AC_CHECK_FUNC and
check for the resulted macro.
Additionally we could check for this macro while checking AF_XDP support
to not enable it if we have no spinlocks.

What do you think?

Best regards, Ilya Maximets.

> 
> Signed-off-by: William Tu <u9012063@gmail.com>
> ---
>  include/openvswitch/thread.h | 22 ++++++++++++++++++++++
>  lib/ovs-thread.c             | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/include/openvswitch/thread.h b/include/openvswitch/thread.h
> index 2987db37c9dc..14cc9ad73900 100644
> --- a/include/openvswitch/thread.h
> +++ b/include/openvswitch/thread.h
> @@ -33,6 +33,13 @@ struct OVS_LOCKABLE ovs_mutex {
>      const char *where;          /* NULL if and only if uninitialized. */
>  };
>  
> +#ifdef __linux__
> +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 +77,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 __linux__
> +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..29a0b9e57acd 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 __linux__
> +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 __linux__
> +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 __linux__
> +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 __linux__
> +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_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
>
Ben Pfaff July 15, 2019, 6:54 p.m. UTC | #2 
On Mon, Jul 15, 2019 at 05:28:44PM +0300, Ilya Maximets wrote:
> On 13.07.2019 2:50, 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.
> 
> IIUC, pthread spinlock requires some specific glibc verions (>= 2.2)
> so it could be not supported even on Linux. Instead of checking
> _POSIX_C_SOURCE version, I think it's better to just check for
> pthread_spin_lock function in configure script with AC_CHECK_FUNC and
> check for the resulted macro.
> Additionally we could check for this macro while checking AF_XDP support
> to not enable it if we have no spinlocks.

glibc 2.2 was released in 2000.  I don't think that it is worth worrying
that a user might have such an ancient version of glibc.

However, I think that feature checks are generally better, so I support
the change.
William Tu July 15, 2019, 8:58 p.m. UTC | #3 
On Mon, Jul 15, 2019 at 11:54 AM Ben Pfaff <blp@ovn.org> wrote:
>
> On Mon, Jul 15, 2019 at 05:28:44PM +0300, Ilya Maximets wrote:
> > On 13.07.2019 2:50, 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.
> >
> > IIUC, pthread spinlock requires some specific glibc verions (>= 2.2)
> > so it could be not supported even on Linux. Instead of checking
> > _POSIX_C_SOURCE version, I think it's better to just check for
> > pthread_spin_lock function in configure script with AC_CHECK_FUNC and
> > check for the resulted macro.
> > Additionally we could check for this macro while checking AF_XDP support
> > to not enable it if we have no spinlocks.
>
> glibc 2.2 was released in 2000.  I don't think that it is worth worrying
> that a user might have such an ancient version of glibc.
>
> However, I think that feature checks are generally better, so I support
> the change.

Thanks Ben and Ilya.

I will add AC_CHECK_FUNC for checking the existence of
pthread_spin_lock. If not exist, this patch won't get compile
in and the following AF_XDP will be disable.

Feature Test Macro Requirements for pthread_spin_lock
_POSIX_C_SOURCE >= 200112L
Since it's been a while, I won't add the _POSIX_C_SOURCE  check.

William
Ilya Maximets July 16, 2019, 3:48 p.m. UTC | #4 
On 13.07.2019 2:50, 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.
> 
> Signed-off-by: William Tu <u9012063@gmail.com>
> ---
>   include/openvswitch/thread.h | 22 ++++++++++++++++++++++
>   lib/ovs-thread.c             | 31 +++++++++++++++++++++++++++++++
>   2 files changed, 53 insertions(+)
> 
> diff --git a/include/openvswitch/thread.h b/include/openvswitch/thread.h
> index 2987db37c9dc..14cc9ad73900 100644
> --- a/include/openvswitch/thread.h
> +++ b/include/openvswitch/thread.h
> @@ -33,6 +33,13 @@ struct OVS_LOCKABLE ovs_mutex {
>       const char *where;          /* NULL if and only if uninitialized. */
>   };
>   
> +#ifdef __linux__
> +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 +77,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 __linux__
> +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..29a0b9e57acd 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 __linux__
> +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 __linux__
> +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 __linux__
> +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 __linux__
> +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_failed");

s/pthread_spin_failed/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
>
diff mbox series

Patch

diff --git a/include/openvswitch/thread.h b/include/openvswitch/thread.h
index 2987db37c9dc..14cc9ad73900 100644
--- a/include/openvswitch/thread.h
+++ b/include/openvswitch/thread.h
@@ -33,6 +33,13 @@  struct OVS_LOCKABLE ovs_mutex {
     const char *where;          /* NULL if and only if uninitialized. */
 };
 
+#ifdef __linux__
+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 +77,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 __linux__
+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..29a0b9e57acd 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 __linux__
+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 __linux__
+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 __linux__
+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 __linux__
+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_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