diff mbox

[RFC] tunables: Add elision tunable

Message ID 605c6017-4e5d-553a-d1ff-36b6480e7313@linux.vnet.ibm.com
State Not applicable
Headers

Commit Message

Stefan Liebler Nov. 21, 2017, 12:56 p.m. UTC 
  On 11/16/2017 07:44 PM, Rogerio Alves wrote:
> Hi Carlos,
> 
> All documentation suggestions was made on patch v5. Thanks for the review.
> 
> Regards
> 
> Rogerio.
> 

Hi Rogerio,

I've applied your patch in order to test it on s390.
Unfortunately building glibc is now failing.

Here are some notes:

git apply warned:
0001-Add-elision-tunables.patch:291: trailing whitespace.
# Disable elision for tst-mutex8 so it can verify error case for
warning: 1 line adds whitespace errors.

Changing configure.ac is not sufficient.
Please regenerate configure in order to reflect the changes in configure.ac.

There are also usages of enable_lock_elision and ENABLE_LOCK_ELISION in 
config.make.in, sysdeps/unix/sysv/linux/s390/Makefile, 
sysdeps/s390/nptl/bits/pthreadtypes-arch.h, sysdeps/s390/configure.ac.

Please have a look into attached diff which applies on top of your patch 
v5. With those changes I was able to build glibc on s390.

Afterwards I've run a small test program which determines if 
pthread_mutex_lock is using a transaction or not:

./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=0 ./prog
Lock was a normal lock!

chmod +s prog_secure
GLIBC_TUNABLES=glibc.elision.enable=1 ./prog_secure
Lock was a normal lock!

Bye.
Stefan

Comments

Tulio Magno Quites Machado Filho Dec. 5, 2017, 7:52 p.m. UTC | #1 
Stefan Liebler <stli@linux.vnet.ibm.com> writes:

> On 11/16/2017 07:44 PM, Rogerio Alves wrote:
>> All documentation suggestions was made on patch v5. Thanks for the review.
>> 
>> Regards
>> 
>> Rogerio.
>> 
>
> I've applied your patch in order to test it on s390.
> Unfortunately building glibc is now failing.
>
> Here are some notes:
>
> git apply warned:
> 0001-Add-elision-tunables.patch:291: trailing whitespace.
> # Disable elision for tst-mutex8 so it can verify error case for
> warning: 1 line adds whitespace errors.
>
> Changing configure.ac is not sufficient.
> Please regenerate configure in order to reflect the changes in configure.ac.
>
> There are also usages of enable_lock_elision and ENABLE_LOCK_ELISION in 
> config.make.in, sysdeps/unix/sysv/linux/s390/Makefile, 
> sysdeps/s390/nptl/bits/pthreadtypes-arch.h, sysdeps/s390/configure.ac.
>
> Please have a look into attached diff which applies on top of your patch 
> v5. With those changes I was able to build glibc on s390.
>
> Afterwards I've run a small test program which determines if 
> pthread_mutex_lock is using a transaction or not:
>
> ./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=0 ./prog
> Lock was a normal lock!
>
> chmod +s prog_secure
> GLIBC_TUNABLES=glibc.elision.enable=1 ./prog_secure
> Lock was a normal lock!

I applied this patch, updated the ChangeLog, run tests on ppc{|64|64le},
s390x and x86_64, and pushed the patch as 07ed18d26a342.

Thanks!
Aurelien Jarno Jan. 9, 2018, 3:54 p.m. UTC | #2 
On 2017-12-05 17:52, Tulio Magno Quites Machado Filho wrote:
> Stefan Liebler <stli@linux.vnet.ibm.com> writes:
> 
> > On 11/16/2017 07:44 PM, Rogerio Alves wrote:
> >> All documentation suggestions was made on patch v5. Thanks for the review.
> >> 
> >> Regards
> >> 
> >> Rogerio.
> >> 
> >
> > I've applied your patch in order to test it on s390.
> > Unfortunately building glibc is now failing.
> >
> > Here are some notes:
> >
> > git apply warned:
> > 0001-Add-elision-tunables.patch:291: trailing whitespace.
> > # Disable elision for tst-mutex8 so it can verify error case for
> > warning: 1 line adds whitespace errors.
> >
> > Changing configure.ac is not sufficient.
> > Please regenerate configure in order to reflect the changes in configure.ac.
> >
> > There are also usages of enable_lock_elision and ENABLE_LOCK_ELISION in 
> > config.make.in, sysdeps/unix/sysv/linux/s390/Makefile, 
> > sysdeps/s390/nptl/bits/pthreadtypes-arch.h, sysdeps/s390/configure.ac.
> >
> > Please have a look into attached diff which applies on top of your patch 
> > v5. With those changes I was able to build glibc on s390.
> >
> > Afterwards I've run a small test program which determines if 
> > pthread_mutex_lock is using a transaction or not:
> >
> > ./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=0 ./prog
> > Lock was a normal lock!
> >
> > chmod +s prog_secure
> > GLIBC_TUNABLES=glibc.elision.enable=1 ./prog_secure
> > Lock was a normal lock!
> 
> I applied this patch, updated the ChangeLog, run tests on ppc{|64|64le},
> s390x and x86_64, and pushed the patch as 07ed18d26a342.

This commit cause the following tests to fail with the default
configuration on ppc{|64|64le}:
FAIL: elf/tst-env-setuid
FAIL: elf/tst-env-setuid-tunables
FAIL: stdlib/tst-secure-getenv

The problem is not new and it was possible to observe it with glibc-2.25
with --enable-tunables --enable-lock-elision=yes. Now --enable-tunables
is the default and following this commit lock elision code is always
there.

Please see https://sourceware.org/bugzilla/show_bug.cgi?id=22685 for
more details.
diff mbox

Patch

diff --git a/config.make.in b/config.make.in
index bd84a57..9da77d1 100644
--- a/config.make.in
+++ b/config.make.in
@@ -99,7 +99,6 @@  build-nscd = @build_nscd@
 use-nscd = @use_nscd@
 build-hardcoded-path-in-tests= @hardcoded_path_in_tests@
 build-pt-chown = @build_pt_chown@
-enable-lock-elision = @enable_lock_elision@
 have-tunables = @have_tunables@
 
 # Build tools.
diff --git a/configure b/configure
index d9d9243..47431bd 100755
--- a/configure
+++ b/configure
@@ -679,7 +679,6 @@  enable_werror
 all_warnings
 force_install
 bindnow
-enable_lock_elision
 hardcoded_path_in_tests
 enable_timezone_tools
 use_default_link
@@ -768,7 +767,6 @@  enable_profile
 enable_timezone_tools
 enable_hardcoded_path_in_tests
 enable_stackguard_randomization
-enable_lock_elision
 enable_hidden_plt
 enable_bind_now
 enable_stack_protector
@@ -1428,8 +1426,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
   --disable-hidden-plt    do not hide internal function calls to avoid PLT
   --enable-bind-now       disable lazy relocations in DSOs
   --enable-stack-protector=[yes|no|all|strong]
@@ -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-hidden-plt was given.
 if test "${enable_hidden_plt+set}" = set; then :
   enableval=$enable_hidden_plt; hidden=$enableval
diff --git a/sysdeps/s390/configure b/sysdeps/s390/configure
index d4a4a3d..74b415f 100644
--- a/sysdeps/s390/configure
+++ b/sysdeps/s390/configure
@@ -35,7 +35,7 @@  fi
 { $as_echo "$as_me:${as_lineno-$LINENO}: result: $libc_cv_gcc_builtin_tbegin" >&5
 $as_echo "$libc_cv_gcc_builtin_tbegin" >&6; }
 
-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
 
diff --git a/sysdeps/s390/configure.ac b/sysdeps/s390/configure.ac
index 7d0b5ce..1cdb021 100644
--- a/sysdeps/s390/configure.ac
+++ b/sysdeps/s390/configure.ac
@@ -26,7 +26,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
 
diff --git a/sysdeps/s390/nptl/bits/pthreadtypes-arch.h b/sysdeps/s390/nptl/bits/pthreadtypes-arch.h
index 1ae2773..fd15931 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 __PTHREAD_MUTEX_NUSERS_AFTER_KIND  (__WORDSIZE != 64)
 #define __PTHREAD_MUTEX_USE_UNION          (__WORDSIZE != 64)
 
diff --git a/sysdeps/unix/sysv/linux/s390/Makefile b/sysdeps/unix/sysv/linux/s390/Makefile
index c5f544d..77f3852 100644
--- a/sysdeps/unix/sysv/linux/s390/Makefile
+++ b/sysdeps/unix/sysv/linux/s390/Makefile
@@ -16,7 +16,6 @@  sysdep_routines += dl-vdso
 endif
 
 ifeq ($(subdir),nptl)
-ifeq ($(enable-lock-elision),yes)
 libpthread-sysdep_routines += elision-lock elision-unlock elision-timed \
 			      elision-trylock
 
@@ -26,7 +25,6 @@  CFLAGS-elision-timed.c = $(elision-CFLAGS)
 CFLAGS-elision-trylock.c = $(elision-CFLAGS)
 CFLAGS-elision-unlock.c = $(elision-CFLAGS)
 endif
-endif
 
 ifeq ($(subdir),misc)
 tests += tst-ptrace-singleblock