mbox series

[0/7] Disable lazy tlsdesc on aarch64 and arm

Message ID 59EF4CEC.8020301@arm.com
Headers show
Series Disable lazy tlsdesc on aarch64 and arm | expand

Message

Szabolcs Nagy Oct. 24, 2017, 2:23 p.m. UTC
Reorganized the aarch64 patches a bit and added patches for arm too.

The reason behind this change is the data race BZ #18034 on aarch64
and equivalent BZ #18572 on arm.  The aarch64 bug was fixed by adding
synchronization but that causes significant performance regression
of tls access. Lazy initialization of tlsdesc was never justified,
so i'm proposing to remove it on weakly ordered memory architectures.
(For consistency i think it should be removed on x86 too, but i don't
have patches for that.)

Szabolcs Nagy (7):
  Mark lazy tlsdesc helper functions unused to avoid warnings
  aarch64: Disable lazy symbol binding of TLSDESC
  aarch64: Remove barriers from TLS descriptor functions
  [BZ #17078] arm: remove prelinker support for R_ARM_TLS_DESC
  [BZ #18572] arm: Disable lazy initialization of tlsdesc entries
  arm: Remove unnecessary volatile qualifier
  arm: Remove lazy tlsdesc initialization related code

 elf/tlsdeschtab.h            |   2 +
 sysdeps/aarch64/dl-machine.h |  23 +++--
 sysdeps/aarch64/dl-tlsdesc.S | 203 -------------------------------------------
 sysdeps/aarch64/dl-tlsdesc.h |   9 --
 sysdeps/aarch64/tlsdesc.c    | 127 +--------------------------
 sysdeps/arm/dl-machine.h     |  59 ++++---------
 sysdeps/arm/dl-tlsdesc.S     |  84 ------------------
 sysdeps/arm/dl-tlsdesc.h     |   4 +-
 sysdeps/arm/tlsdesc.c        | 119 +------------------------
 9 files changed, 35 insertions(+), 595 deletions(-)

Comments

H.J. Lu Oct. 24, 2017, 2:54 p.m. UTC | #1
On Tue, Oct 24, 2017 at 7:23 AM, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> Reorganized the aarch64 patches a bit and added patches for arm too.
>
> The reason behind this change is the data race BZ #18034 on aarch64
> and equivalent BZ #18572 on arm.  The aarch64 bug was fixed by adding
> synchronization but that causes significant performance regression
> of tls access. Lazy initialization of tlsdesc was never justified,
> so i'm proposing to remove it on weakly ordered memory architectures.
> (For consistency i think it should be removed on x86 too, but i don't
> have patches for that.)
>
> Szabolcs Nagy (7):
>   Mark lazy tlsdesc helper functions unused to avoid warnings
>   aarch64: Disable lazy symbol binding of TLSDESC
>   aarch64: Remove barriers from TLS descriptor functions
>   [BZ #17078] arm: remove prelinker support for R_ARM_TLS_DESC
>   [BZ #18572] arm: Disable lazy initialization of tlsdesc entries
>   arm: Remove unnecessary volatile qualifier
>   arm: Remove lazy tlsdesc initialization related code
>
>

TLSDESC is very rarely used/tested.   Should we remove lazy binding
TLSDESC for all targets? It will simplify TLSDESC code.
Szabolcs Nagy Oct. 24, 2017, 4:03 p.m. UTC | #2
On 24/10/17 15:54, H.J. Lu wrote:
> On Tue, Oct 24, 2017 at 7:23 AM, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>> Reorganized the aarch64 patches a bit and added patches for arm too.
>>
>> The reason behind this change is the data race BZ #18034 on aarch64
>> and equivalent BZ #18572 on arm.  The aarch64 bug was fixed by adding
>> synchronization but that causes significant performance regression
>> of tls access. Lazy initialization of tlsdesc was never justified,
>> so i'm proposing to remove it on weakly ordered memory architectures.
>> (For consistency i think it should be removed on x86 too, but i don't
>> have patches for that.)
>>
>> Szabolcs Nagy (7):
>>   Mark lazy tlsdesc helper functions unused to avoid warnings
>>   aarch64: Disable lazy symbol binding of TLSDESC
>>   aarch64: Remove barriers from TLS descriptor functions
>>   [BZ #17078] arm: remove prelinker support for R_ARM_TLS_DESC
>>   [BZ #18572] arm: Disable lazy initialization of tlsdesc entries
>>   arm: Remove unnecessary volatile qualifier
>>   arm: Remove lazy tlsdesc initialization related code
>>
>>
> 
> TLSDESC is very rarely used/tested.   Should we remove lazy binding
> TLSDESC for all targets? It will simplify TLSDESC code.
> 

i think removing it for all targets makes sense.

i think in glibc a small bit of generic code and a
big chunk of target specific asm + c can be removed.

with the approach in this patch, targets can do the
cleanup independently (by doing symbol binding for
R_*_TLSDESC in their elf_machine_lazy_rel)

another approach is to do the non-lazy code path for
for R_*_TLSDESC in the generic elf/do-rel.h code,
however that seems more messy given how that code is
structured.

i haven't yet looked into the bfd linker, i suspect
if tlsdesc relocs are moved to the non-lazy reloc
list then the tlsdesc plt sequence and some related
code in the linker may be removed. (but i dont know
if that is valid to do or if it is abi).
Joseph Myers Oct. 24, 2017, 11:29 p.m. UTC | #3
The ARM patches are OK.