| Message ID | 584704cb-644d-e06f-c1de-0bb76ca6c4cb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Are there any news for this? On 05/12/2017 01:13 PM, Stefan Liebler wrote: > On 05/11/2017 05:45 PM, Carlos O'Donell wrote: >> I am going to propose the following: >> >> * Always build with elision support. >> >> * Elision disabled by default at runtime. >> >> * Use tunables to allow processes to opt-in to transparent elision. >> >> The benefit is that the elision support doesn't bit-rot, and we keep >> it working, and that distributions can conservatively backport elision >> and allow users to test enablement on a per-process basis. > This is an improvement for users as they can simply compare performance > of their workload with or without lock elsion. > With (c) - see below - further tuning is possible. > >> >> The elision is enabled with: >> >> GLIBC_TUNABLE=glibc.elision.enable=1 >> >> The obvious set of patches are: >> >> (a) Split out some cleanups in this patch. >> (b) Always build with elision and us tunables to opt-in. >> (c) Extend tunables to allow modification of elision parameters >> (useful for upcoming rwlock elision re-enablement). >> >> I'm testing this on x86_64, ppc64le, and s390x. >> > I've applied this patch to test on s390x. > Some changes are needed in order to build it. > > Please remove the line "enable-lock-elision = @enable_lock_elision" > in config.make.in. This variable is used in > sysdeps/unix/sysv/linux/s390/Makefile, which has to be adjusted, too: > ifeq ($(enable-lock-elision),yes) > ... > endif > > The s390 configure check to test the availability of __builtin_tbegin > has to be adjusted: > diff --git a/sysdeps/s390/configure.ac b/sysdeps/s390/configure.ac > index 8a782e7..62bcfb9 100644 > --- a/sysdeps/s390/configure.ac > +++ b/sysdeps/s390/configure.ac > @@ -32,7 +32,7 @@ else > fi > rm -f conftest* ]) > > -if test "$enable_lock_elision" = yes && test > "$libc_cv_gcc_builtin_tbegin" = no ; then > +if test "$libc_cv_gcc_builtin_tbegin" = no ; then > critic_missing="$critic_missing The used GCC has no support for > __builtin_tbegin, which is needed for lock-elision on target S390." > fi > > Note: __builtin_tbegin is available on RHEL/SLES GCC 4.8.5. > > > Please remove '--enable-lock-elision=yes' in INSTALL, too. > > With these mentioned changes and the comments below, > I could build glibc and the testsuite runs without regressions. > > I've also used a small s390 specific test program to check wether a > mutex was elided or not: > ./prog > Lock was a normal lock! > > GLIBC_TUNABLES=glibc.elision.enable=0 ./prog > Lock was a normal lock! > > GLIBC_TUNABLES=glibc.elision.enable=1 ./prog > Lock was elided via a transaction! (nesting-depth=1) > > GLIBC_TUNABLES=glibc.elision.enable=1 ./prog_secure > Lock was a normal lock! > > GLIBC_TUNABLES=glibc.elision.enable=2 ./prog > Lock was a normal lock! > > GLIBC_TUNABLES=glibc.elision.enable=yes ./prog > Lock was a normal lock! > > >> Thoughts? >> >> diff --git a/config.h.in b/config.h.in >> index 2caa412..47b139e 100644 >> --- a/config.h.in >> +++ b/config.h.in >> @@ -147,9 +147,6 @@ >> /* Define if __stack_chk_guard canary should be randomized at program >> startup. */ >> #undef ENABLE_STACKGUARD_RANDOMIZE >> >> -/* Define if lock elision should be enabled by default. */ >> -#undef ENABLE_LOCK_ELISION >> - >> /* Package description. */ >> #undef PKGVERSION >> >> diff --git a/configure b/configure >> index 422482f..d3b68c1 100755 >> --- a/configure >> +++ b/configure >> @@ -677,7 +677,6 @@ enable_werror >> all_warnings >> force_install >> bindnow >> -enable_lock_elision >> hardcoded_path_in_tests >> enable_timezone_tools >> use_default_link >> @@ -766,7 +765,6 @@ enable_profile >> enable_timezone_tools >> enable_hardcoded_path_in_tests >> enable_stackguard_randomization >> -enable_lock_elision >> enable_add_ons >> enable_hidden_plt >> enable_bind_now >> @@ -1427,8 +1425,6 @@ Optional Features: >> --enable-stackguard-randomization >> initialize __stack_chk_guard canary with a >> random >> number at program start >> - --enable-lock-elision=yes/no >> - Enable lock elision for pthread mutexes by >> default >> --enable-add-ons[=DIRS...] >> configure and build add-ons in >> DIR1,DIR2,... search >> for add-ons if no parameter given >> @@ -3395,19 +3391,6 @@ if test "$enable_stackguard_randomize" = yes; then >> >> fi >> >> -# Check whether --enable-lock-elision was given. >> -if test "${enable_lock_elision+set}" = set; then : >> - enableval=$enable_lock_elision; enable_lock_elision=$enableval >> -else >> - enable_lock_elision=no >> -fi >> - >> - >> -if test "$enable_lock_elision" = yes ; then >> - $as_echo "#define ENABLE_LOCK_ELISION 1" >>confdefs.h >> - >> -fi >> - >> # Check whether --enable-add-ons was given. >> if test "${enable_add_ons+set}" = set; then : >> enableval=$enable_add_ons; >> diff --git a/configure.ac b/configure.ac >> index 7f43042..a970296 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -199,16 +199,6 @@ if test "$enable_stackguard_randomize" = yes; then >> AC_DEFINE(ENABLE_STACKGUARD_RANDOMIZE) >> fi >> >> -AC_ARG_ENABLE([lock-elision], >> - AC_HELP_STRING([--enable-lock-elision[=yes/no]], >> - [Enable lock elision for pthread mutexes by default]), >> - [enable_lock_elision=$enableval], >> - [enable_lock_elision=no]) >> -AC_SUBST(enable_lock_elision) >> -if test "$enable_lock_elision" = yes ; then >> - AC_DEFINE(ENABLE_LOCK_ELISION) >> -fi >> - >> dnl Generic infrastructure for drop-in additions to libc. >> AC_ARG_ENABLE([add-ons], >> AC_HELP_STRING([--enable-add-ons@<:@=DIRS...@:>@], >> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h >> index f33adfb..3dad518 100644 >> --- a/elf/dl-tunables.h >> +++ b/elf/dl-tunables.h >> @@ -18,8 +18,8 @@ >> License along with the GNU C Library; if not, see >> <http://www.gnu.org/licenses/>. */ >> >> -#ifndef _TUNABLES_H_ >> -#define _TUNABLES_H_ >> +#ifndef _DL_TUNABLES_H_ >> +#define _DL_TUNABLES_H_ >> >> #if !HAVE_TUNABLES >> static inline void >> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list >> index b9f1488..97062f9 100644 >> --- a/elf/dl-tunables.list >> +++ b/elf/dl-tunables.list >> @@ -77,4 +77,12 @@ glibc { >> security_level: SXID_IGNORE >> } >> } >> + elision { >> + enable { >> + type: INT_32 >> + minval: 0 >> + maxval: 1 >> + security_level: SXID_IGNORE >> + } >> + } >> } >> diff --git a/nptl/Makefile b/nptl/Makefile >> index 6d48c0c..a720612 100644 >> --- a/nptl/Makefile >> +++ b/nptl/Makefile >> @@ -704,6 +704,10 @@ $(objpfx)tst-oddstacklimit.out: >> $(objpfx)tst-oddstacklimit $(objpfx)tst-basic1 >> $(evaluate-test) >> endif >> >> +# Disable elision for tst-mutex8 so it can verify error case for >> +# destroying a mutex. >> +tst-mutex8-ENV = GLIBC_TUNABLES=glibc.elision.enable=0 >> + >> # The tests here better do not run in parallel >> ifneq ($(filter %tests,$(MAKECMDGOALS)),) >> .NOTPARALLEL: >> diff --git a/nptl/tst-mutex8.c b/nptl/tst-mutex8.c >> index 1d288d2..ef59db5 100644 >> --- a/nptl/tst-mutex8.c >> +++ b/nptl/tst-mutex8.c >> @@ -127,9 +127,8 @@ check_type (const char *mas, pthread_mutexattr_t *ma) >> return 1; >> } >> >> - /* Elided mutexes don't fail destroy. If elision is not explicitly >> disabled >> - we don't know, so can also not check this. */ >> -#ifndef ENABLE_LOCK_ELISION >> + /* Elided mutexes don't fail destroy, but this test is run with >> + elision disabled so we can test them. */ >> e = pthread_mutex_destroy (m); >> if (e == 0) >> { >> @@ -142,7 +141,6 @@ check_type (const char *mas, pthread_mutexattr_t *ma) >> mas); >> return 1; >> } >> -#endif >> >> if (pthread_mutex_unlock (m) != 0) >> { >> @@ -157,7 +155,6 @@ check_type (const char *mas, pthread_mutexattr_t *ma) >> } >> >> /* Elided mutexes don't fail destroy. */ >> -#ifndef ENABLE_LOCK_ELISION >> e = pthread_mutex_destroy (m); >> if (e == 0) >> { >> @@ -171,7 +168,6 @@ mutex_destroy of self-trylocked mutex did not >> return EBUSY %s\n", >> mas); >> return 1; >> } >> -#endif >> >> if (pthread_mutex_unlock (m) != 0) >> { >> @@ -207,7 +203,6 @@ mutex_destroy of self-trylocked mutex did not >> return EBUSY %s\n", >> } >> >> /* Elided mutexes don't fail destroy. */ >> -#ifndef ENABLE_LOCK_ELISION >> e = pthread_mutex_destroy (m); >> if (e == 0) >> { >> @@ -220,7 +215,6 @@ mutex_destroy of self-trylocked mutex did not >> return EBUSY %s\n", >> mutex_destroy of condvar-used mutex did not return EBUSY for %s\n", >> mas); >> return 1; >> } >> -#endif >> >> done = true; >> if (pthread_cond_signal (&c) != 0) >> @@ -280,7 +274,6 @@ mutex_destroy of condvar-used mutex did not return >> EBUSY for %s\n", mas); >> } >> >> /* Elided mutexes don't fail destroy. */ >> -#ifndef ENABLE_LOCK_ELISION >> e = pthread_mutex_destroy (m); >> if (e == 0) >> { >> @@ -295,7 +288,6 @@ mutex_destroy of condvar-used mutex did not return >> EBUSY for %s\n", mas); >> mas); >> return 1; >> } >> -#endif >> >> if (pthread_cancel (th) != 0) >> { >> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk >> index 601240a..6dba7e1 100644 >> --- a/scripts/gen-tunables.awk >> +++ b/scripts/gen-tunables.awk >> @@ -122,9 +122,9 @@ END { >> } >> >> print "/* AUTOGENERATED by gen-tunables.awk. */" >> - print "#ifndef _TUNABLES_H_" >> + print "#ifndef _DL_TUNABLES_H_" >> print "# error \"Do not include this file directly.\"" >> - print "# error \"Include tunables.h instead.\"" >> + print "# error \"Include dl-tunables.h instead.\"" >> print "#endif" >> >> # Now, the enum names >> diff --git a/sysdeps/powerpc/nptl/elide.h b/sysdeps/powerpc/nptl/elide.h >> index 1c42814..06986cc 100644 >> --- a/sysdeps/powerpc/nptl/elide.h >> +++ b/sysdeps/powerpc/nptl/elide.h >> @@ -19,7 +19,6 @@ >> #ifndef ELIDE_PPC_H >> # define ELIDE_PPC_H >> >> -#ifdef ENABLE_LOCK_ELISION >> # include <htm.h> >> # include <elision-conf.h> >> >> @@ -114,12 +113,4 @@ __elide_unlock (int is_lock_free) >> # define ELIDE_UNLOCK(is_lock_free) \ >> __elide_unlock (is_lock_free) >> >> -# else >> - >> -# define ELIDE_LOCK(adapt_count, is_lock_free) 0 >> -# define ELIDE_TRYLOCK(adapt_count, is_lock_free, write) 0 >> -# define ELIDE_UNLOCK(is_lock_free) 0 >> - >> -#endif /* ENABLE_LOCK_ELISION */ >> - >> #endif >> diff --git a/sysdeps/powerpc/powerpc32/sysdep.h >> b/sysdeps/powerpc/powerpc32/sysdep.h >> index f92ab2c..0ed83d3 100644 >> --- a/sysdeps/powerpc/powerpc32/sysdep.h >> +++ b/sysdeps/powerpc/powerpc32/sysdep.h >> @@ -88,7 +88,7 @@ GOT_LABEL: ; \ >> cfi_endproc; \ >> ASM_SIZE_DIRECTIVE(name) >> >> -#if ! IS_IN(rtld) && defined (ENABLE_LOCK_ELISION) >> +#if ! IS_IN(rtld) >> # define ABORT_TRANSACTION \ >> cmpwi 2,0; \ >> beq 1f; \ >> diff --git a/sysdeps/powerpc/powerpc64/sysdep.h >> b/sysdeps/powerpc/powerpc64/sysdep.h >> index db7c1d7..79a2697 100644 >> --- a/sysdeps/powerpc/powerpc64/sysdep.h >> +++ b/sysdeps/powerpc/powerpc64/sysdep.h >> @@ -272,7 +272,7 @@ LT_LABELSUFFIX(name,_name_end): ; \ >> TRACEBACK_MASK(name,mask) \ >> END_2(name) >> >> -#if !IS_IN(rtld) && defined (ENABLE_LOCK_ELISION) >> +#if !IS_IN(rtld) >> # define ABORT_TRANSACTION \ >> cmpdi 13,0; \ >> beq 1f; \ >> diff --git a/sysdeps/powerpc/sysdep.h b/sysdeps/powerpc/sysdep.h >> index f07b959..d1a9bd9 100644 >> --- a/sysdeps/powerpc/sysdep.h >> +++ b/sysdeps/powerpc/sysdep.h >> @@ -21,10 +21,8 @@ >> */ >> #define _SYSDEPS_SYSDEP_H 1 >> #include <bits/hwcap.h> >> -#ifdef ENABLE_LOCK_ELISION >> #include <tls.h> >> #include <htm.h> >> -#endif >> >> #define PPC_FEATURE_970 (PPC_FEATURE_POWER4 + PPC_FEATURE_HAS_ALTIVEC) >> >> @@ -176,7 +174,7 @@ >> we abort transaction just before syscalls. >> >> [1] Documentation/powerpc/transactional_memory.txt [Syscalls] */ >> -#if !IS_IN(rtld) && defined (ENABLE_LOCK_ELISION) >> +#if !IS_IN(rtld) >> # define ABORT_TRANSACTION \ >> ({ \ >> if (THREAD_GET_TM_CAPABLE ()) \ >> diff --git a/sysdeps/s390/nptl/bits/pthreadtypes.h >> b/sysdeps/s390/nptl/bits/pthreadtypes.h >> index 48ffdb4..88d21f0 100644 >> --- a/sysdeps/s390/nptl/bits/pthreadtypes.h >> +++ b/sysdeps/s390/nptl/bits/pthreadtypes.h >> @@ -89,37 +89,25 @@ typedef union >> binary compatibility with static initializers. */ >> int __kind; >> #if __WORDSIZE == 64 >> -# ifdef ENABLE_LOCK_ELISION >> short __spins; >> short __elision; >> /* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */ >> -# define __PTHREAD_SPINS 0, 0 >> -# else >> - int __spins; >> - /* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */ >> -# define __PTHREAD_SPINS 0 >> -# endif >> +# define __PTHREAD_SPINS 0, 0 >> __pthread_list_t __list; >> # define __PTHREAD_MUTEX_HAVE_PREV 1 >> #else >> unsigned int __nusers; >> __extension__ union >> { >> -# ifdef ENABLE_LOCK_ELISION >> struct >> { >> short __espins; >> short __elision; >> } _d; >> -# define __spins _d.__espins >> -# define __elision _d.__elision >> - /* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */ >> -# define __PTHREAD_SPINS { 0, 0 } >> -# else >> - int __spins; >> - /* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */ >> -# define __PTHREAD_SPINS 0 >> -# endif >> +# define __spins _d.__espins >> +# define __elision _d.__elision >> + /* Mutex __spins initializer used by >> PTHREAD_MUTEX_INITIALIZER. */ >> +# define __PTHREAD_SPINS { 0, 0 } >> __pthread_slist_t __list; >> }; >> #endif > Adhemerval's commit "Move shared pthread definitions to common headers" > consolidates the pthradtypes.h files. > Thus sysdeps/s390/nptl/bits/pthreadtypes.h does not exist anymore. > I've adjusted sysdeps/s390/nptl/bits/pthreadtypes-arch.h for my test: > diff --git a/sysdeps/s390/nptl/bits/pthreadtypes-arch.h > b/sysdeps/s390/nptl/bits/pthreadtypes-arch.h > index 3a9ac57..db7195a 100644 > --- a/sysdeps/s390/nptl/bits/pthreadtypes-arch.h > +++ b/sysdeps/s390/nptl/bits/pthreadtypes-arch.h > @@ -40,11 +40,7 @@ > /* Definitions for internal mutex struct. */ > #define __PTHREAD_COMPAT_PADDING_MID > #define __PTHREAD_COMPAT_PADDING_END > -#ifdef ENABLE_LOCK_ELISION > #define __PTHREAD_MUTEX_LOCK_ELISION 1 > -#else > -#define __PTHREAD_MUTEX_LOCK_ELISION 0 > -#endif > > #define __LOCK_ALIGNMENT > #define __ONCE_ALIGNMENT > > > >> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c >> b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c >> index f631f0a..a767a0c 100644 >> --- a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c >> +++ b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c >> @@ -21,6 +21,7 @@ >> #include <elision-conf.h> >> #include <unistd.h> >> #include <dl-procinfo.h> >> +#include <elf/dl-tunables.h> >> >> /* Reasonable initial tuning values, may be revised in the future. >> This is a conservative initial value. */ >> @@ -54,18 +55,47 @@ int __pthread_force_elision attribute_hidden; >> >> /* Initialize elision. */ >> >> +static inline void >> +do_set_elision_enable (bool elision_enable) >> +{ >> + /* If elision is not enabled, or we are in a secure mode, >> + make sure elision is never used... */ >> + if (!elision_enable || __libc_enable_secure) >> + { >> + __pthread_force_elision = 0; >> + return; >> + } >> + >> + /* ... otherwise enable elision based on hardare availability. */ >> + __pthread_force_elision = (GLRO (dl_hwcap2) >> + & PPC_FEATURE2_HAS_HTM) ? 1 : 0; >> +} >> + >> +#if HAVE_TUNABLES >> +/* The elision->enable tunable is either 0 or 1 to indicate that >> + elision should be disabled or enabled respectively. Availability at >> + the hardware level will still dictate if the feature is used. */ >> +void >> +DL_TUNABLE_CALLBACK (set_elision_enable) (tunable_val_t *elision_enable) > tunable_val_t *valp instead of tunable_val_t *elision_enable. > >> +{ >> + int32_t elision_enable = (int32_t) valp->numval; >> + do_set_elision_enable (elision_enable ? true : false); >> +} >> +#endif >> + >> static void >> 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 to enable and tune elision at >> runtime. */ >> +# define TUNABLE_NAMESPACE elision >> + TUNABLE_SET_VAL_WITH_CALLBACK (enable, NULL, set_elision_enable); >> +#else >> + /* Disable elision when built without tunables. */ >> + do_set_elision_enable (false); >> #endif >> - if (!__pthread_force_elision) >> - /* Disable elision on rwlocks. */ >> - __elision_aconf.try_tbegin = 0; >> } >> >> #ifdef SHARED >> diff --git a/sysdeps/unix/sysv/linux/powerpc/force-elision.h >> b/sysdeps/unix/sysv/linux/powerpc/force-elision.h >> index 318f791..d1feeeb 100644 >> --- a/sysdeps/unix/sysv/linux/powerpc/force-elision.h >> +++ b/sysdeps/unix/sysv/linux/powerpc/force-elision.h >> @@ -16,7 +16,6 @@ >> License along with the GNU C Library; if not, see >> <http://www.gnu.org/licenses/>. */ >> >> -#ifdef ENABLE_LOCK_ELISION >> /* Automatically enable elision for existing user lock kinds. */ >> #define FORCE_ELISION(m, s) \ >> if (__pthread_force_elision \ >> @@ -25,4 +24,3 @@ >> mutex->__data.__kind |= PTHREAD_MUTEX_ELISION_NP; \ >> s; \ >> } >> -#endif >> diff --git a/sysdeps/unix/sysv/linux/s390/elision-conf.c >> b/sysdeps/unix/sysv/linux/s390/elision-conf.c >> index cc0fdef..c6d9962 100644 >> --- a/sysdeps/unix/sysv/linux/s390/elision-conf.c >> +++ b/sysdeps/unix/sysv/linux/s390/elision-conf.c >> @@ -21,6 +21,7 @@ >> #include <elision-conf.h> >> #include <unistd.h> >> #include <dl-procinfo.h> >> +#include <elf/dl-tunables.h> >> >> /* Reasonable initial tuning values, may be revised in the future. >> This is a conservative initial value. */ >> @@ -55,16 +56,46 @@ int __pthread_force_elision attribute_hidden = 0; >> >> /* Initialize elison. */ >> >> +static inline void >> +do_set_elision_enable (bool elision_enable) >> +{ >> + /* If elision is not enabled, or we are in a secure mode, >> + make sure elision is never used... */ >> + if (!elision_enable || __libc_enable_secure) >> + { >> + __pthread_force_elision = 0; >> + return; >> + } >> + >> + /* ... otherwise enable elision based on hardare availability. */ >> + __pthread_force_elision = (GLRO (dl_hwcap) & HWCAP_S390_TE) ? 1 : 0; >> +} >> + >> +#if HAVE_TUNABLES >> +/* The elision->enable tunable is either 0 or 1 to indicate that >> + elision should be disabled or enabled respectively. Availability at >> + the hardware level will still dictate if the feature is used. */ >> +void >> +DL_TUNABLE_CALLBACK (set_elision_enable) (tunable_val_t *elision_enable) > tunable_val_t *valp instead of tunable_val_t *elision_enable. > >> +{ >> + int32_t elision_enable = (int32_t) valp->numval; >> + do_set_elision_enable (elision_enable ? true : false); >> +} >> +#endif >> + >> static void >> 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; >> - >> - __pthread_force_elision = __libc_enable_secure ? 0 : >> elision_available; >> +#if HAVE_TUNABLES >> + /* Elision depends on tunables to enable and tune elision at >> runtime. */ >> +# define TUNABLE_NAMESPACE elision >> + TUNABLE_SET_VAL_WITH_CALLBACK (enable, NULL, set_elision_enable); >> +#else >> + /* Disable elision when built without tunables. */ >> + do_set_elision_enable (false); >> +#endif >> } >> >> #ifdef SHARED >> diff --git a/sysdeps/unix/sysv/linux/s390/elision-conf.h >> b/sysdeps/unix/sysv/linux/s390/elision-conf.h >> index 3143f3b..32f0ed3 100644 >> --- a/sysdeps/unix/sysv/linux/s390/elision-conf.h >> +++ b/sysdeps/unix/sysv/linux/s390/elision-conf.h >> @@ -15,7 +15,6 @@ >> You should have received a copy of the GNU Lesser General Public >> License along with the GNU C Library; if not, see >> <http://www.gnu.org/licenses/>. */ >> -#ifdef ENABLE_LOCK_ELISION >> #ifndef _ELISION_CONF_H >> #define _ELISION_CONF_H 1 >> >> @@ -41,4 +40,3 @@ extern int __pthread_force_elision attribute_hidden; >> #define HAVE_ELISION 1 >> >> #endif >> -#endif >> diff --git a/sysdeps/unix/sysv/linux/s390/force-elision.h >> b/sysdeps/unix/sysv/linux/s390/force-elision.h >> index 3ae3bcd..8e1e33e 100644 >> --- a/sysdeps/unix/sysv/linux/s390/force-elision.h >> +++ b/sysdeps/unix/sysv/linux/s390/force-elision.h >> @@ -16,7 +16,6 @@ >> License along with the GNU C Library; if not, see >> <http://www.gnu.org/licenses/>. */ >> >> -#ifdef ENABLE_LOCK_ELISION >> /* Automatically enable elision for existing user lock kinds. */ >> #define FORCE_ELISION(m, s) \ >> if (__pthread_force_elision \ >> @@ -25,4 +24,3 @@ >> mutex->__data.__kind |= PTHREAD_MUTEX_ELISION_NP; \ >> s; \ >> } >> -#endif >> diff --git a/sysdeps/unix/sysv/linux/s390/lowlevellock.h >> b/sysdeps/unix/sysv/linux/s390/lowlevellock.h >> index 604137f..48f87a8 100644 >> --- a/sysdeps/unix/sysv/linux/s390/lowlevellock.h >> +++ b/sysdeps/unix/sysv/linux/s390/lowlevellock.h >> @@ -22,7 +22,6 @@ >> #include <sysdeps/nptl/lowlevellock.h> >> >> /* Transactional lock elision definitions. */ >> -# ifdef ENABLE_LOCK_ELISION >> extern int __lll_timedlock_elision >> (int *futex, short *adapt_count, const struct timespec *timeout, >> int private) >> attribute_hidden; >> @@ -45,6 +44,5 @@ extern int __lll_trylock_elision(int *futex, short >> *adapt_count) >> __lll_unlock_elision (&(futex), &(adapt_count), private) >> # define lll_trylock_elision(futex, adapt_count) \ > Can you adjust the occurences of "# define" within the old #ifdef > ENABLE_LOCK_ELISON block to "# define". > >> __lll_trylock_elision(&(futex), &(adapt_count)) >> -# endif /* ENABLE_LOCK_ELISION */ >> >> #endif /* lowlevellock.h */ >> diff --git a/sysdeps/unix/sysv/linux/x86/elision-conf.c >> b/sysdeps/unix/sysv/linux/x86/elision-conf.c >> index 673b000..7d3d8fd 100644 >> --- a/sysdeps/unix/sysv/linux/x86/elision-conf.c >> +++ b/sysdeps/unix/sysv/linux/x86/elision-conf.c >> @@ -21,6 +21,7 @@ >> #include <init-arch.h> >> #include <elision-conf.h> >> #include <unistd.h> >> +#include <elf/dl-tunables.h> >> >> /* Reasonable initial tuning values, may be revised in the future. >> This is a conservative initial value. */ >> @@ -52,17 +53,46 @@ int __pthread_force_elision attribute_hidden; >> >> /* Initialize elison. */ >> >> +static inline void >> +do_set_elision_enable (bool elision_enable) >> +{ >> + /* If elision is not enabled, or we are in a secure mode, >> + make sure elision is never used... */ >> + if (!elision_enable || __libc_enable_secure) >> + { >> + __pthread_force_elision = 0; >> + return; >> + } >> + >> + /* ... otherwise enable elision based on hardare availability. */ >> + __pthread_force_elision = HAS_CPU_FEATURE (RTM); >> +} >> + >> +#if HAVE_TUNABLES >> +/* The elision->enable tunable is either 0 or 1 to indicate that >> + elision should be disabled or enabled respectively. Availability at >> + the hardware level will still dictate if the feature is used. */ >> +void >> +DL_TUNABLE_CALLBACK (set_elision_enable) (tunable_val_t *valp) >> +{ >> + int32_t elision_enable = (int32_t) valp->numval; >> + do_set_elision_enable (elision_enable ? true : false); >> +} >> +#endif >> + >> 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 to enable and tune elision at >> runtime. */ >> +# define TUNABLE_NAMESPACE elision >> + TUNABLE_SET_VAL_WITH_CALLBACK (enable, NULL, set_elision_enable); >> +#else >> + /* Disable elision when built without tunables. */ >> + do_set_elision_enable (false); >> #endif >> - if (!elision_available) >> - __elision_aconf.retry_try_xbegin = 0; /* Disable elision on >> rwlocks */ >> } >> >> #ifdef SHARED >> --- >> >
diff --git a/sysdeps/s390/configure.ac b/sysdeps/s390/configure.ac index 8a782e7..62bcfb9 100644 --- a/sysdeps/s390/configure.ac +++ b/sysdeps/s390/configure.ac @@ -32,7 +32,7 @@ else fi rm -f conftest* ]) -if test "$enable_lock_elision" = yes && test "$libc_cv_gcc_builtin_tbegin" = no ; then +if test "$libc_cv_gcc_builtin_tbegin" = no ; then critic_missing="$critic_missing The used GCC has no support for __builtin_tbegin, which is needed for lock-elision on target S390." fi Note: __builtin_tbegin is available on RHEL/SLES GCC 4.8.5. Please remove '--enable-lock-elision=yes' in INSTALL, too. With these mentioned changes and the comments below, I could build glibc and the testsuite runs without regressions. I've also used a small s390 specific test program to check wether a mutex was elided or not: ./prog Lock was a normal lock! GLIBC_TUNABLES=glibc.elision.enable=0 ./prog Lock was a normal lock! GLIBC_TUNABLES=glibc.elision.enable=1 ./prog Lock was elided via a transaction! (nesting-depth=1) GLIBC_TUNABLES=glibc.elision.enable=1 ./prog_secure Lock was a normal lock! GLIBC_TUNABLES=glibc.elision.enable=2 ./prog Lock was a normal lock! GLIBC_TUNABLES=glibc.elision.enable=yes ./prog Lock was a normal lock! > Thoughts? > > diff --git a/config.h.in b/config.h.in > index 2caa412..47b139e 100644 > --- a/config.h.in > +++ b/config.h.in > @@ -147,9 +147,6 @@ > /* Define if __stack_chk_guard canary should be randomized at program startup. */ > #undef ENABLE_STACKGUARD_RANDOMIZE > > -/* Define if lock elision should be enabled by default. */ > -#undef ENABLE_LOCK_ELISION > - > /* Package description. */ > #undef PKGVERSION > > diff --git a/configure b/configure > index 422482f..d3b68c1 100755 > --- a/configure > +++ b/configure > @@ -677,7 +677,6 @@ enable_werror > all_warnings > force_install > bindnow > -enable_lock_elision > hardcoded_path_in_tests > enable_timezone_tools > use_default_link > @@ -766,7 +765,6 @@ enable_profile > enable_timezone_tools > enable_hardcoded_path_in_tests > enable_stackguard_randomization > -enable_lock_elision > enable_add_ons > enable_hidden_plt > enable_bind_now > @@ -1427,8 +1425,6 @@ Optional Features: > --enable-stackguard-randomization > initialize __stack_chk_guard canary with a random > number at program start > - --enable-lock-elision=yes/no > - Enable lock elision for pthread mutexes by default > --enable-add-ons[=DIRS...] > configure and build add-ons in DIR1,DIR2,... search > for add-ons if no parameter given > @@ -3395,19 +3391,6 @@ if test "$enable_stackguard_randomize" = yes; then > > fi > > -# Check whether --enable-lock-elision was given. > -if test "${enable_lock_elision+set}" = set; then : > - enableval=$enable_lock_elision; enable_lock_elision=$enableval > -else > - enable_lock_elision=no > -fi > - > - > -if test "$enable_lock_elision" = yes ; then > - $as_echo "#define ENABLE_LOCK_ELISION 1" >>confdefs.h > - > -fi > - > # Check whether --enable-add-ons was given. > if test "${enable_add_ons+set}" = set; then : > enableval=$enable_add_ons; > diff --git a/configure.ac b/configure.ac > index 7f43042..a970296 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -199,16 +199,6 @@ if test "$enable_stackguard_randomize" = yes; then > AC_DEFINE(ENABLE_STACKGUARD_RANDOMIZE) > fi > > -AC_ARG_ENABLE([lock-elision], > - AC_HELP_STRING([--enable-lock-elision[=yes/no]], > - [Enable lock elision for pthread mutexes by default]), > - [enable_lock_elision=$enableval], > - [enable_lock_elision=no]) > -AC_SUBST(enable_lock_elision) > -if test "$enable_lock_elision" = yes ; then > - AC_DEFINE(ENABLE_LOCK_ELISION) > -fi > - > dnl Generic infrastructure for drop-in additions to libc. > AC_ARG_ENABLE([add-ons], > AC_HELP_STRING([--enable-add-ons@<:@=DIRS...@:>@], > diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h > index f33adfb..3dad518 100644 > --- a/elf/dl-tunables.h > +++ b/elf/dl-tunables.h > @@ -18,8 +18,8 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > -#ifndef _TUNABLES_H_ > -#define _TUNABLES_H_ > +#ifndef _DL_TUNABLES_H_ > +#define _DL_TUNABLES_H_ > > #if !HAVE_TUNABLES > static inline void > diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list > index b9f1488..97062f9 100644 > --- a/elf/dl-tunables.list > +++ b/elf/dl-tunables.list > @@ -77,4 +77,12 @@ glibc { > security_level: SXID_IGNORE > } > } > + elision { > + enable { > + type: INT_32 > + minval: 0 > + maxval: 1 > + security_level: SXID_IGNORE > + } > + } > } > diff --git a/nptl/Makefile b/nptl/Makefile > index 6d48c0c..a720612 100644 > --- a/nptl/Makefile > +++ b/nptl/Makefile > @@ -704,6 +704,10 @@ $(objpfx)tst-oddstacklimit.out: $(objpfx)tst-oddstacklimit $(objpfx)tst-basic1 > $(evaluate-test) > endif > > +# Disable elision for tst-mutex8 so it can verify error case for > +# destroying a mutex. > +tst-mutex8-ENV = GLIBC_TUNABLES=glibc.elision.enable=0 > + > # The tests here better do not run in parallel > ifneq ($(filter %tests,$(MAKECMDGOALS)),) > .NOTPARALLEL: > diff --git a/nptl/tst-mutex8.c b/nptl/tst-mutex8.c > index 1d288d2..ef59db5 100644 > --- a/nptl/tst-mutex8.c > +++ b/nptl/tst-mutex8.c > @@ -127,9 +127,8 @@ check_type (const char *mas, pthread_mutexattr_t *ma) > return 1; > } > > - /* Elided mutexes don't fail destroy. If elision is not explicitly disabled > - we don't know, so can also not check this. */ > -#ifndef ENABLE_LOCK_ELISION > + /* Elided mutexes don't fail destroy, but this test is run with > + elision disabled so we can test them. */ > e = pthread_mutex_destroy (m); > if (e == 0) > { > @@ -142,7 +141,6 @@ check_type (const char *mas, pthread_mutexattr_t *ma) > mas); > return 1; > } > -#endif > > if (pthread_mutex_unlock (m) != 0) > { > @@ -157,7 +155,6 @@ check_type (const char *mas, pthread_mutexattr_t *ma) > } > > /* Elided mutexes don't fail destroy. */ > -#ifndef ENABLE_LOCK_ELISION > e = pthread_mutex_destroy (m); > if (e == 0) > { > @@ -171,7 +168,6 @@ mutex_destroy of self-trylocked mutex did not return EBUSY %s\n", > mas); > return 1; > } > -#endif > > if (pthread_mutex_unlock (m) != 0) > { > @@ -207,7 +203,6 @@ mutex_destroy of self-trylocked mutex did not return EBUSY %s\n", > } > > /* Elided mutexes don't fail destroy. */ > -#ifndef ENABLE_LOCK_ELISION > e = pthread_mutex_destroy (m); > if (e == 0) > { > @@ -220,7 +215,6 @@ mutex_destroy of self-trylocked mutex did not return EBUSY %s\n", > mutex_destroy of condvar-used mutex did not return EBUSY for %s\n", mas); > return 1; > } > -#endif > > done = true; > if (pthread_cond_signal (&c) != 0) > @@ -280,7 +274,6 @@ mutex_destroy of condvar-used mutex did not return EBUSY for %s\n", mas); > } > > /* Elided mutexes don't fail destroy. */ > -#ifndef ENABLE_LOCK_ELISION > e = pthread_mutex_destroy (m); > if (e == 0) > { > @@ -295,7 +288,6 @@ mutex_destroy of condvar-used mutex did not return EBUSY for %s\n", mas); > mas); > return 1; > } > -#endif > > if (pthread_cancel (th) != 0) > { > diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk > index 601240a..6dba7e1 100644 > --- a/scripts/gen-tunables.awk > +++ b/scripts/gen-tunables.awk > @@ -122,9 +122,9 @@ END { > } > > print "/* AUTOGENERATED by gen-tunables.awk. */" > - print "#ifndef _TUNABLES_H_" > + print "#ifndef _DL_TUNABLES_H_" > print "# error \"Do not include this file directly.\"" > - print "# error \"Include tunables.h instead.\"" > + print "# error \"Include dl-tunables.h instead.\"" > print "#endif" > > # Now, the enum names > diff --git a/sysdeps/powerpc/nptl/elide.h b/sysdeps/powerpc/nptl/elide.h > index 1c42814..06986cc 100644 > --- a/sysdeps/powerpc/nptl/elide.h > +++ b/sysdeps/powerpc/nptl/elide.h > @@ -19,7 +19,6 @@ > #ifndef ELIDE_PPC_H > # define ELIDE_PPC_H > > -#ifdef ENABLE_LOCK_ELISION > # include <htm.h> > # include <elision-conf.h> > > @@ -114,12 +113,4 @@ __elide_unlock (int is_lock_free) > # define ELIDE_UNLOCK(is_lock_free) \ > __elide_unlock (is_lock_free) > > -# else > - > -# define ELIDE_LOCK(adapt_count, is_lock_free) 0 > -# define ELIDE_TRYLOCK(adapt_count, is_lock_free, write) 0 > -# define ELIDE_UNLOCK(is_lock_free) 0 > - > -#endif /* ENABLE_LOCK_ELISION */ > - > #endif > diff --git a/sysdeps/powerpc/powerpc32/sysdep.h b/sysdeps/powerpc/powerpc32/sysdep.h > index f92ab2c..0ed83d3 100644 > --- a/sysdeps/powerpc/powerpc32/sysdep.h > +++ b/sysdeps/powerpc/powerpc32/sysdep.h > @@ -88,7 +88,7 @@ GOT_LABEL: ; \ > cfi_endproc; \ > ASM_SIZE_DIRECTIVE(name) > > -#if ! IS_IN(rtld) && defined (ENABLE_LOCK_ELISION) > +#if ! IS_IN(rtld) > # define ABORT_TRANSACTION \ > cmpwi 2,0; \ > beq 1f; \ > diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h > index db7c1d7..79a2697 100644 > --- a/sysdeps/powerpc/powerpc64/sysdep.h > +++ b/sysdeps/powerpc/powerpc64/sysdep.h > @@ -272,7 +272,7 @@ LT_LABELSUFFIX(name,_name_end): ; \ > TRACEBACK_MASK(name,mask) \ > END_2(name) > > -#if !IS_IN(rtld) && defined (ENABLE_LOCK_ELISION) > +#if !IS_IN(rtld) > # define ABORT_TRANSACTION \ > cmpdi 13,0; \ > beq 1f; \ > diff --git a/sysdeps/powerpc/sysdep.h b/sysdeps/powerpc/sysdep.h > index f07b959..d1a9bd9 100644 > --- a/sysdeps/powerpc/sysdep.h > +++ b/sysdeps/powerpc/sysdep.h > @@ -21,10 +21,8 @@ > */ > #define _SYSDEPS_SYSDEP_H 1 > #include <bits/hwcap.h> > -#ifdef ENABLE_LOCK_ELISION > #include <tls.h> > #include <htm.h> > -#endif > > #define PPC_FEATURE_970 (PPC_FEATURE_POWER4 + PPC_FEATURE_HAS_ALTIVEC) > > @@ -176,7 +174,7 @@ > we abort transaction just before syscalls. > > [1] Documentation/powerpc/transactional_memory.txt [Syscalls] */ > -#if !IS_IN(rtld) && defined (ENABLE_LOCK_ELISION) > +#if !IS_IN(rtld) > # define ABORT_TRANSACTION \ > ({ \ > if (THREAD_GET_TM_CAPABLE ()) \ > diff --git a/sysdeps/s390/nptl/bits/pthreadtypes.h b/sysdeps/s390/nptl/bits/pthreadtypes.h > index 48ffdb4..88d21f0 100644 > --- a/sysdeps/s390/nptl/bits/pthreadtypes.h > +++ b/sysdeps/s390/nptl/bits/pthreadtypes.h > @@ -89,37 +89,25 @@ typedef union > binary compatibility with static initializers. */ > int __kind; > #if __WORDSIZE == 64 > -# ifdef ENABLE_LOCK_ELISION > short __spins; > short __elision; > /* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */ > -# define __PTHREAD_SPINS 0, 0 > -# else > - int __spins; > - /* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */ > -# define __PTHREAD_SPINS 0 > -# endif > +# define __PTHREAD_SPINS 0, 0 > __pthread_list_t __list; > # define __PTHREAD_MUTEX_HAVE_PREV 1 > #else > unsigned int __nusers; > __extension__ union > { > -# ifdef ENABLE_LOCK_ELISION > struct > { > short __espins; > short __elision; > } _d; > -# define __spins _d.__espins > -# define __elision _d.__elision > - /* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */ > -# define __PTHREAD_SPINS { 0, 0 } > -# else > - int __spins; > - /* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */ > -# define __PTHREAD_SPINS 0 > -# endif > +# define __spins _d.__espins > +# define __elision _d.__elision > + /* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */ > +# define __PTHREAD_SPINS { 0, 0 } > __pthread_slist_t __list; > }; > #endif Adhemerval's commit "Move shared pthread definitions to common headers" consolidates the pthradtypes.h files. Thus sysdeps/s390/nptl/bits/pthreadtypes.h does not exist anymore. I've adjusted sysdeps/s390/nptl/bits/pthreadtypes-arch.h for my test: diff --git a/sysdeps/s390/nptl/bits/pthreadtypes-arch.h b/sysdeps/s390/nptl/bits/pthreadtypes-arch.h index 3a9ac57..db7195a 100644 --- a/sysdeps/s390/nptl/bits/pthreadtypes-arch.h +++ b/sysdeps/s390/nptl/bits/pthreadtypes-arch.h @@ -40,11 +40,7 @@ /* Definitions for internal mutex struct. */ #define __PTHREAD_COMPAT_PADDING_MID #define __PTHREAD_COMPAT_PADDING_END -#ifdef ENABLE_LOCK_ELISION #define __PTHREAD_MUTEX_LOCK_ELISION 1 -#else -#define __PTHREAD_MUTEX_LOCK_ELISION 0 -#endif #define __LOCK_ALIGNMENT #define __ONCE_ALIGNMENT