Re: [PATCH] Disable elision for PTHREAD_MUTEX_NORMAL mutexes

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

On 06/24/2013 01:29 PM, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> This is the incremential patch that should solve the
> PTHREAD_MUTEX_NORMAL semantics issue. 
> Thanks everyone for their input.
> It is essentially Rich Felker's scheme. I'll send a full
> rebased patchkit with the other changes (remove initializer)
> shortly.

Thanks for this.

I have one critical comment below about pthreadP.h, and the
problems which this causes for pthread_mutexattr_gettype
(which are unacceptable IMO).

My impression is that we are going to need to propagate
the new type internally to ensure gettype/settype work
symmetrically.

> ---
> 
> 
> 
> There was a lot of discussion about the POSIX statement that
> PTHREAD_MUTEX_NORMAL "shall" deadlock for mutexes that
> get re-acquired by the same thread.
> 
> Elided mutexes act like recursive locks and would only deadlock
> sometimes (any time an abort happens, e.g. at program startup
> or when context switches or syscalls happen), but not guaranteed
> every time.
> 
> This does not apply to DEFAULT mutexes.  But currently in glibc DEFAULT and
> NORMAL is the same number.
> 
> The only way NORMAL can be set is through pthread_mutexattr_setkind()
> PTHREAD_MUTEX_INITIALIZER initialized mutexes are default.
> 
> So what we do here is:
> - Add a new numeric value for PTHREAD_MUTEX_NORMAL
> - Add a new symbol version for pthread_mutexattr_setkind
> (and its aliases pthread_mutexattr_settype_np and __pthread_mutexattr_settype)
> that disables elision for the new PTHREAD_MUTEX_NORMAL value
> - The compat version for the old symbol version always
> disables elision when a 0 type (old ambigious DEFUALT/NORMAL)
> is passed.
> 
> The result is that old binaries that use pthread_mutexattr_setkind()
> for NORMAL/DEFAULT/TIMED_NP will disable elision, and new programs that
> use it with PTHREAD_MUTEX_NORMAL only (but not default/timed) will
> also disable elision. Any mutex not using pthread_mutexattr_setkind
> or setting an ADAPTIVE type uses elision. The use of
> pthread_mutexattr_setkind() for the default type is expected to be
> rare, so this should not affect many programs.
> 
> Note an alternative would be to just disable nesting for these
> normal mutexes (so always _xabort() before starting a transaction).
> This would require additional checks in the fast path, and
> since this case should be rare we just disable elision instead.
> 
> This adds new symbol versions to pthread, so the ABI test
> files needed to be updated.
> 
> Thanks to Torvald Riegel, Roland McGrath, Richard Henderson, Rich Felker
> for input on the discussion. This patch is based on the proposal from
> Rich Felker, with some help from H.J. on the aliases.
> 
> nptl/:
> 2013-06-21  Andi Kleen  <ak@linux.intel.com>
>             H.J. Lu  <hongjiu.lu@intel.com>
>             Rich Felker <dalias@aerifal.cx>
> 
> 	* Versions (pthread_mutexattr_settype,
> 	  pthread_mutexattr_setkind_np, __pthread_mutexattr_settype): Add
> 	  new version for GLIBC_2.18.
> 	* pthreadP.h: Use PTHREAD_MUTEX_DEFAULT to define derived
> 	  NORMAL mutexes. These disable elision in any case.
> 	* pthread_mutexattr_init.c (pthread_mutexattr_init):
> 	  Use PTHREAD_MUTEX_DEFAULT instead of PTHREAD_MUTEX_NORMAL.
> 	* pthread_mutex_init.c (default_mutexattr): Use
> 	  PTHREAD_MUTEX_DEFAULT as base value.
> 	  (__pthread_mutex_init): Disable elision flags for
> 	  prio/robust.
> 	* pthread_mutexattr_settype.c
> 	  (pthread_mutexattr_settype_worker): Rename from
> 	  __pthread_mutexattr_settype.
> 	  (__pthread_mutexattr_settype_new):
>           Add new symbol version. Disable lock elision for NORMAL type.
> 	  (__pthread_mutexattr_settype_old): New function to disable lock
> 	  elision for old DEFAULT/NORMAL/TIMED type.
> 	* sysdeps/pthread/pthread.h (PTHREAD_MUTEX_NORMAL): Redefine
> 	  as new value.
> 	* tst-mutex5.c: Use PTHREAD_MUTEX_DEFAULT.
> 	* sysdeps/unix/sysv/linux/x86_64/64/nptl/libpthread.abilist:
> 	  Update.
> ---
>  nptl/Versions                                      |  6 +-
>  nptl/pthreadP.h                                    |  4 +-
>  nptl/pthread_mutex_init.c                          | 11 +++-
>  nptl/pthread_mutexattr_init.c                      |  2 +-
>  nptl/pthread_mutexattr_settype.c                   | 74 +++++++++++++++++++---
>  nptl/sysdeps/pthread/pthread.h                     | 11 +++-
>  nptl/tst-mutex5.c                                  |  2 +-
>  .../unix/sysv/linux/i386/nptl/libpthread.abilist   |  3 +
>  .../powerpc/powerpc32/fpu/nptl/libpthread.abilist  |  3 +
>  .../powerpc/powerpc64/nptl/libpthread.abilist      |  3 +
>  .../linux/s390/s390-32/nptl/libpthread.abilist     |  3 +
>  .../linux/s390/s390-64/nptl/libpthread.abilist     |  3 +
>  sysdeps/unix/sysv/linux/sh/nptl/libpthread.abilist |  3 +
>  .../linux/sparc/sparc32/nptl/libpthread.abilist    |  3 +
>  .../linux/sparc/sparc64/nptl/libpthread.abilist    |  3 +
>  .../sysv/linux/x86_64/64/nptl/libpthread.abilist   |  3 +
>  .../sysv/linux/x86_64/x32/nptl/libpthread.abilist  |  4 ++
>  17 files changed, 124 insertions(+), 17 deletions(-)
> 
> diff --git a/nptl/Versions b/nptl/Versions
> index bb11277..7a3da25 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -255,7 +255,11 @@ libpthread {
>    GLIBC_2.18 {
>      pthread_getattr_default_np;
>      pthread_setattr_default_np;
> -  }
> +    /* Changed value of PTHREAD_MUTEX_NORMAL.  */
> +    pthread_mutexattr_settype;
> +    pthread_mutexattr_setkind_np;
> +    __pthread_mutexattr_settype;
> +  };

OK.

>  
>    GLIBC_PRIVATE {
>      __pthread_initialize_minimal;
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 10b4be8..cbf26ca 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -70,7 +70,7 @@ enum
>    = PTHREAD_MUTEX_ROBUST_NORMAL_NP | PTHREAD_MUTEX_ADAPTIVE_NP,
>    PTHREAD_MUTEX_PRIO_INHERIT_NP = 32,
>    PTHREAD_MUTEX_PI_NORMAL_NP
> -  = PTHREAD_MUTEX_PRIO_INHERIT_NP | PTHREAD_MUTEX_NORMAL,
> +  = PTHREAD_MUTEX_PRIO_INHERIT_NP | PTHREAD_MUTEX_DEFAULT,

This...

>    PTHREAD_MUTEX_PI_RECURSIVE_NP
>    = PTHREAD_MUTEX_PRIO_INHERIT_NP | PTHREAD_MUTEX_RECURSIVE_NP,
>    PTHREAD_MUTEX_PI_ERRORCHECK_NP
> @@ -87,7 +87,7 @@ enum
>    = PTHREAD_MUTEX_PRIO_INHERIT_NP | PTHREAD_MUTEX_ROBUST_ADAPTIVE_NP,
>    PTHREAD_MUTEX_PRIO_PROTECT_NP = 64,
>    PTHREAD_MUTEX_PP_NORMAL_NP
> -  = PTHREAD_MUTEX_PRIO_PROTECT_NP | PTHREAD_MUTEX_NORMAL,
> +  = PTHREAD_MUTEX_PRIO_PROTECT_NP | PTHREAD_MUTEX_DEFAULT,

... and this, look wrong to me.

I've just talked to you on IRC and this change is because of the
following:

* Changing PTHREAD_MUTEX_NORMAL to a new value is only important 
  for settype.

* If we don't change PTHREAD_MUTEX_NORMAL in pthreadP.h then all
  the internal types will have a new bit set which means all of
  internal code has to change also.

There is no way around this IMO.

This current patch has to squash NORMAL and DEFAULT wo elision
into one type, which means pthread_mutexattr_gettype can't distinguish
between the two of them, and that's not acceptable.

The call to pthread_mutexattr_gettype needs to return the type the
user passed into the call to settype.

>    PTHREAD_MUTEX_PP_RECURSIVE_NP
>    = PTHREAD_MUTEX_PRIO_PROTECT_NP | PTHREAD_MUTEX_RECURSIVE_NP,
>    PTHREAD_MUTEX_PP_ERRORCHECK_NP
> diff --git a/nptl/pthread_mutex_init.c b/nptl/pthread_mutex_init.c
> index 174d900..f6f0f80 100644
> --- a/nptl/pthread_mutex_init.c
> +++ b/nptl/pthread_mutex_init.c
> @@ -26,8 +26,8 @@
>  
>  static const struct pthread_mutexattr default_mutexattr =
>    {
> -    /* Default is a normal mutex, not shared between processes.  */
> -    .mutexkind = PTHREAD_MUTEX_NORMAL
> +    /* Default is a default mutex, not shared between processes.  */
> +    .mutexkind = PTHREAD_MUTEX_DEFAULT

OK.

>    };
>  
>  
> @@ -129,6 +129,13 @@ __pthread_mutex_init (mutex, mutexattr)
>  				| PTHREAD_MUTEXATTR_FLAG_ROBUST)) != 0)
>      mutex->__data.__kind |= PTHREAD_MUTEX_PSHARED_BIT;
>  
> +  /* Drop elision bits for any unusual flags, except for PSHARED.

Suggest:
"Drop elision if any non-standard flags are present, except for PSHARED."

> +     These can be set implicitely now, but the other code paths don't
> +     expect them for all cases.  */

s/implicitely/implicitly/g

I don't fully understand what you're trying to say in this sentence.

If you're saying that you could erroneously set the elision flags
and the non-standard flags, then that should be a bug.

If you're saying that "We remove the elision flags because the code
paths for non-standard flags do not currently support elision." then
that seems fine, but you should say that instead.

> +  if ((mutex->__data.__kind & 
> +       (PTHREAD_MUTEXATTR_FLAG_BITS & ~PTHREAD_MUTEXATTR_FLAG_PSHARED)))
> +       mutex->__data.__kind &= ~PTHREAD_MUTEX_ELISION_FLAGS_NP;
> +

OK.

>    /* Default values: mutex not used yet.  */
>    // mutex->__count = 0;	already done by memset
>    // mutex->__owner = 0;	already done by memset
> diff --git a/nptl/pthread_mutexattr_init.c b/nptl/pthread_mutexattr_init.c
> index b08eeab..0c955f1 100644
> --- a/nptl/pthread_mutexattr_init.c
> +++ b/nptl/pthread_mutexattr_init.c
> @@ -30,7 +30,7 @@ __pthread_mutexattr_init (attr)
>    /* We use bit 31 to signal whether the mutex is going to be
>       process-shared or not.  By default it is zero, i.e., the mutex is
>       not process-shared.  */
> -  ((struct pthread_mutexattr *) attr)->mutexkind = PTHREAD_MUTEX_NORMAL;
> +  ((struct pthread_mutexattr *) attr)->mutexkind = PTHREAD_MUTEX_DEFAULT;

OK.

>  
>    return 0;
>  }
> diff --git a/nptl/pthread_mutexattr_settype.c b/nptl/pthread_mutexattr_settype.c
> index 92772e6..8897653 100644
> --- a/nptl/pthread_mutexattr_settype.c
> +++ b/nptl/pthread_mutexattr_settype.c
> @@ -18,27 +18,85 @@
>  
>  #include <errno.h>
>  #include <pthreadP.h>
> +#include <shlib-compat.h>
>  
> -
> -int
> -__pthread_mutexattr_settype (attr, kind)
> -     pthread_mutexattr_t *attr;
> -     int kind;
> +static int
> +pthread_mutexattr_settype_worker (pthread_mutexattr_t *attr, int kind)
>  {
>    struct pthread_mutexattr *iattr;
>    int mkind = kind & ~PTHREAD_MUTEX_ELISION_FLAGS_NP;
>  
> -  if (mkind < PTHREAD_MUTEX_NORMAL || mkind > PTHREAD_MUTEX_ADAPTIVE_NP)
> +  if (mkind < PTHREAD_MUTEX_TIMED_NP || mkind > PTHREAD_MUTEX_NORMAL)

OK.

>      return EINVAL;
>    /* Cannot set multiple flags.  */
>    if ((kind & PTHREAD_MUTEX_ELISION_FLAGS_NP) == PTHREAD_MUTEX_ELISION_FLAGS_NP)
>      return EINVAL;
>  
> +  /* When a NORMAL mutex is explicitly specified, default to no elision
> +     to satisfy POSIX's deadlock requirement. Also convert the NORMAL
> +     type to DEFAULT, as the rest of the lock library doesn't have
> +     the code paths for them.  */
> +  if (mkind == PTHREAD_MUTEX_NORMAL)
> +    {
> +      kind = PTHREAD_MUTEX_DEFAULT | (kind & PTHREAD_MUTEX_ELISION_FLAGS_NP);
> +      if ((kind & PTHREAD_MUTEX_ELISION_FLAGS_NP) == 0)
> +        kind |= PTHREAD_MUTEX_NO_ELISION_NP;
> +    }
> +

OK. 

You reset the kind to DEFAULT/NORMAL

>    iattr = (struct pthread_mutexattr *) attr;
>  
>    iattr->mutexkind = (iattr->mutexkind & PTHREAD_MUTEXATTR_FLAG_BITS) | kind;
>  
>    return 0;
>  }
> -weak_alias (__pthread_mutexattr_settype, pthread_mutexattr_setkind_np)
> -strong_alias (__pthread_mutexattr_settype, pthread_mutexattr_settype)
> +
> +int
> +__pthread_mutexattr_settype_new (pthread_mutexattr_t *attr, int kind)
> +{
> +  return pthread_mutexattr_settype_worker (attr, kind);
> +}
> +
> +weak_alias (__pthread_mutexattr_settype_new, __new_pthread_mutexattr_setkind_np)
> +strong_alias (__pthread_mutexattr_settype_new, __new_pthread_mutexattr_settype)
> +
> +versioned_symbol (libpthread, __new_pthread_mutexattr_setkind_np,
> +		  pthread_mutexattr_setkind_np, GLIBC_2_18);
> +versioned_symbol (libpthread, __new_pthread_mutexattr_settype,
> +		  pthread_mutexattr_settype, GLIBC_2_18);
> +versioned_symbol (libpthread, __pthread_mutexattr_settype_new,
> +		  __pthread_mutexattr_settype, GLIBC_2_18);
> +
> +#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_18) \
> +    || SHLIB_COMPAT (libpthread, GLIBC_2_1, GLIBC_2_18) \
> +
> +int
> +attribute_compat_text_section
> +__pthread_mutexattr_settype_old (pthread_mutexattr_t *attr, int kind)
> +{
> +  /* Force no elision for the old ambigious DEFAULT/NORMAL
> +     kind.  */
> +  if (kind == PTHREAD_MUTEX_DEFAULT)
> +    kind |= PTHREAD_MUTEX_NO_ELISION_NP;
> +  return pthread_mutexattr_settype_worker (attr, kind);
> +}
> +
> +
> +# if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_18)
> +weak_alias (__pthread_mutexattr_settype_old, __old_pthread_mutexattr_setkind_np)
> +strong_alias (__pthread_mutexattr_settype_old, __old_pthread_mutexattr_settype)
> +compat_symbol (libpthread,
> +	       __old_pthread_mutexattr_setkind_np,
> +	       pthread_mutexattr_setkind_np,
> +	       GLIBC_2_0);
> +compat_symbol (libpthread,
> +	       __old_pthread_mutexattr_settype,
> +	       __pthread_mutexattr_settype,
> +	       GLIBC_2_0);
> +# endif
> +# if SHLIB_COMPAT (libpthread, GLIBC_2_1, GLIBC_2_18)
> +compat_symbol (libpthread,
> +	       __pthread_mutexattr_settype_old,
> +	       pthread_mutexattr_settype,
> +	       GLIBC_2_1);
> +# endif
> +#endif
> diff --git a/nptl/sysdeps/pthread/pthread.h b/nptl/sysdeps/pthread/pthread.h
> index 485a865..adc259e 100644
> --- a/nptl/sysdeps/pthread/pthread.h
> +++ b/nptl/sysdeps/pthread/pthread.h
> @@ -46,6 +46,14 @@ enum
>    PTHREAD_MUTEX_ERRORCHECK_NP,
>    PTHREAD_MUTEX_ADAPTIVE_NP,
>  
> +#if defined __USE_UNIX98 || defined __USE_XOPEN2K8
> +  /* Before glibc 2.18 this used to be identical to TIMED_NP.
> +     Now with lock elision it's a new type that does not enable
> +     elision when set by pthread_mutexattr_settype(), to satisfy
> +     POSIX's deadlock requirement.  */
> +  PTHREAD_MUTEX_NORMAL,
> +#endif
> +

OK.

>    PTHREAD_MUTEX_ELISION_NP    = 1024,
>    PTHREAD_MUTEX_NO_ELISION_NP = 2048,
>    PTHREAD_MUTEX_UPGRADED_ELISION_NP = 4096,
> @@ -53,10 +61,9 @@ enum
>  
>  #if defined __USE_UNIX98 || defined __USE_XOPEN2K8
>    ,
> -  PTHREAD_MUTEX_NORMAL = PTHREAD_MUTEX_TIMED_NP,
>    PTHREAD_MUTEX_RECURSIVE = PTHREAD_MUTEX_RECURSIVE_NP,
>    PTHREAD_MUTEX_ERRORCHECK = PTHREAD_MUTEX_ERRORCHECK_NP,
> -  PTHREAD_MUTEX_DEFAULT = PTHREAD_MUTEX_NORMAL
> +  PTHREAD_MUTEX_DEFAULT = PTHREAD_MUTEX_TIMED_NP

OK.

>  #endif
>  #ifdef __USE_GNU
>    /* For compatibility.  */
> diff --git a/nptl/tst-mutex5.c b/nptl/tst-mutex5.c
> index f0c6374..aa6b748 100644
> --- a/nptl/tst-mutex5.c
> +++ b/nptl/tst-mutex5.c
> @@ -25,7 +25,7 @@
>  
>  
>  #ifndef TYPE
> -# define TYPE PTHREAD_MUTEX_NORMAL
> +# define TYPE PTHREAD_MUTEX_DEFAULT

OK.

>  #endif
>  
>  

Cheers,
Carlos.