[RFC] tunables: Add elision tunable

  Hi Stefan,

 >Please also change the order of the lines (in all elision-conf.c >files):
 >+# include <elf/dl-tunables.h>
 >+# define TUNABLE_NAMESPACE elision
 >
 >If TUNABLE_NAMESPACE is not defined before dl-tunables.h is included,
 >there will be a build error as TUNABLE_GET will require 5 instead of 
 >three arguments.

That's odd I did not get a build error on my tests on POWER. But you're 
right. Fixed on v3.

Thanks,
Rogerio
  

rcardoso <rcardoso@linux.vnet.ibm.com> writes:

> This patch adds several new tunables to control the behavior of
> elision on supported platforms.  This also disables elision
> by default on powerpc.
>
> This patch was initially proposed by Paul Murphy[1] but was
> "staled" because the framework have changed since the patch was
> originally proposed.
>
> [1] https://patchwork.sourceware.org/patch/10342/
>
> 2017-06-06  Rogerio A. Cardoso  <rcardoso@linux.vnet.ibm.com>,
> 	    Paul E. Murphy  <murphyp@linux.vnet.ibm.com>
>
> 	* elf/dl-tunables.list: Add elision parameters.
> 	* manual/tunables.texi: Add entries about elision tunable.
> 	* sysdeps/unix/sysv/linux/powerpc/elision-conf.c:
> 	Add callback functions to dynamically enable/disable elision.
> 	Add multiple callbacks functions to set elision parameters.
> 	* sysdeps/unix/sysv/linux/s390/elision-conf.c: Likewise.
> 	* sysdeps/unix/sysv/linux/x86/elision-conf.c: Likewise.

Reviewed-by: Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com>

I'm going to push it later today.

Thanks!
  

On Thursday 14 September 2017 08:00 PM, rcardoso wrote:
> Hi Stefan,
> 
>>Please also change the order of the lines (in all elision-conf.c >files):
>>+# include <elf/dl-tunables.h>
>>+# define TUNABLE_NAMESPACE elision
>>
>>If TUNABLE_NAMESPACE is not defined before dl-tunables.h is included,
>>there will be a build error as TUNABLE_GET will require 5 instead of
>>three arguments.
> 
> That's odd I did not get a build error on my tests on POWER. But you're
> right. Fixed on v3.
> 
> Thanks,
> Rogerio

Sorry, here's a very late review since I noticed the patch only today.
There are a couple of points below, specifically concerning elision
behaviour for setxid binaries and their children.

>  elf/dl-tunables.list                           | 34 ++++++++++++
>  manual/tunables.texi                           | 65 +++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/powerpc/elision-conf.c | 73 +++++++++++++++++++++++---
>  sysdeps/unix/sysv/linux/s390/elision-conf.c    | 70 ++++++++++++++++++++++--
>  sysdeps/unix/sysv/linux/x86/elision-conf.c     | 72 ++++++++++++++++++++++---
>  5 files changed, 297 insertions(+), 17 deletions(-)
> 
> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
> index c188c6a..77dfcb4 100644
> --- a/elf/dl-tunables.list
> +++ b/elf/dl-tunables.list
> @@ -96,4 +96,38 @@ glibc {
>        default: HWCAP_IMPORTANT
>        }
>    }
> +
> +  elision {
> +    enable {
> +      type: INT_32
> +      minval: 0
> +      maxval: 1
> +      security_level: SXID_IGNORE
> +    }
> +    skip_lock_busy {
> +      type: INT_32
> +      default: 3
> +      security_level: SXID_IGNORE
> +    }
> +    skip_lock_internal_abort {
> +      type: INT_32
> +      default: 3
> +      security_level: SXID_IGNORE
> +    }
> +    skip_lock_after_retries {
> +      type: INT_32
> +      default: 3
> +      security_level: SXID_IGNORE
> +    }
> +    tries {
> +      type: INT_32
> +      default: 3
> +      security_level: SXID_IGNORE
> +    }
> +    skip_trylock_internal_abort {
> +      type: INT_32
> +      default: 3
> +      security_level: SXID_IGNORE

SXID_IGNORE means that you don't want setxid binaries to read it but you
would like children of setxid binaries to read it.  Why is this use case
necessary to be supported?  We don't want to do this unless there is a
specific reason to allow it.  That is, set to SXID_ERASE unless there is
a very good reason to allow children of setxid process to read the
elision tunables.

> +    }
> +  }
>  }
> diff --git a/manual/tunables.texi b/manual/tunables.texi
> index 3c19567..5f660e0 100644
> --- a/manual/tunables.texi
> +++ b/manual/tunables.texi
> @@ -31,6 +31,7 @@ their own namespace.
>  @menu
>  * Tunable names::  The structure of a tunable name
>  * Memory Allocation Tunables::  Tunables in the memory allocation subsystem
> +* Elision Tunables::  Tunables in elision subsystem
>  * Hardware Capability Tunables::  Tunables that modify the hardware
>  				  capabilities seen by @theglibc{}
>  @end menu
> @@ -226,6 +227,70 @@ pre-fill the per-thread cache with.  The default, or when set to zero,
>  is no limit.
>  @end deftp
>  
> +@node Elision Tunables
> +@section Elision Tunables
> +@cindex elision tunables
> +@cindex tunables, elision
> +
> +@deftp {Tunable namespace} glibc.elision
> +Elision behavior can be modified by setting any of the following tunables in
> +the @code{elision} namespace:

Should this have a brief description of what elision is?  Maybe just a
single line to give context to what we're controlling.

> +@end deftp
> +
> +@deftp Tunable glibc.elision.enable
> +The @code{glibc.elision.enable} tunable enables lock elision if the feature is
> +supported by the hardware.  If elision is not supported by the hardware this
> +tunable has no effect.
> +
> +Elision tunables are supported for x86-64, PowerPC, and S390 architectures.
> +@end deftp
> +
> +@deftp Tunable glibc.elision.skip_lock_busy
> +The @code{glibc.elision.skip_lock_busy} tunable sets how many times to use a
> +non-transactional lock after a transactional failure has occurred because the
> +lock is already acquired.  Expressed in number of lock acquisition attempts.
> +
> +The default value of this tunable is @samp{3}.
> +@end deftp
> +
> +@deftp Tunable glibc.elision.skip_lock_internal_abort
> +The @code{glibc.elision.skip_lock_internal_abort} tunable sets how many times
> +to not attempt to use elision if a transaction aborted due to reasons other
> +than other threads' memory accesses.  Expressed in number of lock acquisition
> +attempts.
> +
> +The default value of this tunable is @samp{3}.
> +@end deftp
> +
> +@deftp Tunable glibc.elision.skip_lock_after_retries
> +The @code{glibc.elision.skip_lock_after_retries} tunable sets how my times to
> +try to elide a lock with transactions that only fail due other threads' memory
> +accesses, before falling back to regular lock.
> +Expressed in number of lock elision attempts.
> +
> +This tunable is not supported by x86.
> +
> +The default value of this tunable is @samp{3}.
> +@end deftp
> +
> +@deftp Tunable glibc.elision.tries
> +The @code{glibc.elision.tries} sets how many times we retry using elision if
> +there is chance for the transaction to finish execution (e.g., it wasn't aborted
> +due to the lock being already acquired).  This tunable is set to @samp{0} if
> +elision is not supported by the hardware to avoid retries.
> +
> +The default value of this tunable is @samp{3}.
> +@end deftp
> +
> +@deftp Tunable glibc.elision.skip_trylock_internal_abort
> +The @code{glibc.elision.skip_trylock_internal_abort} tunable sets how many times
> +to not attempt to use elision for trylocks if a transaction aborted due to
> +reasons other than other threads' memory accesses.  Expressed in number of try
> +lock attempts.
> +
> +The default value of this tunable is @samp{3}.
> +@end deftp
> +
>  @node Hardware Capability Tunables
>  @section Hardware Capability Tunables
>  @cindex hardware capability tunables
> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
> index f631f0a..4fd7741 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
> @@ -22,6 +22,11 @@
>  #include <unistd.h>
>  #include <dl-procinfo.h>
>  
> +#if HAVE_TUNABLES
> +# define TUNABLE_NAMESPACE elision
> +#endif
> +#include <elf/dl-tunables.h>
> +
>  /* Reasonable initial tuning values, may be revised in the future.
>     This is a conservative initial value.  */
>  
> @@ -50,7 +55,50 @@ struct elision_config __elision_aconf =
>     DEFAULT locks should be automatically use elision in pthread_mutex_lock().
>     Disabled for suid programs.  Only used when elision is available.  */
>  
> -int __pthread_force_elision attribute_hidden;
> +int __pthread_force_elision attribute_hidden = 0;
> +
> +#if HAVE_TUNABLES
> +static inline void
> +__always_inline
> +do_set_elision_enable (int32_t elision_enable)
> +{
> +  /* Enable elision if it's avaliable in hardware.  */
> +  if (elision_enable && !__libc_enable_secure)

If we set the tunable permissions correctly (i.e. the SXID_*) the
__libc_enable_secure check is unnecessary.  elision_enable will be set
according to the default, which is disabled.  You could replace the
check with a comment explaining why the check is unnecessary.

> +    __pthread_force_elision = (GLRO (dl_hwcap2)
> +			       & PPC_FEATURE2_HAS_HTM) ? 1 : 0;
> +}
> +
> +/* The pthread->elision_enable tunable is 0 or 1 indicating that elision
> +   should be disabled or enabled respectively.  The feature will only be used
> +   if it's supported by the hardware.  */
> +
> +void
> +TUNABLE_CALLBACK (set_elision_enable) (tunable_val_t *valp)
> +{
> +  int32_t elision_enable = (int32_t) valp->numval;
> +  do_set_elision_enable (elision_enable);
> +}
> +
> +#define TUNABLE_CALLBACK_FNDECL(__name, __type)			\
> +static inline void						\
> +__always_inline							\
> +do_set_elision_ ## __name (__type value)			\
> +{								\
> +  __elision_aconf.__name = value;				\
> +}								\
> +void								\
> +TUNABLE_CALLBACK (set_elision_ ## __name) (tunable_val_t *valp) \
> +{								\
> +  __type value = (__type) (valp)->numval;			\
> +  do_set_elision_ ## __name (value);				\
> +}
> +
> +TUNABLE_CALLBACK_FNDECL (skip_lock_busy, int32_t);
> +TUNABLE_CALLBACK_FNDECL (skip_lock_internal_abort, int32_t);
> +TUNABLE_CALLBACK_FNDECL (skip_lock_out_of_tbegin_retries, int32_t);
> +TUNABLE_CALLBACK_FNDECL (try_tbegin, int32_t);
> +TUNABLE_CALLBACK_FNDECL (skip_trylock_internal_abort, int32_t);
> +#endif
>  
>  /* Initialize elision.  */
>  
> @@ -59,13 +107,26 @@ elision_init (int argc __attribute__ ((unused)),
>  	      char **argv  __attribute__ ((unused)),
>  	      char **environ)
>  {
> -#ifdef ENABLE_LOCK_ELISION
> -  int elision_available = (GLRO (dl_hwcap2) & PPC_FEATURE2_HAS_HTM) ? 1 : 0;
> -  __pthread_force_elision = __libc_enable_secure ? 0 : elision_available;
> +#if HAVE_TUNABLES
> +  /* Elision depends on tunables and must be explicitly turned on by setting
> +     the appropriate tunable on a supported platform.  */
> +
> +  TUNABLE_GET (enable, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_enable));
> +  TUNABLE_GET (skip_lock_busy, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_skip_lock_busy));
> +  TUNABLE_GET (skip_lock_internal_abort, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_skip_lock_internal_abort));
> +  TUNABLE_GET (skip_lock_after_retries, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_skip_lock_out_of_tbegin_retries));
> +  TUNABLE_GET (tries, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_try_tbegin));
> +  TUNABLE_GET (skip_trylock_internal_abort, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_skip_trylock_internal_abort));
>  #endif
> +
>    if (!__pthread_force_elision)
> -    /* Disable elision on rwlocks.  */
> -    __elision_aconf.try_tbegin = 0;
> +    __elision_aconf.try_tbegin = 0; /* Disable elision on rwlocks.  */
>  }
>  
>  #ifdef SHARED
> diff --git a/sysdeps/unix/sysv/linux/s390/elision-conf.c b/sysdeps/unix/sysv/linux/s390/elision-conf.c
> index cc0fdef..6a57269 100644
> --- a/sysdeps/unix/sysv/linux/s390/elision-conf.c
> +++ b/sysdeps/unix/sysv/linux/s390/elision-conf.c
> @@ -22,6 +22,11 @@
>  #include <unistd.h>
>  #include <dl-procinfo.h>
>  
> +#if HAVE_TUNABLES
> +# define TUNABLE_NAMESPACE elision
> +#endif
> +#include <elf/dl-tunables.h>
> +
>  /* Reasonable initial tuning values, may be revised in the future.
>     This is a conservative initial value.  */
>  
> @@ -53,6 +58,48 @@ struct elision_config __elision_aconf =
>  
>  int __pthread_force_elision attribute_hidden = 0;
>  
> +#if HAVE_TUNABLES
> +static inline void
> +__always_inline
> +do_set_elision_enable (int32_t elision_enable)
> +{
> +  /* Enable elision if it's avaliable in hardware.  */
> +  if (elision_enable && !__libc_enable_secure)

Same comment about tunables protection applies here.
__libc_enable_secure check is unnecessary since elision_tunable will
only be set when !__libc_enable_secure.

> +    __pthread_force_elision = (GLRO (dl_hwcap) & HWCAP_S390_TE) ? 1 : 0;
> +}
> +
> +/* The pthread->elision_enable tunable is 0 or 1 indicating that elision
> +   should be disabled or enabled respectively.  The feature will only be used
> +   if it's supported by the hardware.  */
> +
> +void
> +TUNABLE_CALLBACK (set_elision_enable) (tunable_val_t *valp)
> +{
> +  int32_t elision_enable = (int32_t) valp->numval;
> +  do_set_elision_enable (elision_enable);
> +}
> +
> +#define TUNABLE_CALLBACK_FNDECL(__name, __type)			\
> +static inline void						\
> +__always_inline							\
> +do_set_elision_ ## __name (__type value)			\
> +{								\
> +  __elision_aconf.__name = value;				\
> +}								\
> +void								\
> +TUNABLE_CALLBACK (set_elision_ ## __name) (tunable_val_t *valp) \
> +{								\
> +  __type value = (__type) (valp)->numval;			\
> +  do_set_elision_ ## __name (value);				\
> +}
> +
> +TUNABLE_CALLBACK_FNDECL (skip_lock_busy, int32_t);
> +TUNABLE_CALLBACK_FNDECL (skip_lock_internal_abort, int32_t);
> +TUNABLE_CALLBACK_FNDECL (skip_lock_out_of_tbegin_retries, int32_t);
> +TUNABLE_CALLBACK_FNDECL (try_tbegin, int32_t);
> +TUNABLE_CALLBACK_FNDECL (skip_trylock_internal_abort, int32_t);
> +#endif
> +
>  /* Initialize elison.  */
>  
>  static void
> @@ -60,11 +107,26 @@ elision_init (int argc __attribute__ ((unused)),
>  	      char **argv  __attribute__ ((unused)),
>  	      char **environ)
>  {
> -  /* Set when the CPU and the kernel supports transactional execution.
> -     When false elision is never attempted.  */
> -  int elision_available = (GLRO (dl_hwcap) & HWCAP_S390_TE) ? 1 : 0;
> +#if HAVE_TUNABLES
> +  /* Elision depends on tunables and must be explicitly turned on by setting
> +     the appropriate tunable on a supported platform.  */
> +
> +  TUNABLE_GET (enable, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_enable));
> +  TUNABLE_GET (skip_lock_busy, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_skip_lock_busy));
> +  TUNABLE_GET (skip_lock_internal_abort, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_skip_lock_internal_abort));
> +  TUNABLE_GET (skip_lock_after_retries, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_skip_lock_out_of_tbegin_retries));
> +  TUNABLE_GET (tries, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_try_tbegin));
> +  TUNABLE_GET (skip_trylock_internal_abort, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_skip_trylock_internal_abort));
> +#endif
>  
> -  __pthread_force_elision = __libc_enable_secure ? 0 : elision_available;
> +  if (!__pthread_force_elision)
> +    __elision_aconf.try_tbegin = 0; /* Disable elision on rwlocks.  */
>  }
>  
>  #ifdef SHARED
> diff --git a/sysdeps/unix/sysv/linux/x86/elision-conf.c b/sysdeps/unix/sysv/linux/x86/elision-conf.c
> index 673b000..89a4a8e 100644
> --- a/sysdeps/unix/sysv/linux/x86/elision-conf.c
> +++ b/sysdeps/unix/sysv/linux/x86/elision-conf.c
> @@ -22,6 +22,11 @@
>  #include <elision-conf.h>
>  #include <unistd.h>
>  
> +#if HAVE_TUNABLES
> +# define TUNABLE_NAMESPACE elision
> +#endif
> +#include <elf/dl-tunables.h>
> +
>  /* Reasonable initial tuning values, may be revised in the future.
>     This is a conservative initial value.  */
>  
> @@ -48,21 +53,74 @@ struct elision_config __elision_aconf =
>     pthread_mutex_lock().  Disabled for suid programs.  Only used when elision
>     is available.  */
>  
> -int __pthread_force_elision attribute_hidden;
> +int __pthread_force_elision attribute_hidden = 0;
> +
> +#if HAVE_TUNABLES
> +static inline void
> +__always_inline
> +do_set_elision_enable (int32_t elision_enable)
> +{
> +  /* Enable elision if it's avaliable in hardware.  */
> +  if (elision_enable && !__libc_enable_secure)

Likewise, elision_enable is set only when !__libc_enable_secure, so the
check is unnecessary.

> +    __pthread_force_elision = HAS_CPU_FEATURE (RTM) ? 1 : 0;
> +}
> +
> +/* The pthread->elision_enable tunable is 0 or 1 indicating that elision
> +   should be disabled or enabled respectively.  The feature will only be used
> +   if it's supported by the hardware.  */
>  
> -/* Initialize elison.  */
> +void
> +TUNABLE_CALLBACK (set_elision_enable) (tunable_val_t *valp)
> +{
> +  int32_t elision_enable = (int32_t) valp->numval;
> +  do_set_elision_enable (elision_enable);
> +}
> +
> +#define TUNABLE_CALLBACK_FNDECL(__name, __type)			\
> +static inline void						\
> +__always_inline							\
> +do_set_elision_ ## __name (__type value)			\
> +{								\
> +  __elision_aconf.__name = value;				\
> +}								\
> +void								\
> +TUNABLE_CALLBACK (set_elision_ ## __name) (tunable_val_t *valp) \
> +{								\
> +  __type value = (__type) (valp)->numval;			\
> +  do_set_elision_ ## __name (value);				\
> +}
> +
> +TUNABLE_CALLBACK_FNDECL (skip_lock_busy, int32_t);
> +TUNABLE_CALLBACK_FNDECL (skip_lock_internal_abort, int32_t);
> +TUNABLE_CALLBACK_FNDECL (retry_try_xbegin, int32_t);
> +TUNABLE_CALLBACK_FNDECL (skip_trylock_internal_abort, int32_t);
> +#endif
> +
> +/* Initialize elision.  */
>  
>  static void
>  elision_init (int argc __attribute__ ((unused)),
>  	      char **argv  __attribute__ ((unused)),
>  	      char **environ)
>  {
> -  int elision_available = HAS_CPU_FEATURE (RTM);
> -#ifdef ENABLE_LOCK_ELISION
> -  __pthread_force_elision = __libc_enable_secure ? 0 : elision_available;
> +#if HAVE_TUNABLES
> +  /* Elision depends on tunables and must be explicitly turned on by setting
> +     the appropriate tunable on a supported platform.  */
> +
> +  TUNABLE_GET (enable, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_enable));
> +  TUNABLE_GET (skip_lock_busy, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_skip_lock_busy));
> +  TUNABLE_GET (skip_lock_internal_abort, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_skip_lock_internal_abort));
> +  TUNABLE_GET (tries, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_retry_try_xbegin));
> +  TUNABLE_GET (skip_trylock_internal_abort, int32_t,
> +	       TUNABLE_CALLBACK (set_elision_skip_trylock_internal_abort));
>  #endif
> -  if (!elision_available)
> -    __elision_aconf.retry_try_xbegin = 0; /* Disable elision on rwlocks */
> +
> +  if (!__pthread_force_elision)
> +    __elision_aconf.retry_try_xbegin = 0; /* Disable elision on rwlocks.  */
>  }
>  
>  #ifdef SHARED
>