Re: [PATCH 3/9] Add external interface changes: new lock types for pthread_mutex_t

["Carlos O'Donell" <carlos at redhat dot com>]

On 05/10/2013 06:41 PM, 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.

While you did add mention of this macro in your manual section,
it needs a definition in the threads.texi proper.

Please add a section for "initilize and destroy a mutex",
into which you should define only the new functionality you're
added e.g. static initialization via:
`pthread_mutex_t mutex = PTHREAD_MUTEX_INIT_NP(...)'.
Don't worry that it leaves the manual incomplete. You're not
responsible for completing it.

> 
> 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-02  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.

ChangeLog should mention `Define PTHREAD_MUTEX_INIT_NP' somewhere since
it's the important thing to mention as the new macro.

> ---
>  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,

OK.

>    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

OK.

>  };
>  #define PTHREAD_MUTEX_PSHARED_BIT 128
>  
>  #define PTHREAD_MUTEX_TYPE(m) \
>    ((m)->__data.__kind & 127)
> +#define PTHREAD_MUTEX_TYPE_EL(m) \
> +  ((m)->__data.__kind & (127|PTHREAD_MUTEX_ELISION_FLAGS_NP))

OK.

>  
>  #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..5afadaa 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,
> +  PTHREAD_MUTEX_PSHARED_NP    = 128
> +

OK.

>  #if defined __USE_UNIX98 || defined __USE_XOPEN2K8
>    ,
>    PTHREAD_MUTEX_NORMAL = PTHREAD_MUTEX_TIMED_NP,
> @@ -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
> +

OK (Note __PTHREAD_MUTEX_HAVE_ELISION comes from pthreadtypes.h changes in patch 4).

>  #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. */
> +#  define PTHREAD_MUTEX_INIT_NP(type) \
> +   { { 0, 0, 0, 0, (type), __PTHREAD_SPINS, { 0, 0 } } }

OK.

>  # 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 } } }
> +

OK.

>  # 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 = (1U << 7),
> +  PTHREAD_RWLOCK_NO_ELISION_NP = (1U << 8)

Can you make this consistent with what we use for mutex's e.g. 128, 256?

If you want you can have a follow-on patch to clean it all up and make
them shifts if you think that looks better, but for now lets stick to
the style you already used.

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

Cheers,
Carlos.