Re: [PATCH 03/11] Add external interface changes: new lock types for pthread_mutex_t

[Torvald Riegel <triegel at redhat dot com>]

On Tue, 2013-06-11 at 09:50 -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Add a new PTHREAD_MUTEX_INIT_NP() macro that allows to set generic
> mutex type/flags combinations. Expose PTHREAD_MUTEX_ELISION_NP
> and PTHREAD_MUTEX_NO_ELISION_NP flags. In addition also expose
> the existing PTHREAD_MUTEX_PSHARED_NP flag.
> 
> Similar flags are defined for the rwlocks.
> 
> This allows programs to set elision in a fine grained matter.
> See the manual for more details.
> 
> recursive/pi/error checking mutexes do not elide.
> Recursive could be implemented at some point, but are not currently.
> The initializer allows to set illegal combinations currently.
> 
> 2013-05-16  Andi Kleen  <ak@linux.intel.com>
> 
> 	* pthreadP.h: Add elision types.
>           (PTHREAD_MUTEX_TYPE_EL): Add.
> 	* sysdeps/pthread/pthread.h: Add elision initializers.
> 	  (PTHREAD_MUTEX_ELISION_NP, PTHREAD_MUTEX_NO_ELISION_NP,
> 	   PTHREAD_MUTEX_PSHARED_NP): Add new flags.
>           (__PTHREAD_SPINS): Add.
> 	  Update mutex initializers.
> 	  (PTHREAD_RWLOCK_ELISION_NP, PTHREAD_RWLOCK_NO_ELISION_NP): Add.
> 	  Define PTHREAD_MUTEX_INIT_NP.
> ---
>  nptl/pthreadP.h                | 17 ++++++++++++--
>  nptl/sysdeps/pthread/pthread.h | 51 +++++++++++++++++++++++++++++++++---------
>  2 files changed, 56 insertions(+), 12 deletions(-)
> 
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 31cae86..50196e4 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -60,7 +60,7 @@
>  /* Internal mutex type value.  */
>  enum
>  {
> -  PTHREAD_MUTEX_KIND_MASK_NP = 3,
> +  PTHREAD_MUTEX_KIND_MASK_NP = 7,
>    PTHREAD_MUTEX_ROBUST_NORMAL_NP = 16,
>    PTHREAD_MUTEX_ROBUST_RECURSIVE_NP
>    = PTHREAD_MUTEX_ROBUST_NORMAL_NP | PTHREAD_MUTEX_RECURSIVE_NP,
> @@ -93,12 +93,25 @@ enum
>    PTHREAD_MUTEX_PP_ERRORCHECK_NP
>    = PTHREAD_MUTEX_PRIO_PROTECT_NP | PTHREAD_MUTEX_ERRORCHECK_NP,
>    PTHREAD_MUTEX_PP_ADAPTIVE_NP
> -  = PTHREAD_MUTEX_PRIO_PROTECT_NP | PTHREAD_MUTEX_ADAPTIVE_NP
> +  = PTHREAD_MUTEX_PRIO_PROTECT_NP | PTHREAD_MUTEX_ADAPTIVE_NP,
> +  PTHREAD_MUTEX_ELISION_FLAGS_NP
> +  = PTHREAD_MUTEX_ELISION_NP | PTHREAD_MUTEX_NO_ELISION_NP,
> +
> +  PTHREAD_MUTEX_TIMED_ELISION_NP =
> +	  PTHREAD_MUTEX_TIMED_NP | PTHREAD_MUTEX_ELISION_NP,
> +  PTHREAD_MUTEX_TIMED_NO_ELISION_NP =
> +	  PTHREAD_MUTEX_TIMED_NP | PTHREAD_MUTEX_NO_ELISION_NP,
> +  PTHREAD_MUTEX_ADAPTIVE_ELISION_NP =
> +	  PTHREAD_MUTEX_ADAPTIVE_NP | PTHREAD_MUTEX_ELISION_NP,
> +  PTHREAD_MUTEX_ADAPTIVE_NO_ELISION_NP =
> +	  PTHREAD_MUTEX_ADAPTIVE_NP | PTHREAD_MUTEX_NO_ELISION_NP
>  };
>  #define PTHREAD_MUTEX_PSHARED_BIT 128
>  
>  #define PTHREAD_MUTEX_TYPE(m) \
>    ((m)->__data.__kind & 127)
> +#define PTHREAD_MUTEX_TYPE_EL(m) \

At least call this _ELISION and not just _EL.  You're not using EL as
abbreviation anywhere else, so please give it a name that makes a little
bit clear what it actually is.

> +  ((m)->__data.__kind & (127|PTHREAD_MUTEX_ELISION_FLAGS_NP))
>  
>  #if LLL_PRIVATE == 0 && LLL_SHARED == 128
>  # define PTHREAD_MUTEX_PSHARED(m) \
> diff --git a/nptl/sysdeps/pthread/pthread.h b/nptl/sysdeps/pthread/pthread.h
> index 10bcb80..bba5852 100644
> --- a/nptl/sysdeps/pthread/pthread.h
> +++ b/nptl/sysdeps/pthread/pthread.h
> @@ -44,7 +44,12 @@ enum
>    PTHREAD_MUTEX_TIMED_NP,
>    PTHREAD_MUTEX_RECURSIVE_NP,
>    PTHREAD_MUTEX_ERRORCHECK_NP,
> -  PTHREAD_MUTEX_ADAPTIVE_NP
> +  PTHREAD_MUTEX_ADAPTIVE_NP,
> +
> +  PTHREAD_MUTEX_ELISION_NP    = 1024,
> +  PTHREAD_MUTEX_NO_ELISION_NP = 2048,

This needs documentation, if not here then somewhere else.  Also note my
comments in the reply to the whole patch set: This should be a
performance hint, not something that changes semantics.

> +  PTHREAD_MUTEX_PSHARED_NP    = 128
> +
>  #if defined __USE_UNIX98 || defined __USE_XOPEN2K8
>    ,
>    PTHREAD_MUTEX_NORMAL = PTHREAD_MUTEX_TIMED_NP,

We cannot use elision for PTHREAD_MUTEX_NORMAL without changing the
semantics.  See the guidelines, and the discussion that led to those
guidelines.

Please give PTHREAD_MUTEX_DEFAULT a new enum value, leaving
PTHREAD_MUTEX_NORMAL unchanged.  Next, don't use elision for
PTHREAD_MUTEX_NORMAL.

I *guess* PTHREAD_MUTEX_TIMED_NP is intended to behave like
PTHREAD_MUTEX_NORMAL (ie, it would have to stay unchanged too).  Or are
they supposed to follow the MUTEX_DEFAULT semantics; this would allow
elision to be used more widely.  Does anybody know for sure?


> @@ -83,27 +88,50 @@ enum
>  
> 
>  /* Mutex initializers.  */
> +#if __PTHREAD_MUTEX_HAVE_ELISION == 1
> +#define __PTHREAD_SPINS 0, 0
> +#elif __PTHREAD_MUTEX_HAVE_ELISION == 2
> +#define __PTHREAD_SPINS { 0, 0 }
> +#else
> +#define __PTHREAD_SPINS 0
> +#endif
> +
>  #ifdef __PTHREAD_MUTEX_HAVE_PREV
>  # define PTHREAD_MUTEX_INITIALIZER \
> -  { { 0, 0, 0, 0, 0, 0, { 0, 0 } } }
> +  { { 0, 0, 0, 0, 0, __PTHREAD_SPINS, { 0, 0 } } }
>  # ifdef __USE_GNU
>  #  define PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP \
> -  { { 0, 0, 0, 0, PTHREAD_MUTEX_RECURSIVE_NP, 0, { 0, 0 } } }
> +  { { 0, 0, 0, 0, PTHREAD_MUTEX_RECURSIVE_NP, __PTHREAD_SPINS, { 0, 0 } } }
>  #  define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP \
> -  { { 0, 0, 0, 0, PTHREAD_MUTEX_ERRORCHECK_NP, 0, { 0, 0 } } }
> +  { { 0, 0, 0, 0, PTHREAD_MUTEX_ERRORCHECK_NP, __PTHREAD_SPINS, { 0, 0 } } }
> +#  define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP \
> +  { { 0, 0, 0, 0, PTHREAD_MUTEX_ADAPTIVE_NP, __PTHREAD_SPINS, { 0, 0 } } }
>  #  define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP \
> -  { { 0, 0, 0, 0, PTHREAD_MUTEX_ADAPTIVE_NP, 0, { 0, 0 } } }
> +  { { 0, 0, 0, 0, PTHREAD_MUTEX_ADAPTIVE_NP, __PTHREAD_SPINS, { 0, 0 } } }
> +
> +/* Generic initializer allowing to specify type and additional flags.
> +   Valid types are
> +   PTHREAD_MUTEX_TIMED_NP, PTHREAD_MUTEX_ADAPTIVE_NP, ...
> +   Valid flags are 
> +   PTHREAD_MUTEX_PSHARED_NP, PTHREAD_MUTEX_ELISION_NP, PTHREAD_MUTEX_NO_ELISION_NP.
> +   Both are or'ed together. Some combinations are not legal. */

Whitespace.  s/or'ed together/combined with a bit-wise Or operator/

> +#  define PTHREAD_MUTEX_INIT_NP(type) \
> +   { { 0, 0, 0, 0, (type), __PTHREAD_SPINS, { 0, 0 } } }
>  # endif
>  #else
>  # define PTHREAD_MUTEX_INITIALIZER \
> -  { { 0, 0, 0, 0, 0, { 0 } } }
> +  { { 0, 0, 0, 0, 0, { __PTHREAD_SPINS } } }
>  # ifdef __USE_GNU
>  #  define PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP \
> -  { { 0, 0, 0, PTHREAD_MUTEX_RECURSIVE_NP, 0, { 0 } } }
> +  { { 0, 0, 0, PTHREAD_MUTEX_RECURSIVE_NP, 0, { __PTHREAD_SPINS } } }
>  #  define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP \
> -  { { 0, 0, 0, PTHREAD_MUTEX_ERRORCHECK_NP, 0, { 0 } } }
> +  { { 0, 0, 0, PTHREAD_MUTEX_ERRORCHECK_NP, 0, { __PTHREAD_SPINS } } }
>  #  define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP \
> -  { { 0, 0, 0, PTHREAD_MUTEX_ADAPTIVE_NP, 0, { 0 } } }
> +  { { 0, 0, 0, PTHREAD_MUTEX_ADAPTIVE_NP, 0, { __PTHREAD_SPINS } } }
> +/* Generic initializer allowing to specify type and additional flags. */
> +#  define PTHREAD_MUTEX_INIT_NP(type) \
> +  { { 0, 0, 0, (type), 0, { __PTHREAD_SPINS } } }
> +
>  # endif
>  #endif
>  
> @@ -115,7 +143,10 @@ enum
>    PTHREAD_RWLOCK_PREFER_READER_NP,
>    PTHREAD_RWLOCK_PREFER_WRITER_NP,
>    PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP,
> -  PTHREAD_RWLOCK_DEFAULT_NP = PTHREAD_RWLOCK_PREFER_READER_NP
> +  PTHREAD_RWLOCK_DEFAULT_NP = PTHREAD_RWLOCK_PREFER_READER_NP,
> +
> +  PTHREAD_RWLOCK_ELISION_NP = 128,
> +  PTHREAD_RWLOCK_NO_ELISION_NP = 256 

Needs documentation. See above.

> 
>  };
>  
>  /* Define __PTHREAD_RWLOCK_INT_FLAGS_SHARED to 1 if pthread_rwlock_t