Disable all lock elision unless --enable-lock-elision=yes

  Andi.

The rwlock elision support you added in 2014-06-13
appears to unconditionally enable elision for rwlock's
if the RTM-bit indicates TSX is present. That isn't what
we do for mutexes, and isn't very conservative in that
case.

Did I read the code right?

If I did, could we please put rwlock TSX usage under
the control of the --enable-lock-elision flag? I need
to easily disable TSX for all of glibc, until we can
get early microcode updates worked out, and I had to
hack things up because --enable-lock-elision no longer
controls all of the elision uses in glibc.

For example the following WIP patch does what I'm thinking.

Cheers,
Carlos.

---
I should probably patch configure.ac to change the description
of the enable option to talk about all locks.

Cheers,
Carlos.
  

Hi Carlos,

On Sat, Sep 27, 2014 at 12:40:28AM -0400, Carlos O'Donell wrote:
> Andi.
> 
> The rwlock elision support you added in 2014-06-13
> appears to unconditionally enable elision for rwlock's
> if the RTM-bit indicates TSX is present. That isn't what
> we do for mutexes, and isn't very conservative in that
> case.
> 
> Did I read the code right?

You're right. It should have been disabled in this case.

Patch looks good to me.

-Andi

> 
> diff -urN glibc-2.20.mod/sysdeps/unix/sysv/linux/x86/elision-conf.c glibc-2.20/sysdeps/unix/sysv/linux/x86/elision-conf.c
> --- glibc-2.20.mod/sysdeps/unix/sysv/linux/x86/elision-conf.c	2014-09-27 00:25:46.443462345 -0400
> +++ glibc-2.20/sysdeps/unix/sysv/linux/x86/elision-conf.c	2014-09-27 00:29:53.586615813 -0400
> @@ -62,12 +62,16 @@
>  	      char **argv  __attribute__ ((unused)),
>  	      char **environ)
>  {
> -  __elision_available = HAS_RTM;
>  #ifdef ENABLE_LOCK_ELISION
> +  __elision_available = HAS_RTM;
>    __pthread_force_elision = __libc_enable_secure ? 0 : __elision_available;
> -#endif
>    if (!HAS_RTM)
>      __elision_aconf.retry_try_xbegin = 0; /* Disable elision on rwlocks */
> +#else
> +  __elision_available = 0;
> +  __pthread_force_elision = 0;
> +  __elision_aconf.retry_try_xbegin = 0;
> +#endif
>  }
>  
>  #ifdef SHARED
  

On 09/27/2014 02:10 PM, Andi Kleen wrote:
> Hi Carlos,
> 
> On Sat, Sep 27, 2014 at 12:40:28AM -0400, Carlos O'Donell wrote:
>> Andi.
>>
>> The rwlock elision support you added in 2014-06-13
>> appears to unconditionally enable elision for rwlock's
>> if the RTM-bit indicates TSX is present. That isn't what
>> we do for mutexes, and isn't very conservative in that
>> case.
>>
>> Did I read the code right?
> 
> You're right. It should have been disabled in this case.
> 
> Patch looks good to me.

Andi,

Thanks. I'll get that checked in shortly. I just wanted to make
sure that we weren't doing it for any other reason.

I'll adjust the wording of the configure switch to indicate it
is for all elision usage in the library.

Cheers,
Carlos.
  

> Andi,
> 
> Thanks. I'll get that checked in shortly. I just wanted to make
> sure that we weren't doing it for any other reason.
> 
> I'll adjust the wording of the configure switch to indicate it
> is for all elision usage in the library.

Ok.

BTW I doubt it's really needed for the systemd problems,
as systemd afaik doesn't use rwlocks.

-Andi
  

Carlos O'Donell skrev den 29.9.2014 17:05:
> On 09/27/2014 02:10 PM, Andi Kleen wrote:
>> Hi Carlos,
>>
>> On Sat, Sep 27, 2014 at 12:40:28AM -0400, Carlos O'Donell wrote:
>>> Andi.
>>>
>>> The rwlock elision support you added in 2014-06-13
>>> appears to unconditionally enable elision for rwlock's
>>> if the RTM-bit indicates TSX is present. That isn't what
>>> we do for mutexes, and isn't very conservative in that
>>> case.
>>>
>>> Did I read the code right?
>>
>> You're right. It should have been disabled in this case.
>>
>> Patch looks good to me.
>
> Andi,
>
> Thanks. I'll get that checked in shortly. I just wanted to make
> sure that we weren't doing it for any other reason.
>
> I'll adjust the wording of the configure switch to indicate it
> is for all elision usage in the library.
>


We seem to hit problems with this in Mageia when the installer runs :/

Building glibc with the patch applied:
http://svnweb.mageia.org/packages/cauldron/glibc/current/SOURCES/glibc-2.20-disable-all-lock-elision-unless--enable-lock-elision-is-yes.patch?view=markup&pathrev=731421

and dropping "--enable-lock-elision" from configure we hit SIGILL
on an old Intel E8400 CPU running x86_64 (and this with old 20131009 
firmware) and some others:


Program received signal SIGILL, Illegal instruction.
__GI___pthread_rwlock_unlock (rwlock=rwlock@entry=0x7ffff40dcd00 
<_globalCtx.5791>) at pthread_rwlock_unlock.c:34
34        if (ELIDE_UNLOCK (rwlock->__data.__writer == 0
(gdb) bt
#0  0x00007ffff6ca0192 in __GI___pthread_rwlock_unlock 
(rwlock=rwlock@entry=0x7ffff40dcd00 <_globalCtx.5791>) at 
pthread_rwlock_unlock.c:34
#1  0x00007ffff3ec59a4 in rpmlog (ctx=0x7ffff40dcd00 <_globalCtx.5791>) 
at rpmlog.c:49
#2  0x00007ffff3ec59a4 in rpmlog (code=code@entry=4, fmt=0x7ffff4131db6 
"%s created as %s\n") at rpmlog.c:287
#3  0x00007ffff410cd4a in rpmPackageFilesInstall (suffix=<optimized 
out>, action=FA_ALTNAME, fi=0x81d9470, path=<synthetic pointer>) at 
fsm.c:756
#4  0x00007ffff410cd4a in rpmPackageFilesInstall (ts=<optimized out>, 
te=<optimized out>, files=<optimized out>, psm=psm@entry=0x5a877e0, 
failedFile=failedFile@entry=0x7fffffffd848) at fsm.c:948
#5  0x00007ffff4110725 in rpmpsmUnpack (psm=psm@entry=0x5a877e0) at 
psm.c:662
#6  0x00007ffff4110d8c in rpmpsmRun (psm=0x5a877e0, ts=0x910ca60) at 
psm.c:741
#7  0x00007ffff4110d8c in rpmpsmRun (ts=0x910ca60, 
te=te@entry=0x9464300, goal=goal@entry=PKG_INSTALL) at psm.c:842
#8  0x00007ffff4122dcc in rpmteProcess (te=0x9464300, goal=PKG_INSTALL) 
at rpmte.c:757
#9  0x00007ffff4128e7a in rpmtsRun (ts=<optimized out>) at 
transaction.c:1376
#10 0x00007ffff4128e7a in rpmtsRun (ts=0x0, okProbs=okProbs@entry=0x0, 
ignoreSet=153090448) at transaction.c:1491
#11 0x00007ffff4355530 in XS_URPM__Transaction_run (my_perl=<optimized 
out>, cv=<optimized out>) at URPM.xs:2732
#12 0x00007ffff7aee60b in Perl_pp_entersub (my_perl=0x603010) at 
pp_hot.c:2794
#13 0x00007ffff7ae6fd6 in Perl_runops_standard (my_perl=0x603010) at 
run.c:42
#14 0x00007ffff7a78ded in perl_run (oldscope=1, my_perl=0x603010) at 
perl.c:2451
#15 0x00007ffff7a78ded in perl_run (my_perl=0x603010) at perl.c:2372
#16 0x0000000000400ea9 in main (argc=11, argv=0x7fffffffdf38, 
env=0x7fffffffdf98) at perlmain.c:114


--

Thomas