powerpc: Only enable TLE with PPC_FEATURE2_HTM_NOSC
Commit Message
Linux from 3.9 through 4.2 does not abort HTM transaction on syscalls,
instead it suspend and resume it when leaving the kernel. The
side-effects of the syscall will always remain visible, even if the
transaction is aborted. This is an issue when transaction is used along
with futex syscall, on pthread_cond_wait for instance, where the futex
call might succeed but the transaction is rolled back leading the
pthread_cond object in an inconsistent state.
GLIBC used to prevent it by always aborting a transaction before issuing
a syscall. Linux 4.2 also decided to abort active transaction in
syscalls which makes the GLIBC workaround superflours. Worse, GLIBC
transaction abortion leads to a performance issue on recent kernels
where the HTM state is saved/restore lazilyi (v4.9). By aborting a
transaction on every syscalls, regardless whether a transaction has being
initiated before, GLIBS makes the kernel always save/restore HTM state
(it can not even lazily disable it after a certain number of syscall
iterations).
Because of this shortcoming, Lock Elision is just enabled when it has
been explictly set (either by tunables of by a configure switch) and if
kernel aborts HTM transactions on syscalls (PPC_FEATURE2_HTM_NOSC). It
is reported that using simple benchmark [1], the context-switch is about
5% faster by not issuing a tabort in every syscall in newer kernels.
Checked on powerpc64le-linux-gnu with 4.4.0 kernel (Ubuntu 16.04).
* sysdeps/powerpc/nptl/tcb-offsets.sym (TM_CAPABLE): Remove.
* sysdeps/powerpc/nptl/tls.h (tcbhead_t): Rename tm_capable to
__ununsed1.
(TLS_INIT_TP, TLS_DEFINE_INIT_TP): Remove tm_capable setup.
(THREAD_GET_TM_CAPABLE, THREAD_SET_TM_CAPABLE): Remove macros.
* sysdeps/powerpc/powerpc32/sysdep.h,
sysdeps/powerpc/powerpc64/sysdep.h (ABORT_TRANSACTION_IMPL,
ABORT_TRANSACTION): Remove macros.
* sysdeps/powerpc/sysdep.h (ABORT_TRANSACTION): Likewise.
* sysdeps/unix/sysv/linux/powerpc/elision-conf.c (elision_init): Set
__pthread_force_elision iff PPC_FEATURE2_HTM_NOSC is set.
* sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h,
sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
sysdeps/unix/sysv/linux/powerpc/syscall.S (ABORT_TRANSACTION): Remove
usage.
Reported-by: Breno Leitão <leitao@debian.org>
[1] https://github.com/gromero/tabort
---
ChangeLog | 18 ++++++++++++++++
sysdeps/powerpc/nptl/tcb-offsets.sym | 1 -
sysdeps/powerpc/nptl/tls.h | 13 +-----------
sysdeps/powerpc/powerpc32/sysdep.h | 17 ---------------
sysdeps/powerpc/powerpc64/sysdep.h | 17 ---------------
sysdeps/powerpc/sysdep.h | 20 ------------------
sysdeps/unix/sysv/linux/powerpc/elision-conf.c | 24 ++++++++++++++++++++++
sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h | 1 -
sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h | 1 -
sysdeps/unix/sysv/linux/powerpc/syscall.S | 1 -
10 files changed, 43 insertions(+), 70 deletions(-)
Comments
Hi Zanella,
On 08/27/2018 05:54 PM, Adhemerval Zanella wrote:
> Linux from 3.9 through 4.2 does not abort HTM transaction on syscalls,
> instead it suspend and resume it when leaving the kernel. The
> side-effects of the syscall will always remain visible, even if the
> transaction is aborted. This is an issue when transaction is used along
> with futex syscall, on pthread_cond_wait for instance, where the futex
> call might succeed but the transaction is rolled back leading the
> pthread_cond object in an inconsistent state.
>
> GLIBC used to prevent it by always aborting a transaction before issuing
> a syscall. Linux 4.2 also decided to abort active transaction in
> syscalls which makes the GLIBC workaround superflours. Worse, GLIBC
> transaction abortion leads to a performance issue on recent kernels
> where the HTM state is saved/restore lazilyi (v4.9). By aborting a
> transaction on every syscalls, regardless whether a transaction has being
> initiated before, GLIBS makes the kernel always save/restore HTM state
> (it can not even lazily disable it after a certain number of syscall
> iterations).
>
> Because of this shortcoming, Lock Elision is just enabled when it has
> been explictly set (either by tunables of by a configure switch) and if
> kernel aborts HTM transactions on syscalls (PPC_FEATURE2_HTM_NOSC). It
> is reported that using simple benchmark [1], the context-switch is about
> 5% faster by not issuing a tabort in every syscall in newer kernels.
>
> Checked on powerpc64le-linux-gnu with 4.4.0 kernel (Ubuntu 16.04).
Thanks a lot for addressing this issue.
Since the lazy mechanism was introduced in 4.9:
commit 5d176f751ee3c6eededd984ad409bff201f436a7
Author: Cyril Bur <cyrilbur@gmail.com>
Date: Wed Sep 14 18:02:16 2016 +1000
powerpc: tm: Enable transactional memory (TM) lazily for userspace
I think it's not a bad idea to mention that it was also tested on upstream
kernel (4.18).
LGTM.
Only some nits:
s/Lock Elision/Transactional Lock Elision/
s/superflours/superfluous/
s/lazilyi/lazily/
s/explictly/explicitly/
Acked-by: Gustavo Romero <gromero@linux.ibm.com>
Best regards,
Gustavo
> * sysdeps/powerpc/nptl/tcb-offsets.sym (TM_CAPABLE): Remove.
> * sysdeps/powerpc/nptl/tls.h (tcbhead_t): Rename tm_capable to
> __ununsed1.
> (TLS_INIT_TP, TLS_DEFINE_INIT_TP): Remove tm_capable setup.
> (THREAD_GET_TM_CAPABLE, THREAD_SET_TM_CAPABLE): Remove macros.
> * sysdeps/powerpc/powerpc32/sysdep.h,
> sysdeps/powerpc/powerpc64/sysdep.h (ABORT_TRANSACTION_IMPL,
> ABORT_TRANSACTION): Remove macros.
> * sysdeps/powerpc/sysdep.h (ABORT_TRANSACTION): Likewise.
> * sysdeps/unix/sysv/linux/powerpc/elision-conf.c (elision_init): Set
> __pthread_force_elision iff PPC_FEATURE2_HTM_NOSC is set.
> * sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h,
> sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
> sysdeps/unix/sysv/linux/powerpc/syscall.S (ABORT_TRANSACTION): Remove
> usage.
>
> Reported-by: Breno Leitão <leitao@debian.org>
>
> [1] https://github.com/gromero/tabort
> ---
> ChangeLog | 18 ++++++++++++++++
> sysdeps/powerpc/nptl/tcb-offsets.sym | 1 -
> sysdeps/powerpc/nptl/tls.h | 13 +-----------
> sysdeps/powerpc/powerpc32/sysdep.h | 17 ---------------
> sysdeps/powerpc/powerpc64/sysdep.h | 17 ---------------
> sysdeps/powerpc/sysdep.h | 20 ------------------
> sysdeps/unix/sysv/linux/powerpc/elision-conf.c | 24 ++++++++++++++++++++++
> sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h | 1 -
> sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h | 1 -
> sysdeps/unix/sysv/linux/powerpc/syscall.S | 1 -
> 10 files changed, 43 insertions(+), 70 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 157689f..8bf4029 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,21 @@
> +2018-08-27 Adhemerval Zanella <adhemerval.zanella@linaro.org>
> +
> + * sysdeps/powerpc/nptl/tcb-offsets.sym (TM_CAPABLE): Remove.
> + * sysdeps/powerpc/nptl/tls.h (tcbhead_t): Rename tm_capable to
> + __ununsed1.
> + (TLS_INIT_TP, TLS_DEFINE_INIT_TP): Remove tm_capable setup.
> + (THREAD_GET_TM_CAPABLE, THREAD_SET_TM_CAPABLE): Remove macros.
> + * sysdeps/powerpc/powerpc32/sysdep.h,
> + sysdeps/powerpc/powerpc64/sysdep.h (ABORT_TRANSACTION_IMPL,
> + ABORT_TRANSACTION): Remove macros.
> + * sysdeps/powerpc/sysdep.h (ABORT_TRANSACTION): Likewise.
> + * sysdeps/unix/sysv/linux/powerpc/elision-conf.c (elision_init): Set
> + __pthread_force_elision iff PPC_FEATURE2_HTM_NOSC is set.
> + * sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h,
> + sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
> + sysdeps/unix/sysv/linux/powerpc/syscall.S (ABORT_TRANSACTION): Remove
> + usage.
> +
> 2018-08-27 Joseph Myers <joseph@codesourcery.com>
>
> * sysdeps/generic/math-tests-trap-force.h: New file.
> diff --git a/sysdeps/powerpc/nptl/tcb-offsets.sym b/sysdeps/powerpc/nptl/tcb-offsets.sym
> index e5bb2b3..4c01615 100644
> --- a/sysdeps/powerpc/nptl/tcb-offsets.sym
> +++ b/sysdeps/powerpc/nptl/tcb-offsets.sym
> @@ -21,7 +21,6 @@ DSO_SLOT2 (offsetof (tcbhead_t, dso_slot2) - TLS_TCB_OFFSET - sizeof (tcbhead_
> #ifdef __powerpc64__
> TCB_AT_PLATFORM (offsetof (tcbhead_t, at_platform) - TLS_TCB_OFFSET - sizeof(tcbhead_t))
> #endif
> -TM_CAPABLE (offsetof (tcbhead_t, tm_capable) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
> #ifndef __powerpc64__
> TCB_AT_PLATFORM (offsetof (tcbhead_t, at_platform) - TLS_TCB_OFFSET - sizeof(tcbhead_t))
> PADDING (offsetof (tcbhead_t, padding) - TLS_TCB_OFFSET - sizeof(tcbhead_t))
> diff --git a/sysdeps/powerpc/nptl/tls.h b/sysdeps/powerpc/nptl/tls.h
> index f88fed5..8db970d 100644
> --- a/sysdeps/powerpc/nptl/tls.h
> +++ b/sysdeps/powerpc/nptl/tls.h
> @@ -67,8 +67,7 @@ typedef struct
> uint32_t padding;
> uint32_t at_platform;
> #endif
> - /* Indicate if HTM capable (ISA 2.07). */
> - uint32_t tm_capable;
> + uint32_t __unused1;
> /* Reservation for AT_PLATFORM data - powerpc64. */
> #ifdef __powerpc64__
> uint32_t at_platform;
> @@ -142,7 +141,6 @@ register void *__thread_register __asm__ ("r13");
> # define TLS_INIT_TP(tcbp) \
> ({ \
> __thread_register = (void *) (tcbp) + TLS_TCB_OFFSET; \
> - THREAD_SET_TM_CAPABLE (__tcb_hwcap & PPC_FEATURE2_HAS_HTM ? 1 : 0); \
> THREAD_SET_HWCAP (__tcb_hwcap); \
> THREAD_SET_AT_PLATFORM (__tcb_platform); \
> NULL; \
> @@ -151,8 +149,6 @@ register void *__thread_register __asm__ ("r13");
> /* Value passed to 'clone' for initialization of the thread register. */
> # define TLS_DEFINE_INIT_TP(tp, pd) \
> void *tp = (void *) (pd) + TLS_TCB_OFFSET + TLS_PRE_TCB_SIZE; \
> - (((tcbhead_t *) ((char *) tp - TLS_TCB_OFFSET))[-1].tm_capable) = \
> - THREAD_GET_TM_CAPABLE (); \
> (((tcbhead_t *) ((char *) tp - TLS_TCB_OFFSET))[-1].hwcap) = \
> THREAD_GET_HWCAP (); \
> (((tcbhead_t *) ((char *) tp - TLS_TCB_OFFSET))[-1].at_platform) = \
> @@ -210,13 +206,6 @@ register void *__thread_register __asm__ ("r13");
> + TLS_PRE_TCB_SIZE))[-1].pointer_guard \
> = THREAD_GET_POINTER_GUARD())
>
> -/* tm_capable field in TCB head. */
> -# define THREAD_GET_TM_CAPABLE() \
> - (((tcbhead_t *) ((char *) __thread_register \
> - - TLS_TCB_OFFSET))[-1].tm_capable)
> -# define THREAD_SET_TM_CAPABLE(value) \
> - (THREAD_GET_TM_CAPABLE () = (value))
> -
> /* hwcap field in TCB head. */
> # define THREAD_GET_HWCAP() \
> (((tcbhead_t *) ((char *) __thread_register \
> diff --git a/sysdeps/powerpc/powerpc32/sysdep.h b/sysdeps/powerpc/powerpc32/sysdep.h
> index 5f1294e..93097c5 100644
> --- a/sysdeps/powerpc/powerpc32/sysdep.h
> +++ b/sysdeps/powerpc/powerpc32/sysdep.h
> @@ -90,24 +90,7 @@ GOT_LABEL: ; \
> cfi_endproc; \
> ASM_SIZE_DIRECTIVE(name)
>
> -#if !IS_IN(rtld) && !defined(__SPE__)
> -# define ABORT_TRANSACTION_IMPL \
> - cmpwi 2,0; \
> - beq 1f; \
> - lwz 0,TM_CAPABLE(2); \
> - cmpwi 0,0; \
> - beq 1f; \
> - li 11,_ABORT_SYSCALL; \
> - tabort. 11; \
> - .align 4; \
> -1:
> -#else
> -# define ABORT_TRANSACTION_IMPL
> -#endif
> -#define ABORT_TRANSACTION ABORT_TRANSACTION_IMPL
> -
> #define DO_CALL(syscall) \
> - ABORT_TRANSACTION \
> li 0,syscall; \
> sc
>
> diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h
> index 2df1d9b..50e64f9 100644
> --- a/sysdeps/powerpc/powerpc64/sysdep.h
> +++ b/sysdeps/powerpc/powerpc64/sysdep.h
> @@ -263,24 +263,7 @@ LT_LABELSUFFIX(name,_name_end): ; \
> TRACEBACK_MASK(name,mask); \
> END_2(name)
>
> -#if !IS_IN(rtld)
> -# define ABORT_TRANSACTION_IMPL \
> - cmpdi 13,0; \
> - beq 1f; \
> - lwz 0,TM_CAPABLE(13); \
> - cmpwi 0,0; \
> - beq 1f; \
> - li 11,_ABORT_SYSCALL; \
> - tabort. 11; \
> - .p2align 4; \
> -1:
> -#else
> -# define ABORT_TRANSACTION_IMPL
> -#endif
> -#define ABORT_TRANSACTION ABORT_TRANSACTION_IMPL
> -
> #define DO_CALL(syscall) \
> - ABORT_TRANSACTION \
> li 0,syscall; \
> sc
>
> diff --git a/sysdeps/powerpc/sysdep.h b/sysdeps/powerpc/sysdep.h
> index 8a6d236..c8bf25e 100644
> --- a/sysdeps/powerpc/sysdep.h
> +++ b/sysdeps/powerpc/sysdep.h
> @@ -21,8 +21,6 @@
> */
> #define _SYSDEPS_SYSDEP_H 1
> #include <bits/hwcap.h>
> -#include <tls.h>
> -#include <htm.h>
>
> #define PPC_FEATURE_970 (PPC_FEATURE_POWER4 + PPC_FEATURE_HAS_ALTIVEC)
>
> @@ -166,22 +164,4 @@
> #define ALIGNARG(log2) log2
> #define ASM_SIZE_DIRECTIVE(name) .size name,.-name
>
> -#else
> -
> -/* Linux kernel powerpc documentation [1] states issuing a syscall inside a
> - transaction is not recommended and may lead to undefined behavior. It
> - also states syscalls do not abort transactions. To avoid such traps,
> - we abort transaction just before syscalls.
> -
> - [1] Documentation/powerpc/transactional_memory.txt [Syscalls] */
> -#if !IS_IN(rtld) && !defined(__SPE__)
> -# define ABORT_TRANSACTION \
> - ({ \
> - if (THREAD_GET_TM_CAPABLE ()) \
> - __libc_tabort (_ABORT_SYSCALL); \
> - })
> -#else
> -# define ABORT_TRANSACTION
> -#endif
> -
> #endif /* __ASSEMBLER__ */
> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
> index 906882a..508b917 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
> @@ -127,6 +127,30 @@ elision_init (int argc __attribute__ ((unused)),
> TUNABLE_CALLBACK (set_elision_skip_trylock_internal_abort));
> #endif
>
> + /* Linux from 3.9 through 4.2 does not abort HTM transaction on syscalls,
> + instead it suspend and resume it when leaving the kernel. The
> + side-effects of the syscall will always remain visible, even if the
> + transaction is aborted. This is an issue when transaction is used along
> + with futex syscall, on pthread_cond_wait for instance, where the futex
> + call might succeed but the transaction is rolled back leading the
> + pthread_cond object in an inconsistent state.
> +
> + GLIBC used to prevent it by always aborting a transaction before issuing
> + a syscall. Linux 4.2 also decided to abort active transaction in
> + syscalls which makes the GLIBC workaround superflours. Worse, GLIBC
> + transaction abortion leads to a performance issue on recent kernels
> + where the HTM state is saved/restore lazily. By aborting a transaction
> + on every syscalls, regardless whether a transaction has being initiated
> + before, glibc make the kernel always save/restore HTM state (it can not
> + even lazily disable it after a certain number of syscall iterations).
> +
> + Because of this shortcoming, Lock Elision is just enabled when it has
> + been explictly set (either by tunables of by a configure switch) and if
> + kernel aborts HTM transactions on syscalls (PPC_FEATURE2_HTM_NOSC) */
> +
> + __pthread_force_elision = __pthread_force_elision &&
> + GLRO (dl_hwcap2) & PPC_FEATURE2_HTM_NOSC;
> +
> if (!__pthread_force_elision)
> __elision_aconf.try_tbegin = 0; /* Disable elision on rwlocks. */
> }
> diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
> index f7277d5..ec5c525 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
> @@ -109,7 +109,6 @@
> register long int r11 __asm__ ("r11"); \
> register long int r12 __asm__ ("r12"); \
> LOADARGS_##nr(name, args); \
> - ABORT_TRANSACTION; \
> __asm__ __volatile__ \
> ("sc \n\t" \
> "mfcr %0" \
> diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
> index 0956cf0..1f17f7b 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
> @@ -131,7 +131,6 @@
> register long int r7 __asm__ ("r7"); \
> register long int r8 __asm__ ("r8"); \
> LOADARGS_##nr (name, ##args); \
> - ABORT_TRANSACTION; \
> __asm__ __volatile__ \
> ("sc\n\t" \
> "mfcr %0\n\t" \
> diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S b/sysdeps/unix/sysv/linux/powerpc/syscall.S
> index 2da9172..bbab613 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
> +++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
> @@ -18,7 +18,6 @@
> #include <sysdep.h>
>
> ENTRY (syscall)
> - ABORT_TRANSACTION
> mr r0,r3
> mr r3,r4
> mr r4,r5
>
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> diff --git a/sysdeps/powerpc/nptl/tls.h b/sysdeps/powerpc/nptl/tls.h
> index f88fed5..8db970d 100644
> --- a/sysdeps/powerpc/nptl/tls.h
> +++ b/sysdeps/powerpc/nptl/tls.h
> @@ -67,8 +67,7 @@ typedef struct
> uint32_t padding;
> uint32_t at_platform;
> #endif
> - /* Indicate if HTM capable (ISA 2.07). */
> - uint32_t tm_capable;
> + uint32_t __unused1;
Is the TCB part of the library ABI?
> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
> index 906882a..508b917 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
> @@ -127,6 +127,30 @@ elision_init (int argc __attribute__ ((unused)),
> TUNABLE_CALLBACK (set_elision_skip_trylock_internal_abort));
> #endif
>
> + /* Linux from 3.9 through 4.2 does not abort HTM transaction on syscalls,
> + instead it suspend and resume it when leaving the kernel. The
> + side-effects of the syscall will always remain visible, even if the
> + transaction is aborted. This is an issue when transaction is used along
> + with futex syscall, on pthread_cond_wait for instance, where the futex
> + call might succeed but the transaction is rolled back leading the
> + pthread_cond object in an inconsistent state.
> +
> + GLIBC used to prevent it by always aborting a transaction before issuing
> + a syscall. Linux 4.2 also decided to abort active transaction in
> + syscalls which makes the GLIBC workaround superflours. Worse, GLIBC
> + transaction abortion leads to a performance issue on recent kernels
> + where the HTM state is saved/restore lazily. By aborting a transaction
> + on every syscalls, regardless whether a transaction has being initiated
> + before, glibc make the kernel always save/restore HTM state (it can not
> + even lazily disable it after a certain number of syscall iterations).
> +
> + Because of this shortcoming, Lock Elision is just enabled when it has
> + been explictly set (either by tunables of by a configure switch) and if
> + kernel aborts HTM transactions on syscalls (PPC_FEATURE2_HTM_NOSC) */
> +
> + __pthread_force_elision = __pthread_force_elision &&
> + GLRO (dl_hwcap2) & PPC_FEATURE2_HTM_NOSC;
In other words: this patch is requiring Linux 4.3 for TLE.
Can't ABORT_TRANSACTION check for PPC_FEATURE2_HTM_NOSC in tcbhead_t.hwcap and
continue to support older Linux versions?
On 27/08/2018 19:44, Tulio Magno Quites Machado Filho wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>
>> diff --git a/sysdeps/powerpc/nptl/tls.h b/sysdeps/powerpc/nptl/tls.h
>> index f88fed5..8db970d 100644
>> --- a/sysdeps/powerpc/nptl/tls.h
>> +++ b/sysdeps/powerpc/nptl/tls.h
>> @@ -67,8 +67,7 @@ typedef struct
>> uint32_t padding;
>> uint32_t at_platform;
>> #endif
>> - /* Indicate if HTM capable (ISA 2.07). */
>> - uint32_t tm_capable;
>> + uint32_t __unused1;
>
> Is the TCB part of the library ABI?
>
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
>> index 906882a..508b917 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
>> +++ b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
>> @@ -127,6 +127,30 @@ elision_init (int argc __attribute__ ((unused)),
>> TUNABLE_CALLBACK (set_elision_skip_trylock_internal_abort));
>> #endif
>>
>> + /* Linux from 3.9 through 4.2 does not abort HTM transaction on syscalls,
In fact it should be 4.1, 4.2 is the first release which actually abort transactions
on syscalls.
>> + instead it suspend and resume it when leaving the kernel. The
>> + side-effects of the syscall will always remain visible, even if the
>> + transaction is aborted. This is an issue when transaction is used along
>> + with futex syscall, on pthread_cond_wait for instance, where the futex
>> + call might succeed but the transaction is rolled back leading the
>> + pthread_cond object in an inconsistent state.
>> +
>> + GLIBC used to prevent it by always aborting a transaction before issuing
>> + a syscall. Linux 4.2 also decided to abort active transaction in
>> + syscalls which makes the GLIBC workaround superflours. Worse, GLIBC
>> + transaction abortion leads to a performance issue on recent kernels
>> + where the HTM state is saved/restore lazily. By aborting a transaction
>> + on every syscalls, regardless whether a transaction has being initiated
>> + before, glibc make the kernel always save/restore HTM state (it can not
>> + even lazily disable it after a certain number of syscall iterations).
>> +
>> + Because of this shortcoming, Lock Elision is just enabled when it has
>> + been explictly set (either by tunables of by a configure switch) and if
>> + kernel aborts HTM transactions on syscalls (PPC_FEATURE2_HTM_NOSC) */
>> +
>> + __pthread_force_elision = __pthread_force_elision &&
>> + GLRO (dl_hwcap2) & PPC_FEATURE2_HTM_NOSC;
>
> In other words: this patch is requiring Linux 4.3 for TLE.
>
> Can't ABORT_TRANSACTION check for PPC_FEATURE2_HTM_NOSC in tcbhead_t.hwcap and
> continue to support older Linux versions?
>
I don't have a strong opinion here, but chatting with Breno I had the impression
that TLE (and HTM in general) is only used on kernel with this recent fix. This
patch is really a hack insertion that penalizes all syscall for an specific
feature which may not be present in the kernel. It is unfortunate that kernel
developers did not realize at feature inclusion this design for transaction
state is not the best one for library usage.
Anyhow, another possible fix would be to set tcbhead::tm_capable to 0 in case
of PPC_FEATURE2_HTM_NOSC (and also maybe rename to something like tm_nosc).
On 27/08/2018 19:44, Tulio Magno Quites Machado Filho wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>
>> diff --git a/sysdeps/powerpc/nptl/tls.h b/sysdeps/powerpc/nptl/tls.h
>> index f88fed5..8db970d 100644
>> --- a/sysdeps/powerpc/nptl/tls.h
>> +++ b/sysdeps/powerpc/nptl/tls.h
>> @@ -67,8 +67,7 @@ typedef struct
>> uint32_t padding;
>> uint32_t at_platform;
>> #endif
>> - /* Indicate if HTM capable (ISA 2.07). */
>> - uint32_t tm_capable;
>> + uint32_t __unused1;
>
> Is the TCB part of the library ABI?
Not the field itself, but afaik other members are accessed by libgcc
and by removing it at_platform offset will be change for powerpc (not
sure if this really an issue for libgcc).
Hi, Adhemerval,
Just an FYI... Tulio is on leave until next week, so he'll probably only
get back to you later. :)
On Tue, 28 Aug 2018, Adhemerval Zanella wrote:
>On 27/08/2018 19:44, Tulio Magno Quites Machado Filho wrote:
>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>
>>> diff --git a/sysdeps/powerpc/nptl/tls.h b/sysdeps/powerpc/nptl/tls.h
>>> index f88fed5..8db970d 100644
>>> --- a/sysdeps/powerpc/nptl/tls.h
>>> +++ b/sysdeps/powerpc/nptl/tls.h
>>> @@ -67,8 +67,7 @@ typedef struct
>>> uint32_t padding;
>>> uint32_t at_platform;
>>> #endif
>>> - /* Indicate if HTM capable (ISA 2.07). */
>>> - uint32_t tm_capable;
>>> + uint32_t __unused1;
>>
>> Is the TCB part of the library ABI?
>
>Not the field itself, but afaik other members are accessed by libgcc
>and by removing it at_platform offset will be change for powerpc (not
>sure if this really an issue for libgcc).
On 8/27/18, 4:44 PM, "Tulio Magno Quites Machado Filho" <libc-alpha-owner@sourceware.org on behalf of tuliom@ascii.art.br> wrote:
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> diff --git a/sysdeps/powerpc/nptl/tls.h b/sysdeps/powerpc/nptl/tls.h
> index f88fed5..8db970d 100644
> --- a/sysdeps/powerpc/nptl/tls.h
> +++ b/sysdeps/powerpc/nptl/tls.h
> @@ -67,8 +67,7 @@ typedef struct
> uint32_t padding;
> uint32_t at_platform;
> #endif
> - /* Indicate if HTM capable (ISA 2.07). */
> - uint32_t tm_capable;
> + uint32_t __unused1;
Is the TCB part of the library ABI?
Not this field, but if you remove it, you will mess with the tcb symbol offsets, and gcc's __builtin_cpu_is/supports rely on the hwcap and at_platform fields here, IIRC.
--
Carlos Eduardo Seo
Hi Adhemerval,
On 08/28/2018 08:48 AM, Adhemerval Zanella wrote:
>
>
> On 27/08/2018 19:44, Tulio Magno Quites Machado Filho wrote:
>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
<snip>
>> Can't ABORT_TRANSACTION check for PPC_FEATURE2_HTM_NOSC in tcbhead_t.hwcap and
>> continue to support older Linux versions?
>>
>
> I don't have a strong opinion here, but chatting with Breno I had the impression
> that TLE (and HTM in general) is only used on kernel with this recent fix. This
> patch is really a hack insertion that penalizes all syscall for an specific
> feature which may not be present in the kernel.
In fact, it is more than a penalization (which is also there). The current
design seems to be incoherent with the Kernel design[1] and limiting the
feature usage. By the kernel design, a syscall executed inside a suspended
transaction is allowed, as said by the kernel documentation[1]:
"Syscalls made from within a suspended transaction are performed as normal and
the transaction is not explicitly doomed by the kernel"
The statement above becomes invalid if the syscall is performed through glibc
nowadays, which I understand is an undesired behavior.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/powerpc/transactional_memory.txt#n81
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> On 27/08/2018 19:44, Tulio Magno Quites Machado Filho wrote:
>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>
>>> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
>>> index 906882a..508b917 100644
>>> --- a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
>>> +++ b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
>>> @@ -127,6 +127,30 @@ elision_init (int argc __attribute__ ((unused)),
>>> TUNABLE_CALLBACK (set_elision_skip_trylock_internal_abort));
>>> #endif
>>>
>>> + /* Linux from 3.9 through 4.2 does not abort HTM transaction on syscalls,
>
> In fact it should be 4.1, 4.2 is the first release which actually abort transactions
> on syscalls.
Ack.
>>> + instead it suspend and resume it when leaving the kernel. The
>>> + side-effects of the syscall will always remain visible, even if the
>>> + transaction is aborted. This is an issue when transaction is used along
>>> + with futex syscall, on pthread_cond_wait for instance, where the futex
>>> + call might succeed but the transaction is rolled back leading the
>>> + pthread_cond object in an inconsistent state.
>>> +
>>> + GLIBC used to prevent it by always aborting a transaction before issuing
>>> + a syscall. Linux 4.2 also decided to abort active transaction in
>>> + syscalls which makes the GLIBC workaround superflours. Worse, GLIBC
>>> + transaction abortion leads to a performance issue on recent kernels
>>> + where the HTM state is saved/restore lazily. By aborting a transaction
>>> + on every syscalls, regardless whether a transaction has being initiated
>>> + before, glibc make the kernel always save/restore HTM state (it can not
>>> + even lazily disable it after a certain number of syscall iterations).
>>> +
>>> + Because of this shortcoming, Lock Elision is just enabled when it has
>>> + been explictly set (either by tunables of by a configure switch) and if
>>> + kernel aborts HTM transactions on syscalls (PPC_FEATURE2_HTM_NOSC) */
>>> +
>>> + __pthread_force_elision = __pthread_force_elision &&
>>> + GLRO (dl_hwcap2) & PPC_FEATURE2_HTM_NOSC;
>>
>> In other words: this patch is requiring Linux 4.3 for TLE.
>>
>> Can't ABORT_TRANSACTION check for PPC_FEATURE2_HTM_NOSC in tcbhead_t.hwcap and
>> continue to support older Linux versions?
>>
>
> I don't have a strong opinion here, but chatting with Breno I had the impression
> that TLE (and HTM in general) is only used on kernel with this recent fix. This
> patch is really a hack insertion that penalizes all syscall for an specific
> feature which may not be present in the kernel. It is unfortunate that kernel
> developers did not realize at feature inclusion this design for transaction
> state is not the best one for library usage.
My point was actually related to the fact that INSTALL still mentions Linux 3.2.
I completely agree this solution is better in terms of performance and
maintenance in the long term.
Maybe we (me?) could document this problem somewhere (INSTALL?) pointing
users that want to run the latest glibc and use HTM on Linux to use
Linux >= 4.2.
With the removal of ABORT_TRANSACTION, it's also necessary to remove
sysdeps/unix/sysv/linux/powerpc/not-errno.h.
With that said, my questions have already been answered and this patch looks
good to me with the fix in the comment you mentioned and the removal of
not-errno.h.
On 19/09/2018 16:20, Tulio Magno Quites Machado Filho wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>
>> On 27/08/2018 19:44, Tulio Magno Quites Machado Filho wrote:
>>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>>
>>>> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
>>>> index 906882a..508b917 100644
>>>> --- a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
>>>> +++ b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
>>>> @@ -127,6 +127,30 @@ elision_init (int argc __attribute__ ((unused)),
>>>> TUNABLE_CALLBACK (set_elision_skip_trylock_internal_abort));
>>>> #endif
>>>>
>>>> + /* Linux from 3.9 through 4.2 does not abort HTM transaction on syscalls,
>>
>> In fact it should be 4.1, 4.2 is the first release which actually abort transactions
>> on syscalls.
>
> Ack.
>
>>>> + instead it suspend and resume it when leaving the kernel. The
>>>> + side-effects of the syscall will always remain visible, even if the
>>>> + transaction is aborted. This is an issue when transaction is used along
>>>> + with futex syscall, on pthread_cond_wait for instance, where the futex
>>>> + call might succeed but the transaction is rolled back leading the
>>>> + pthread_cond object in an inconsistent state.
>>>> +
>>>> + GLIBC used to prevent it by always aborting a transaction before issuing
>>>> + a syscall. Linux 4.2 also decided to abort active transaction in
>>>> + syscalls which makes the GLIBC workaround superflours. Worse, GLIBC
>>>> + transaction abortion leads to a performance issue on recent kernels
>>>> + where the HTM state is saved/restore lazily. By aborting a transaction
>>>> + on every syscalls, regardless whether a transaction has being initiated
>>>> + before, glibc make the kernel always save/restore HTM state (it can not
>>>> + even lazily disable it after a certain number of syscall iterations).
>>>> +
>>>> + Because of this shortcoming, Lock Elision is just enabled when it has
>>>> + been explictly set (either by tunables of by a configure switch) and if
>>>> + kernel aborts HTM transactions on syscalls (PPC_FEATURE2_HTM_NOSC) */
>>>> +
>>>> + __pthread_force_elision = __pthread_force_elision &&
>>>> + GLRO (dl_hwcap2) & PPC_FEATURE2_HTM_NOSC;
>>>
>>> In other words: this patch is requiring Linux 4.3 for TLE.
>>>
>>> Can't ABORT_TRANSACTION check for PPC_FEATURE2_HTM_NOSC in tcbhead_t.hwcap and
>>> continue to support older Linux versions?
>>>
>>
>> I don't have a strong opinion here, but chatting with Breno I had the impression
>> that TLE (and HTM in general) is only used on kernel with this recent fix. This
>> patch is really a hack insertion that penalizes all syscall for an specific
>> feature which may not be present in the kernel. It is unfortunate that kernel
>> developers did not realize at feature inclusion this design for transaction
>> state is not the best one for library usage.
>
> My point was actually related to the fact that INSTALL still mentions Linux 3.2.
>
> I completely agree this solution is better in terms of performance and
> maintenance in the long term.
> Maybe we (me?) could document this problem somewhere (INSTALL?) pointing
> users that want to run the latest glibc and use HTM on Linux to use
> Linux >= 4.2.
The INSTALL file still lacks to mention that powerpc64le still requires
at least a 3.10 kernel (which is the minimum version where the ABI is
actually supported). And there still some other ABIs that does not
have support for 3.2 (aarch64 for instance).
At least, I think we start to document powerpc64le requirements: a minimum
Linux version of 3.10 and 4.2 for TLE.
>
> With the removal of ABORT_TRANSACTION, it's also necessary to remove
> sysdeps/unix/sysv/linux/powerpc/not-errno.h.
>
> With that said, my questions have already been answered and this patch looks
> good to me with the fix in the comment you mentioned and the removal of
> not-errno.h.>
I will update the patch with this removal.
@@ -1,3 +1,21 @@
+2018-08-27 Adhemerval Zanella <adhemerval.zanella@linaro.org>
+
+ * sysdeps/powerpc/nptl/tcb-offsets.sym (TM_CAPABLE): Remove.
+ * sysdeps/powerpc/nptl/tls.h (tcbhead_t): Rename tm_capable to
+ __ununsed1.
+ (TLS_INIT_TP, TLS_DEFINE_INIT_TP): Remove tm_capable setup.
+ (THREAD_GET_TM_CAPABLE, THREAD_SET_TM_CAPABLE): Remove macros.
+ * sysdeps/powerpc/powerpc32/sysdep.h,
+ sysdeps/powerpc/powerpc64/sysdep.h (ABORT_TRANSACTION_IMPL,
+ ABORT_TRANSACTION): Remove macros.
+ * sysdeps/powerpc/sysdep.h (ABORT_TRANSACTION): Likewise.
+ * sysdeps/unix/sysv/linux/powerpc/elision-conf.c (elision_init): Set
+ __pthread_force_elision iff PPC_FEATURE2_HTM_NOSC is set.
+ * sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h,
+ sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
+ sysdeps/unix/sysv/linux/powerpc/syscall.S (ABORT_TRANSACTION): Remove
+ usage.
+
2018-08-27 Joseph Myers <joseph@codesourcery.com>
* sysdeps/generic/math-tests-trap-force.h: New file.
@@ -21,7 +21,6 @@ DSO_SLOT2 (offsetof (tcbhead_t, dso_slot2) - TLS_TCB_OFFSET - sizeof (tcbhead_
#ifdef __powerpc64__
TCB_AT_PLATFORM (offsetof (tcbhead_t, at_platform) - TLS_TCB_OFFSET - sizeof(tcbhead_t))
#endif
-TM_CAPABLE (offsetof (tcbhead_t, tm_capable) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
#ifndef __powerpc64__
TCB_AT_PLATFORM (offsetof (tcbhead_t, at_platform) - TLS_TCB_OFFSET - sizeof(tcbhead_t))
PADDING (offsetof (tcbhead_t, padding) - TLS_TCB_OFFSET - sizeof(tcbhead_t))
@@ -67,8 +67,7 @@ typedef struct
uint32_t padding;
uint32_t at_platform;
#endif
- /* Indicate if HTM capable (ISA 2.07). */
- uint32_t tm_capable;
+ uint32_t __unused1;
/* Reservation for AT_PLATFORM data - powerpc64. */
#ifdef __powerpc64__
uint32_t at_platform;
@@ -142,7 +141,6 @@ register void *__thread_register __asm__ ("r13");
# define TLS_INIT_TP(tcbp) \
({ \
__thread_register = (void *) (tcbp) + TLS_TCB_OFFSET; \
- THREAD_SET_TM_CAPABLE (__tcb_hwcap & PPC_FEATURE2_HAS_HTM ? 1 : 0); \
THREAD_SET_HWCAP (__tcb_hwcap); \
THREAD_SET_AT_PLATFORM (__tcb_platform); \
NULL; \
@@ -151,8 +149,6 @@ register void *__thread_register __asm__ ("r13");
/* Value passed to 'clone' for initialization of the thread register. */
# define TLS_DEFINE_INIT_TP(tp, pd) \
void *tp = (void *) (pd) + TLS_TCB_OFFSET + TLS_PRE_TCB_SIZE; \
- (((tcbhead_t *) ((char *) tp - TLS_TCB_OFFSET))[-1].tm_capable) = \
- THREAD_GET_TM_CAPABLE (); \
(((tcbhead_t *) ((char *) tp - TLS_TCB_OFFSET))[-1].hwcap) = \
THREAD_GET_HWCAP (); \
(((tcbhead_t *) ((char *) tp - TLS_TCB_OFFSET))[-1].at_platform) = \
@@ -210,13 +206,6 @@ register void *__thread_register __asm__ ("r13");
+ TLS_PRE_TCB_SIZE))[-1].pointer_guard \
= THREAD_GET_POINTER_GUARD())
-/* tm_capable field in TCB head. */
-# define THREAD_GET_TM_CAPABLE() \
- (((tcbhead_t *) ((char *) __thread_register \
- - TLS_TCB_OFFSET))[-1].tm_capable)
-# define THREAD_SET_TM_CAPABLE(value) \
- (THREAD_GET_TM_CAPABLE () = (value))
-
/* hwcap field in TCB head. */
# define THREAD_GET_HWCAP() \
(((tcbhead_t *) ((char *) __thread_register \
@@ -90,24 +90,7 @@ GOT_LABEL: ; \
cfi_endproc; \
ASM_SIZE_DIRECTIVE(name)
-#if !IS_IN(rtld) && !defined(__SPE__)
-# define ABORT_TRANSACTION_IMPL \
- cmpwi 2,0; \
- beq 1f; \
- lwz 0,TM_CAPABLE(2); \
- cmpwi 0,0; \
- beq 1f; \
- li 11,_ABORT_SYSCALL; \
- tabort. 11; \
- .align 4; \
-1:
-#else
-# define ABORT_TRANSACTION_IMPL
-#endif
-#define ABORT_TRANSACTION ABORT_TRANSACTION_IMPL
-
#define DO_CALL(syscall) \
- ABORT_TRANSACTION \
li 0,syscall; \
sc
@@ -263,24 +263,7 @@ LT_LABELSUFFIX(name,_name_end): ; \
TRACEBACK_MASK(name,mask); \
END_2(name)
-#if !IS_IN(rtld)
-# define ABORT_TRANSACTION_IMPL \
- cmpdi 13,0; \
- beq 1f; \
- lwz 0,TM_CAPABLE(13); \
- cmpwi 0,0; \
- beq 1f; \
- li 11,_ABORT_SYSCALL; \
- tabort. 11; \
- .p2align 4; \
-1:
-#else
-# define ABORT_TRANSACTION_IMPL
-#endif
-#define ABORT_TRANSACTION ABORT_TRANSACTION_IMPL
-
#define DO_CALL(syscall) \
- ABORT_TRANSACTION \
li 0,syscall; \
sc
@@ -21,8 +21,6 @@
*/
#define _SYSDEPS_SYSDEP_H 1
#include <bits/hwcap.h>
-#include <tls.h>
-#include <htm.h>
#define PPC_FEATURE_970 (PPC_FEATURE_POWER4 + PPC_FEATURE_HAS_ALTIVEC)
@@ -166,22 +164,4 @@
#define ALIGNARG(log2) log2
#define ASM_SIZE_DIRECTIVE(name) .size name,.-name
-#else
-
-/* Linux kernel powerpc documentation [1] states issuing a syscall inside a
- transaction is not recommended and may lead to undefined behavior. It
- also states syscalls do not abort transactions. To avoid such traps,
- we abort transaction just before syscalls.
-
- [1] Documentation/powerpc/transactional_memory.txt [Syscalls] */
-#if !IS_IN(rtld) && !defined(__SPE__)
-# define ABORT_TRANSACTION \
- ({ \
- if (THREAD_GET_TM_CAPABLE ()) \
- __libc_tabort (_ABORT_SYSCALL); \
- })
-#else
-# define ABORT_TRANSACTION
-#endif
-
#endif /* __ASSEMBLER__ */
@@ -127,6 +127,30 @@ elision_init (int argc __attribute__ ((unused)),
TUNABLE_CALLBACK (set_elision_skip_trylock_internal_abort));
#endif
+ /* Linux from 3.9 through 4.2 does not abort HTM transaction on syscalls,
+ instead it suspend and resume it when leaving the kernel. The
+ side-effects of the syscall will always remain visible, even if the
+ transaction is aborted. This is an issue when transaction is used along
+ with futex syscall, on pthread_cond_wait for instance, where the futex
+ call might succeed but the transaction is rolled back leading the
+ pthread_cond object in an inconsistent state.
+
+ GLIBC used to prevent it by always aborting a transaction before issuing
+ a syscall. Linux 4.2 also decided to abort active transaction in
+ syscalls which makes the GLIBC workaround superflours. Worse, GLIBC
+ transaction abortion leads to a performance issue on recent kernels
+ where the HTM state is saved/restore lazily. By aborting a transaction
+ on every syscalls, regardless whether a transaction has being initiated
+ before, glibc make the kernel always save/restore HTM state (it can not
+ even lazily disable it after a certain number of syscall iterations).
+
+ Because of this shortcoming, Lock Elision is just enabled when it has
+ been explictly set (either by tunables of by a configure switch) and if
+ kernel aborts HTM transactions on syscalls (PPC_FEATURE2_HTM_NOSC) */
+
+ __pthread_force_elision = __pthread_force_elision &&
+ GLRO (dl_hwcap2) & PPC_FEATURE2_HTM_NOSC;
+
if (!__pthread_force_elision)
__elision_aconf.try_tbegin = 0; /* Disable elision on rwlocks. */
}
@@ -109,7 +109,6 @@
register long int r11 __asm__ ("r11"); \
register long int r12 __asm__ ("r12"); \
LOADARGS_##nr(name, args); \
- ABORT_TRANSACTION; \
__asm__ __volatile__ \
("sc \n\t" \
"mfcr %0" \
@@ -131,7 +131,6 @@
register long int r7 __asm__ ("r7"); \
register long int r8 __asm__ ("r8"); \
LOADARGS_##nr (name, ##args); \
- ABORT_TRANSACTION; \
__asm__ __volatile__ \
("sc\n\t" \
"mfcr %0\n\t" \
@@ -18,7 +18,6 @@
#include <sysdep.h>
ENTRY (syscall)
- ABORT_TRANSACTION
mr r0,r3
mr r3,r4
mr r4,r5