[RFC] tunables: Add elision tunable
Commit Message
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
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!
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.
@@ -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.
@@ -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
@@ -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
@@ -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
@@ -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)
@@ -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