This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 03/11] Add external interface changes: new lock types for pthread_mutex_t
- From: Torvald Riegel <triegel at redhat dot com>
- To: Andi Kleen <andi at firstfloor dot org>
- Cc: libc-alpha at sourceware dot org, Andi Kleen <ak at linux dot jf dot intel dot com>
- Date: Thu, 13 Jun 2013 18:27:22 +0200
- Subject: Re: [PATCH 03/11] Add external interface changes: new lock types for pthread_mutex_t
- References: <1370969416-8337-1-git-send-email-andi at firstfloor dot org> <1370969416-8337-4-git-send-email-andi at firstfloor dot org>
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