This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Andi,

This patch should get split into two parts.

One part public, which goes into a second patch set.

One part private.

On 06/24/2013 02:24 PM, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Add the new elision/no elision flags that can be set by
> pthread_mutexattr_settype.  Setting elision with initializers
> is not supported anymore, to improve compatibility with
> old glibcs.
> 
> 2013-06-18  Andi Kleen  <ak@linux.intel.com>
> 
> 	* pthreadP.h: Add elision types.
>           (PTHREAD_MUTEX_TYPE_ELISION): 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.
> ---
>  nptl/pthreadP.h                | 17 +++++++++++++++--
>  nptl/sysdeps/pthread/pthread.h | 40 ++++++++++++++++++++++++++++++----------
>  2 files changed, 45 insertions(+), 12 deletions(-)
> 
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 7883fdf..72d5829 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,

No change here for private implementation, since the public
parts are in a second changeset, so it remains at 3.

>    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_ELISION(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 ded5ae5..3dc552f 100644
> --- a/nptl/sysdeps/pthread/pthread.h
> +++ b/nptl/sysdeps/pthread/pthread.h

This file can only have limited implementation defined changes
in a first changeset that doesn't change ABI/API.

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

These defines are needed internally and used by you and can
move into pthreadP.h.

By not exposing them in the first patch set we don't fix
the ABI.

Otherwise, is there any reason you chose 1024 and 2024 and
not 256 and 512? I think we should just use the next available
values.

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

You need to add comments saying exactly what
__PTHREAD_MUTEX_HAVE_ELISION 1 and 2 mean
(with similar short comment in bits/pthreadtypes.h).

> +
>  #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, 0, { 0, 0 } } }
> +  { { 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, __PTHREAD_SPINS, { 0, 0 } } }

OK, internal implementation details.

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

OK, internal implementation details.

> +
>  # endif
>  #endif
>  
> @@ -115,7 +132,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 

Not OK, needs to move into pthreadP.h if you need it or into
a second patchset for the public ABI/API bits.

Otherwise it looks OK to me, leaving the low 8-bits for future
kind values is OK and similar to what we have for a mutex.

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

Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]